Fix Helm Chart resource lookup key handling for objects in default namespace #2655
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Does the PR have any schema changes?Looking good! No breaking changes found. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2655 +/- ##
==========================================
+ Coverage 18.41% 18.62% +0.20%
==========================================
Files 47 47
Lines 9589 9589
==========================================
+ Hits 1766 1786 +20
+ Misses 7724 7699 -25
- Partials 99 104 +5 ☔ View full report in Codecov by Sentry. |
a479487 to
d994349
Compare
d994349 to
4229cf4
Compare
rquitales
commented
Dec 12, 2023
| // to searching for the resource by namespace and name. This occurs when the Helm template manifest explicitly | ||
| // sets the namespace field on the resource. | ||
| if namespace == "default" || namespace == "" { | ||
| return resources[fmt.Sprintf("%s::default/%s", gvk, name)] |
There was a problem hiding this comment.
The fallback logic to also explicitly specify default namespace.
rquitales
commented
Dec 12, 2023
| kind: Deployment | ||
| metadata: | ||
| name: {{ include "local-test-chart.fullname" . }}-new | ||
| namespace: {{ .Release.Namespace }} # This is what causes the difference when needing to specify a namespace or not during templating. |
There was a problem hiding this comment.
This is the helm template format that requires explicit specification of default in our resource lookup key.
blampe
reviewed
Dec 12, 2023
blampe
approved these changes
Dec 12, 2023
rquitales
added a commit
that referenced
this pull request
Dec 15, 2023
…mespace (#2655) This pull request modifies the `GetResource` Go method for the Helm Chart resource. Previously, when a resource was deployed to the default namespace, the `default` namespace was always omitted from the resource lookup key. However, in cases where the namespace is explicitly defined in the Helm chart template, the `default` namespace was inadvertently excluded from the resource lookup key despite it being needed. This behavior in Helm is documented in the upstream issue: helm/helm#3553. **Changes Made:** - Implemented a fallback mechanism to include the `default` namespace in the resource lookup key when necessary for the `GetResource` method. **Test Added:** - Added a test (`ChartGetResource`) to verify the `GetResource` method for both types of Helm charts—those with the explicitly defined default namespace and those without. **Verification:** - Verified that the modified `GetResource` method successfully handles scenarios where the namespace is explicitly defined in the Helm chart template. - Confirmed that the added test covers both types of Helm charts and fails when the fallback logic is reverted. Fixes: #2638
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed changes
This pull request modifies the
GetResourceGo method for the Helm Chart resource. Previously, when a resource was deployed to the default namespace, thedefaultnamespace was always omitted from the resource lookup key. However, in cases where the namespace is explicitly defined in the Helm chart template, thedefaultnamespace was inadvertently excluded from the resource lookup key despite it being needed. This behavior in Helm is documented in the upstream issue: helm/helm#3553.Changes Made:
defaultnamespace in the resource lookup key when necessary for theGetResourcemethod.Test Added:
ChartGetResource) to verify theGetResourcemethod for both types of Helm charts—those with the explicitly defined default namespace and those without.Verification:
GetResourcemethod successfully handles scenarios where the namespace is explicitly defined in the Helm chart template.Related issues (optional)
Fixes: #2638