-
Notifications
You must be signed in to change notification settings - Fork 639
Introduce ECS_PAUSE_LABELS env var to apply labels to pause container #4789
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
base: dev
Are you sure you want to change the base?
Conversation
47d0aba to
282864b
Compare
|
Thank you @shogondo for your contribution with this pull request! I have made some updates to this pull request to address the comments I raised and will get this pull request reviewed by another member of our team. |
282864b to
660e68b
Compare
| switch container.Type { | ||
| case apicontainer.ContainerCNIPause, apicontainer.ContainerNamespacePause: | ||
| if pauseLabels := os.Getenv(pauseLabelsEnvVar); pauseLabels != "" { | ||
| // Set labels to pause container if it's provieded as env var. |
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.
Nit: typo in provided
| } | ||
|
|
||
| // Parse label string and set them to the given container configuration. | ||
| func setLabelsFromJsonString(config *dockercontainer.Config, labelsString string) { |
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.
Nit: rename function to setLabelsFromJSONString to be consistent with Go conventions
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.
The function is not exported so we can keep it as is
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.
Im referring to Json -> JSON
| return | ||
| } | ||
| if len(labels) > 0 { | ||
| config.Labels = labels |
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.
In the case where config.Labels is already defined, this line will overwrite it.
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.
The containerConfig is defined at L1788, and later we unmarshall and update containerConfig at L1803. Not sure whether a pause container's container.DockerConfig.Config will contain any Labels information but perhaps it's better to be cautious here.
Summary
This change introduces new environment variable
ECS_PAUSE_LABELS. This can be used to apply custom labels to pause containers. Related issue: #4427.Implementation details
Changes:
task.DockerConfigto apply custom labels to pause containers if theECS_PAUSE_LABELSprovided.ECS_PAUSE_LABELScan be specified with JSON format, similar toECS_AGENT_LABELSECS_PAUSE_LABELSisn't provided, does nothing.ECS_PAUSE_LABELSis invalid format, just ignores it.Testing
Tested manually on an EC2 container instance.
Config:
Before:
After:
New tests cover the changes: yes
Description for the changelog
Enhancement: Add new environment variable
ECS_PAUSE_LABELSto apply custom labels to pause containersAdditional Information
Does this PR include breaking model changes? If so, Have you added transformation functions?
Does this PR include the addition of new environment variables in the README?
Licensing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.