fix(api/types): map HelmChartArgs.ReleaseNamespace -> HelmChart.Namespace#6128
fix(api/types): map HelmChartArgs.ReleaseNamespace -> HelmChart.Namespace#6128SAY-5 wants to merge 4 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: SAY-5 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Welcome @SAY-5! |
|
Hi @SAY-5. 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 Regular contributors should join the org to skip this step. 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. |
423cfbb to
1dfeaef
Compare
|
This PR has multiple commits, and the default merge method is: merge. 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. |
|
/ok-to-test |
| // ReleaseNamespace maps onto Namespace in the (non-deprecated) | ||
| // HelmChart type. Forgetting this mapping is why releaseNamespace | ||
| // set under the deprecated helmChartInflationGenerator field is | ||
| // silently ignored, see kubernetes-sigs/kustomize#4593. |
There was a problem hiding this comment.
This comment seems unnecessary.
8ecd3da to
f293690
Compare
|
Good point, dropped that comment. |
…pace SplitHelmParameters goes through makeHelmChartFromHca to translate the deprecated HelmChartArgs into the current HelmChart type. That mapping forgot to copy ReleaseNamespace onto Namespace, so kustomiz- ations that used the deprecated helmChartInflationGenerator field with releaseNamespace silently rendered every resource into the default namespace instead of the requested one. HelmChartArgs is documented as deprecated, but kustomize still supports it, and the rendering bug is visible: users follow the docs for releaseNamespace and get a kustomization that emits the wrong namespace with no warning. The fix is one line (copy ReleaseNamespace onto Namespace in makeHelmChartFromHca) plus a regression test under TestSplitHelmParametersPropagatesReleaseNamespace that pins the mapping so the bug cannot silently regress. Signed-off-by: Sai Asish Y <say.apm35@gmail.com>
Signed-off-by: SAY-5 <say.apm35@gmail.com>
Signed-off-by: Sai Asish Y <say.apm35@gmail.com>
f293690 to
a864bbc
Compare
Fixes #4593.
Problem
Kustomizations using the (deprecated)
helmChartInflationGeneratorfield withreleaseNamespacesilently emit resources into thedefaultnamespace instead of the requested one:Root cause
SplitHelmParameterstranslates everyHelmChartArgs(deprecated) into aHelmChart(current) viamakeHelmChartFromHca. That translator was missing one field:HelmChart.Namespaceis what later gets passed as--namespacetohelm templateinAsHelmArgs, so when it was left empty the chart was rendered againstdefault.Fix
Copy
old.ReleaseNamespaceintoc.Namespace. One-line change; preserves full backward compatibility with any user that was relying on the broken behaviour (they were never getting the namespace they asked for in the first place).Test
Added
TestSplitHelmParametersPropagatesReleaseNamespaceinapi/types/helmchartargs_test.go, which directly asserts the mapping:Signed-off per DCO.