-
Notifications
You must be signed in to change notification settings - Fork 1.6k
🐛 (helm/v2-alpha): Support custom container names and fix templating bugs #5463
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
🐛 (helm/v2-alpha): Support custom container names and fix templating bugs #5463
Conversation
|
Hi @AlirezaPourchali. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
… bugs This PR adds support for custom container names via kubectl.kubernetes.io/default-container annotation and fixes two critical bugs found during testing: 1. Container names were being incorrectly templated by substituteResourceNamesWithPrefix. Container names are internal pod identifiers and should remain literal, not templated like cluster-wide resource names. 2. Image templating failed due to order-of-operations issue where substituteResourceNamesWithPrefix runs before templateImageReference. Modified all image-related functions to check for both literal and templated container names. Testing: Verified with custom container names (controller-test) and ensures backward compatibility with default 'manager' container name.
0ce6f96 to
d9b67c1
Compare
camilamacedo86
left a comment
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.
/lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AlirezaPourchali, camilamacedo86 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/ok-to-test |
Fixes #5449
This PR adds support for custom container names via
kubectl.kubernetes.io/default-containerannotation and fixes two critical bugs found during testing.Background
PR #5452 attempted to add this feature but has implementation issues that prevent it from working correctly. This PR provides a complete working solution.
Changes
New Feature:
getDefaultContainerName()function to readkubectl.kubernetes.io/default-containerannotation"manager"for backward compatibility when annotation is missingBug Fix 1: Container names were being incorrectly templated
substituteResourceNamesWithPrefix()was converting container names to Helm templates like{{ include "chart.resourceName" ... }}osiris-manager,controller-test) match the same regex patternproject-prefix-*intended for resource names likeproject-webhook-servicecontainers:) and excluding them from templatingBug Fix 2: Image templating failed with custom container names
substituteResourceNamesWithPrefixruns beforetemplateImageReferencetemplateImageReference,templateEnvironmentVariables,templateResources,templateContainerSecurityContext,templateControllerManagerArgs) to check for both literal and templated container namesTesting
controller-testmanagercontainer nameExample
Projects can now use any container name: