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.

Sign up for GitHub

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

Jump to bottom

feat(ecs-patterns): add containerName to QueueProcessingEc2Service #11093

Merged
merged 8 commits into from
Dec 23, 2020
Merged

feat(ecs-patterns): add containerName to QueueProcessingEc2Service #11093

merged 8 commits into from
Dec 23, 2020

Conversation

iRoachie
Copy link
Contributor

resolves #10517

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@gitpod-io
Copy link

gitpod-io bot commented Oct 25, 2020 •
edited
Loading

@SomayaB SomayaB added the @aws-cdk/aws-ecs-patterns Related to ecs-patterns library label Oct 26, 2020
@iRoachie
Copy link
Contributor Author

@uttarasridhar @MrArnoldPalmer @SomayaB All good here?

@MrArnoldPalmer
Copy link
Contributor

ApplicationLoadBalancedEc2Service has a nested taskImageOptions prop to configure properties on the container definition that the pattern creates. Unfortunately we didn't follow that pattern for this construct so we have to handle this at the top level. We should add this to QueueProcessingFargateService as well to keep behavior consistent.

Long term however the container name and construct IDs need to be separated. Naming containers should not be done through the construct's ID parameter. This is a hole in the underlying ContainerDefinition construct that doesn't allow passing a name property to the underlying CFN resource separate from the ID.

MrArnoldPalmer
MrArnoldPalmer previously requested changes Nov 2, 2020
View reviewed changes
Copy link
Contributor

@MrArnoldPalmer MrArnoldPalmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets add to QueueProcessingFargateService as well.

@mergify mergify bot dismissed MrArnoldPalmer’s stale review November 3, 2020 14:03

Pull request has been modified.

@iRoachie
Copy link
Contributor Author

iRoachie commented Nov 3, 2020

Added to QueueProcessingFargateService @MrArnoldPalmer

@SoManyHs SoManyHs mentioned this pull request Nov 10, 2020
Closed
SoManyHs
SoManyHs previously approved these changes Dec 22, 2020
View reviewed changes
Copy link
Contributor

@SoManyHs SoManyHs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for contributing! 🎉

@mergify mergify bot dismissed SoManyHs’s stale review December 22, 2020 17:50

Pull request has been modified.

@SoManyHs
Copy link
Contributor

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Dec 22, 2020

Command update: success

Branch already up to date

@SoManyHs SoManyHs changed the title feat(ecs-patterns): containerName for QueueProcessingEc2Service feat(ecs-patterns): add containerName to QueueProcessingEc2Service Dec 22, 2020
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 1d0148a
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@SoManyHs SoManyHs merged commit 52a80bb into aws:master Dec 23, 2020
@iRoachie iRoachie deleted the container-name branch December 23, 2020 00:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SoManyHs SoManyHs SoManyHs approved these changes

@MrArnoldPalmer MrArnoldPalmer Awaiting requested review from MrArnoldPalmer

Assignees

@uttarasridhar uttarasridhar

@MrArnoldPalmer MrArnoldPalmer

Labels
@aws-cdk/aws-ecs-patterns Related to ecs-patterns library
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[ecs-patterns] Add container name to QueueProcessingEc2Service
6 participants
@iRoachie @MrArnoldPalmer @SoManyHs @aws-cdk-automation @uttarasridhar @SomayaB