-
Notifications
You must be signed in to change notification settings - Fork 4k
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): computeEnvironmentName is not set in FargateComputeEnvironment #25805
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 - Existing testcases are sufficient to test this small change in parameter. |
Clarification Request - Existing testcases are sufficient for the new changes. |
It looks like you've gone ahead and tried to work on tests since this, so thanks 🙂 You'll need to run the integration tests and update their snapshots, currently only the file has been adjusted, we need to verify that the integration tests still deploy. |
Hey @peterwoodworth, I am not able to run integration tests on my end. Is there any way we can skip the testcases since it is not required in this scenario? |
We should be testing that every property gets synthesized and that deployment works - since we aren't currently doing that, test cases are required.
What's preventing you from running integration tests? |
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.
thanks for the fix, this is almost ready to merge. We definitely need changes to the integration test snapshots here, because you've (correctly) changed the integ test file. Just remove the taggedFargate
resource and I can update the snapshots for you.
const taggedFargate = new FargateComputeEnvironment(stack, 'taggedFargate', { | ||
vpc, | ||
computeEnvironmentName: 'taggedFargateCE', | ||
}); | ||
|
||
Tags.of(taggedFargate).add('computeEnvironmentName', 'bar') | ||
|
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.
you don't need to add this here, you already added it to the CE above.
Hey @comcalvi, Thank you so much for the help. Please update the snapshots. |
sigh...I broke it. Sorry about this. Please do not pull from the remote. I ran Can you ensure maintainers are allowed to push to your fork? This also shouldn't be closed...not sure why it was closed |
The pull request linter fails with the following errors:
PRs must pass status checks before we can provide a meaningful review. If you would like to request an exemption from the status checks or clarification on feedback, please leave a comment on this PR containing |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
@comcalvi 😓 I am also not sure why is it closed. I guess they have merged the changes from some other PR 😞 |
Can you push some code to branch of this PR @akshaypilankar, so I can reopen it? 🙂 So don't pull from the branch, instead do a force push to your |
computeEnvironmentName property was missing in FargateComputeEnvironment due to which ComputeEnvironmentName property set on the resulting AWS::Batch::ComputeEnvironment resource in the outputted CloudFormation.
Updated managed-compute-environment to reflect the fix made in FargateComputeEnvironment.
Closes #25794.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license