feat(ecs): add external network modes to ExternalTaskDefinition and TaskDefinition #17762
Conversation
…xternal compatibility
|
|
|
Title does not follow the guidelines of Conventional Commits. Please adjust title before merge. |
beezly
changed the title
github-actions
bot
added
the
@aws-cdk/aws-ecs
|
Hey @neilkuan @clarencetw @dwchiang @kimisme9386 can you take a first look? |
packages/@aws-cdk/aws-ecs/test/external/external-task-definition.test.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-ecs/test/external/external-task-definition.test.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-ecs/test/external/external-task-definition.test.ts
Outdated
Show resolved
Hide resolved
|
@pahud is anything else needed for this - I also have a branch building against the v2 code. https://github.com/beezly/aws-cdk/tree/v2_network_modes |
There was a problem hiding this comment.
Thanks for opening this PR, @beezly! Just a few minor comments, and then it is ready to go.
| * | ||
| * With ECS Anywhere, supported modes are bridge, host and none. | ||
| * | ||
| * @default - NetworkMode.Bridge |
There was a problem hiding this comment.
| * @default - NetworkMode.Bridge | |
| * @default NetworkMode.BRIDGE |
| /** | ||
| * The networking mode to use for the containers in the task. | ||
| */ | ||
| public readonly networkMode: NetworkMode; | ||
|
|
There was a problem hiding this comment.
This public property is not used anywhere, so let's not add it until there is a requirement for it.
There was a problem hiding this comment.
Actually this already exists in the base class, TaskDefinition. It should still be removed from here.
| }); | ||
|
|
||
| this.networkMode = props.networkMode ?? NetworkMode.BRIDGE; |
There was a problem hiding this comment.
| this.networkMode = props.networkMode ?? NetworkMode.BRIDGE; |
There was a problem hiding this comment.
This is already done by passing in props.networkMode ?? NetworkMode.BRIDGE to the super constructor.
| @@ -49,7 +50,7 @@ describe('external task definition', () => { | |||
| ], | |||
| }, | |||
| Family: 'ecs-tasks', | |||
| NetworkMode: ecs.NetworkMode.BRIDGE, | |||
| NetworkMode: ecs.NetworkMode.HOST, | |||
There was a problem hiding this comment.
Let's test on the actual string value of the enum, so if someone changes the value of NetworkMode.HOST in the future, this test will catch it.
| NetworkMode: ecs.NetworkMode.HOST, | |
| NetworkMode: 'host', |
mergify
bot
dismissed
madeline-k’s stale review
Pull request has been modified.
|
@madeline-k looks like this didn't merge but I'm not sure why. Are you able to give me a hint? |
TheRealAmazonKendra
added
the
pr-linter/exempt-integ-test
TheRealAmazonKendra
changed the title
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
…askDefinition (aws#17762) Adds support for Host and None network modes when creating Task Definitions with External compatibility. See https://docs.aws.amazon.com/AmazonECS/latest/developerguide/ecs-anywhere.html ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Adds support for Host and None network modes when creating Task Definitions with External compatibility.
See https://docs.aws.amazon.com/AmazonECS/latest/developerguide/ecs-anywhere.html
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license