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-ecs): Expose environment on containerDefinition #17867

Closed
1 of 2 tasks
biffgaut opened this issue Dec 6, 2021 · 2 comments · Fixed by #17889
Closed
1 of 2 tasks

(aws-ecs): Expose environment on containerDefinition #17867

biffgaut opened this issue Dec 6, 2021 · 2 comments · Fixed by #17889
Assignees
@madeline-k
Labels
@aws-cdk/aws-ecs Related to Amazon Elastic Container feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged.

Comments

@biffgaut
Copy link
Contributor

biffgaut commented Dec 6, 2021

Description

There doesn't seem to be a way to update the environment variables on an already instantiated container in a TaskDefinition. Exposing environment on ContainerDefinition would allow clients to add more environment variables to an already instantiated TaskDefinition/Container

Use Case

My use case is create the taskdef/container, then programmatically choose to give that container access to a dynamically chosen set of queues, buckets, topics, etc. As these are dynamically named, I use environment variables to identify the resources to the code within my container. 'addPropertyOverride' allows me to set the variables once, but it deletes the current contents rather than allowing me to add more.

Proposed Solution

From looking at the code in container-definition.ts, it seems that this would require:

  • Creating a public property on ContainerDefinition
  • Assigning the environment from the props to the public property in the constructor
  • Changing the rendering to use the property rather than the private props property
  • Updating tests

It seems too simple to have not been done before - is there a reason things are the way they are that I don't see?

Other information

No response

Acknowledge

  • I may be able to implement this feature request
  • This feature might incur a breaking change
@biffgaut biffgaut added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Dec 6, 2021
@github-actions github-actions bot added the @aws-cdk/aws-ecs Related to Amazon Elastic Container label Dec 6, 2021
@biffgaut
Copy link
Contributor Author

biffgaut commented Dec 6, 2021

FWIW - I'm on the AWS Solutions Constructs team and we need this to support a new batch of Constructs we're developing that support Fargate containers.

@biffgaut biffgaut mentioned this issue Dec 7, 2021
Merged
@mergify mergify bot closed this as completed in #17889 Dec 13, 2021
mergify bot pushed a commit that referenced this issue Dec 13, 2021
@biffgaut
feat(aws-ecs): expose environment from containerDefinition (#17889)
4937cd0 
Closes #17867

* Assigned props.environment to a public readonly member
* Added integration test that confirms the environment can be appended after the task is instantiated

Made 2 cosmetic, but no obvious changes. Environment values are specified:

name: value
name2: value

But in the test and the README.md files the sample values were:

name: something
value: something else

This is using the string 'value" as a key - which, as someone reading the code for the first time, was confusing. So I changed the sample values to more clearly display what's a key and what's a value.

----

*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.

TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this issue Feb 21, 2022
@biffgaut @TikiTDO
feat(aws-ecs): expose environment from containerDefinition (aws#17889)
a1b4737 
Closes aws#17867

* Assigned props.environment to a public readonly member
* Added integration test that confirms the environment can be appended after the task is instantiated

Made 2 cosmetic, but no obvious changes. Environment values are specified:

name: value
name2: value

But in the test and the README.md files the sample values were:

name: something
value: something else

This is using the string 'value" as a key - which, as someone reading the code for the first time, was confusing. So I changed the sample values to more clearly display what's a key and what's a value.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@madeline-k madeline-k

Labels
@aws-cdk/aws-ecs Related to Amazon Elastic Container feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat(aws-ecs): expose environment from containerDefinition
2 participants
@biffgaut @madeline-k