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

@aws-cdk/ecs: Override container name #8044

Closed
1 of 2 tasks
karupanerura opened this issue May 18, 2020 · 10 comments · Fixed by #13829
Closed
1 of 2 tasks

@aws-cdk/ecs: Override container name #8044

karupanerura opened this issue May 18, 2020 · 10 comments · Fixed by #13829
Assignees
@timohirt
Labels
@aws-cdk/aws-ecs Related to Amazon Elastic Container effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. needs-reproduction This issue needs reproduction. p2

Comments

@karupanerura
Copy link
Contributor

Allow to override container name by constructor property.

Use Case

With the current implementation, these will be the same value implicitly.
This is confusing for our team because the naming conventions for the names in our current(exists) configuration and the Construct IDs are different.
So we need to manage the Construct ID and the name separately.

Proposed Solution

Add containerName property to ContainerDefinitionProps and override containerName by this.

this.containerName = props.containerName ?? this.node.id;

https://github.com/aws/aws-cdk/blob/v1.39.0/packages/%40aws-cdk/aws-ecs/lib/container-definition.ts#L380

Other

  • 👋 I may be able to implement this feature request
  • ⚠️ This feature might incur a breaking change

This is a 🚀 Feature Request
@karupanerura karupanerura added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels May 18, 2020
@SomayaB SomayaB added the @aws-cdk/aws-ecs Related to Amazon Elastic Container label May 19, 2020
@piradeepk piradeepk added needs-reproduction This issue needs reproduction. and removed needs-triage This issue or PR still needs to be triaged. labels Jun 1, 2020
@MrArnoldPalmer MrArnoldPalmer added effort/small Small work item – less than a day of effort p2 labels Dec 7, 2020
@timohirt
Copy link
Contributor

Hey folks! 👋

Can I help here to get this done? It doesn't look too hard. I would volunteer to implement it and open a PR. What do you think?

@karupanerura
Copy link
Contributor Author

Great! Unfortunately, I haven't been able to find the time to work on it. So I'm happy if you work on it.

@timohirt timohirt mentioned this issue Mar 27, 2021
Merged
@timohirt
Copy link
Contributor

@karupanerura looking forward to your feedback. I just copy and paste your code in the description.

@karupanerura
Copy link
Contributor Author

Looks good!

@timohirt
Copy link
Contributor

So, what to do next? This is my first contribution to CDK. Do you review / approve the PR, do I need to find someone else, or are there reviews who will pick up the PR eventually?

@karupanerura
Copy link
Contributor Author

please fix it: https://github.com/aws/aws-cdk/runs/2216420393

@timohirt
Copy link
Contributor

timohirt commented Apr 6, 2021

This error is already fixed. I don't think this is a feature and thus changed the title of the PR.

@SoManyHs
Copy link
Contributor

Related: #13681

@SoManyHs SoManyHs mentioned this issue Apr 29, 2021
Closed
2 tasks
@mergify mergify bot closed this as completed in #13829 Apr 29, 2021
mergify bot pushed a commit that referenced this issue Apr 29, 2021
@timohirt
chore(aws-ecs): add container name parameters (#13829)
caa8208 
Adds container name property to ContainerDefinitionProps which allows
to explicitly set container name. If undefined, this default is still
the node id.

Closes #8044 
Closes #13681

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

john-tipper pushed a commit to john-tipper/aws-cdk that referenced this issue May 10, 2021
@timohirt @john-tipper
chore(aws-ecs): add container name parameters (aws#13829)
57e0dc8 
Adds container name property to ContainerDefinitionProps which allows
to explicitly set container name. If undefined, this default is still
the node id.

Closes aws#8044 
Closes aws#13681

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
hollanddd pushed a commit to hollanddd/aws-cdk that referenced this issue Aug 26, 2021
@timohirt @hollanddd
chore(aws-ecs): add container name parameters (aws#13829)
6d68828 
Adds container name property to ContainerDefinitionProps which allows
to explicitly set container name. If undefined, this default is still
the node id.

Closes aws#8044 
Closes aws#13681

----

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

Was this ever merged? Are we able to change the container name using overrides?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@timohirt timohirt

Labels
@aws-cdk/aws-ecs Related to Amazon Elastic Container effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. needs-reproduction This issue needs reproduction. p2
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

chore(aws-ecs): add container name parameters
8 participants
@karupanerura @uttarasridhar @timohirt @MrArnoldPalmer @betancourtl @SomayaB @SoManyHs @piradeepk