Repository navigation
[SPARK-59540] Warn about deprecated Helm enable keys via NOTES.txt - #830
peter-toth wants to merge 2 commits into
Conversation
| - name: Validate the deprecated helm key warning | ||
| if: matrix.test-group == 'configmap-metadata' | ||
| run: | | ||
| # `helm template` does not render NOTES.txt, so an install or upgrade against a |
There was a problem hiding this comment.
helm install --dry-run=client renders NOTES.txt without a cluster (verified locally with KUBECONFIG=/dev/null), so a real cluster is not required here. Could we move this check to the lint job, next to Validate deprecated helm values are still honored? That avoids coupling it to the unrelated configmap-metadata group and avoids applying a NetworkPolicy and dynamicConfig RBAC changes to the live release.
- name: Validate the deprecated helm key warning
run: |
notes=$(helm install spark build-tools/helm/spark-kubernetes-operator --dry-run=client \
--set operatorDeployment.networkPolicy.enable=true \
--set operatorConfiguration.dynamicConfig.enable=true)
echo "$notes" | grep -q 'operatorDeployment.networkPolicy.enable is deprecated'
echo "$notes" | grep -q 'operatorConfiguration.dynamicConfig.enable is deprecated'
if helm install spark build-tools/helm/spark-kubernetes-operator --dry-run=client \
| grep -q 'is deprecated'; then
echo "Deprecation warning printed without a legacy key"; exit 1
fiIf so, please also update the PR description's "a real cluster is the only place this is observable".
There was a problem hiding this comment.
Moved to the lint job, description updated. One wrinkle: on helm 3.18.6 that command still fails with Kubernetes cluster unreachable, even with KUBECONFIG=/dev/null. The runner has Helm 4.2.4 so CI is fine, but a local run on helm 3.x will report a false failure.
| limitations under the License. | ||
| */ -}} | ||
| Apache Spark Kubernetes Operator {{ .Chart.AppVersion }} is installed. | ||
| {{- if hasKey .Values.operatorDeployment.networkPolicy "enable" }} |
There was a problem hiding this comment.
nit: hasKey fails on a nil map. With --set operatorDeployment.networkPolicy=null, the install now fails here:
NOTES.txt:18:21 ... wrong type for value; expected map[string]interface {}; got interface {}
The current chart renders fine with the same input because _helpers.tpl uses or $np.enabled $np.enable. It is an unrealistic input, but it could be guarded with hasKey (.Values.operatorDeployment.networkPolicy | default dict) "enable" (and likewise for dynamicConfig).
There was a problem hiding this comment.
Reproduced and fixed with | default dict on both toggles. --set operatorConfiguration.dynamicConfig=null still fails, but identically on main: _helpers.tpl:137 reads .dynamicConfig.source unguarded.
|
Thank you, @peter-toth! |
What changes were proposed in this pull request?
This PR adds
templates/NOTES.txtto the Helm chart. It prints a deprecation warning when a values file still carriesoperatorDeployment.networkPolicy.enableoroperatorConfiguration.dynamicConfig.enable, the legacy keys that SPARK-59504 replaced withenabled.hasKeyis an exact presence signal for these two, because SPARK-59504 removedenablefrom the chart defaults. A key set tofalsestill warns: the deprecation is about the key, not its value.The ASF header sits inside a
{{- /* ... */ -}}comment, soskywalking-eyessees it andhelm installdoes not print it.Why are the changes needed?
The deprecation is announced in
values.yaml,values.schema.jsonanddocs/operations.md, buthelm installandhelm upgradesay nothing. Docs only reach people who go looking. When SPARK-59533 removes the keys in chart2.0.0, a user still onnetworkPolicy.enable: trueloses their NetworkPolicy with no error.Does this PR introduce any user-facing change?
Yes.
helm installandhelm upgradenow print a NOTES section. Users on the currentenabledkeys see one line naming the installed version:Users still on a legacy key additionally get a warning naming the replacement and the removal target:
How was this patch tested?
helm lint --strictpasses with the default values and with each legacy key set.networkPolicy.enable=truedynamicConfig.enable=truefalseenabledkeys setnetworkPolicy=nullValidate the deprecated helm key warningstep in thelintjob, next to the other deprecated-key assertions: renders with both legacy keys and asserts both warnings are printed, then renders with the default values and fails if any warning is printed.helm templateskipsNOTES.txt, so the step useshelm install --dry-run=client, which renders it without reaching a cluster.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Opus 5