Skip to content
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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ashishdhingra
Copy link
Contributor

@ashishdhingra ashishdhingra commented Oct 7, 2024

Issue # (if applicable)

Closes #31621.

Reason for this change

When defining the Type property for the AWS::Batch::ComputeEnvironment resource via the ManagedEc2EcsComputeEnvironment L2 construct or the CfnComputeEnvironment 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 for ManagedEc2EcsComputeEnvironment to avoid false drift in CloudFormation.

Per AWS::Batch::ComputeEnvironment, the Type property accepts MANAGED | UNMANAGED values. Although lowercase value (e.g. managed) works when using ManagedEc2EcsComputeEnvironment L2 construct, it results in false positive drift in CloudFormation console.

Description of how you validated changes

  • Updated Unit tests
  • Updated Integration tests snapshots

IMPORTANT NOTE (for reviewer): This simple case change from managed to MANAGED could cause resource update when deploying via cdk 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

@github-actions github-actions bot added bug This issue is a bug. effort/small Small work item – less than a day of effort p2 labels Oct 7, 2024
@aws-cdk-automation aws-cdk-automation requested a review from a team October 7, 2024 21:31
@ashishdhingra ashishdhingra changed the title User/ashdhin/managed ec2 ecs compute environment drift issue31621 fix(batch): changed Type to MANAGED for ManagedEc2EcsComputeEnvironment to avoid false drift in CloudFormation. Oct 7, 2024
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Oct 7, 2024
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a 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.

@ashishdhingra
Copy link
Contributor Author

Exemption Request

This PR just changes the Type value to MANAGED (per docs). The existing integration test snapshots are updated. No new integration test is required.

@aws-cdk-automation aws-cdk-automation added the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Oct 7, 2024
@pahud
Copy link
Contributor

pahud commented Oct 8, 2024

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

@amazon-codecatalyst amazon-codecatalyst bot force-pushed the user/ashdhin/ManagedEc2EcsComputeEnvironment-Drift-Issue31621 branch from 00d6f87 to 06d5875 Compare October 8, 2024 21:57
@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Oct 8, 2024
Copy link
Contributor

@comcalvi comcalvi left a 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.

@comcalvi comcalvi added pr-linter/exempt-integ-test The PR linter will not require integ test changes and removed pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. labels Oct 9, 2024
@aws-cdk-automation aws-cdk-automation dismissed their stale review October 9, 2024 17:52

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

@aws-cdk-automation aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Oct 9, 2024
@amazon-codecatalyst amazon-codecatalyst bot force-pushed the user/ashdhin/ManagedEc2EcsComputeEnvironment-Drift-Issue31621 branch from 06d5875 to bd81d12 Compare October 9, 2024 21:33
@ashishdhingra ashishdhingra requested a review from comcalvi October 9, 2024 21:34
@ashishdhingra
Copy link
Contributor Author

Can you check if this applies to unmanaged compute environments as well? If it does, we should fix it there too.

@comcalvi Thanks for the feedback. Added commit to correct casing for UnmamagedComputeEnvironment as well. Please review.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 9ffc6bc
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

Copy link
Contributor

@comcalvi comcalvi left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. contribution/core This is a PR that came from AWS. effort/small Small work item – less than a day of effort p2 pr-linter/exempt-integ-test The PR linter will not require integ test changes
Projects
None yet
4 participants