-
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
feat(ecs): support container version consistency #32225
base: main
Are you sure you want to change the base?
Conversation
63311e2
to
f077a3f
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #32225 +/- ##
=======================================
Coverage 81.37% 81.37%
=======================================
Files 222 222
Lines 13693 13693
Branches 2412 2412
=======================================
Hits 11143 11143
Misses 2271 2271
Partials 279 279
Flags with carried forward coverage won't be shown. Click here to find out more.
|
e4d61ca
to
1255057
Compare
1255057
to
23808f7
Compare
@iliapolo I'm not sure why you updated the branch, but it seems that the resulting CodeBuild CI job hanged or something, the GitHub check is stuck in pending. |
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.
Thank you for the contribution.
Some comments.
...ramework-integ/test/aws-ecs/test/base/integ.task-definition-container-version-consistency.ts
Outdated
Show resolved
Hide resolved
719bb27
to
b28e007
Compare
Support the new ECS::TaskDefinition ContainerDefinition VersionConsistency property. This is a simple enabled/disabled flag. Additionally, set a default disabled value if the container image is a CDK asset, for the reasons described in the comments.
b28e007
to
cb2995a
Compare
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.
Thanks!
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.
Left one comment to help me understand.
@@ -45,6 +45,7 @@ export abstract class ContainerImage { | |||
public static fromDockerImageAsset(asset: DockerImageAsset): ContainerImage { | |||
return { | |||
bind(_scope: Construct, containerDefinition: ContainerDefinition): ContainerImageConfig { | |||
containerDefinition._defaultDisableVersionConsistency?.(); |
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.
This seems like a weird usage, can you help me understand this syntax.
Also if it's public
method, we should remove _
.
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 reviewing.
can you help me understand this syntax.
This is optional chaining for invoking functions. I did it because evil code passes arguments to these functions that are in fact not ContainerDefinition
s:
aws-cdk/packages/aws-cdk-lib/aws-batch/lib/ecs-container-definition.ts
Lines 621 to 626 in 7b5f5a5
this.imageConfig = props.image.bind(this, { | |
...this as any, | |
taskDefinition: { | |
obtainExecutionRole: () => this.executionRole, | |
}, | |
}); |
So, _defaultDisableVersionConsistency
can be undefined in spite of what TypeScript indicates.
Also if it's public method, we should remove _.
I see many @internal
public
methods that have leading underscores. I can remove it if you like, but it does not seem unusal.
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 information! This is good to know.
Thanks for getting back to me. If it's @internal
method, it's okay to stay public
for internal constructs usage. However, I see in the unit test files that this method is also exposed to be used in CDK stacks. If we will allow that kind of usage, I think we should remove _
.
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.
I see in the unit test files that this method is also exposed to be used in CDK stacks.
I'm not sure what you mean by "exposed to be used in CDK stacks." I'm not intending normal users to call this. In these tests, I'm just testing the behavior of _defaultDisableVersionConsistency
on behalf of internal callers like those here in container-image.ts
.
Issue # (if applicable)
Closes #32202.
Reason for this change
Support the new ECS::TaskDefinition ContainerDefinition VersionConsistency property. This is a simple enabled/disabled flag.
Description of changes
Add a simple enabled/disabled enum prop to the construct.
More consequentially, default the prop to disabled (instead of unset) if the container image is a CDK asset, for the reasons described in the comments.
Description of how you validated changes
Unit and integration tests.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license