-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(batch): changed Type to MANAGED for ManagedEc2EcsComputeEnvironment to avoid false drift in CloudFormation. #31691
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request
. Additionally, if clarification is needed add Clarification Request
to a comment.
Exemption Request This PR just changes the |
try run $ yarn integ test/aws-stepfunctions-tasks/test/batch/integ.run-batch-job.js --update-on-failed --disable-update-workflow
$ yarn integ test/aws-stepfunctions-tasks/test/batch/integ.submit-job.js --update-on-failed --disable-update-workflow to update the snapshots |
00d6f87
to
06d5875
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you check if this applies to unmanaged compute environments as well? If it does, we should fix it there too.
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
…nt to avoid false drift in CloudFormation.
… to avoid false drift in CloudFormation.
06d5875
to
bd81d12
Compare
@comcalvi Thanks for the feedback. Added commit to correct casing for |
…-Drift-Issue31621
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second thoughts, can you make this MANAGED
and UNMANAGED
thing an enum? That way we don't have to specify this with raw strings anymore. The enum should not be exposed to consumers of this construct.
Issue # (if applicable)
Closes #31621.
Reason for this change
When defining the Type property for the
AWS::Batch::ComputeEnvironment
resource via theManagedEc2EcsComputeEnvironment
L2 construct or theCfnComputeEnvironment
construct, if the value isn't set to MANAGED (case sensitive), this will result in a false positive drift result for this resource.Description of changes
Changed Type to
MANAGED
forManagedEc2EcsComputeEnvironment
to avoid false drift in CloudFormation.Per AWS::Batch::ComputeEnvironment, the
Type
property acceptsMANAGED
|UNMANAGED
values. Although lowercase value (e.g.managed
) works when usingManagedEc2EcsComputeEnvironment
L2 construct, it results in false positive drift in CloudFormation console.Description of how you validated changes
IMPORTANT NOTE (for reviewer): This simple case change from
managed
toMANAGED
could cause resource update when deploying viacdk deploy
. Please review if this should be mentioned as breaking change in PR. We are just correcting the casing for the value.Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license