You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "dongjoon-hyun (via GitHub)" <gi...@apache.org> on 2026/09/14 19:58:52 UTC
[PR] [SPARK-59504] Use `enabled` key convention in Helm `values.yaml` while honoring legacy `enable` [spark-kubernetes-operator]
dongjoon-hyun opened a new pull request, #825:
URL: https://github.com/apache/spark-kubernetes-operator/pull/825
### What changes were proposed in this pull request?
This PR aims to use the `enabled` key convention in the Helm chart `values.yaml` while still honoring the legacy `enable` keys.
- `operatorDeployment.networkPolicy.enable` → `operatorDeployment.networkPolicy.enabled`
- `operatorConfiguration.dynamicConfig.enable` → `operatorConfiguration.dynamicConfig.enabled`
Two helpers in `_helpers.tpl` resolve each toggle to `true` when either the new or the legacy key is `true`, and all templates use them. The legacy keys stay valid in `values.schema.json`, marked as deprecated, and will be removed in chart `2.0.0`.
### Why are the changes needed?
`<feature>.enabled` is the common Helm convention, and the operator's runtime property is already `spark.kubernetes.operator.dynamicConfig.enabled`. Resolving with `or` keeps existing `enable: true` users working because the chart default `enabled: false` is always merged and cannot be distinguished from a user-supplied value.
### Does this PR introduce _any_ user-facing change?
Yes, compared to [v1.0.0 (2026-07-23)](https://github.com/apache/spark-kubernetes-operator/releases/tag/1.0.0). New users should use `enabled`. Existing values files with `enable` keep working without modification.
### How was this patch tested?
- `helm lint --strict` passes.
- Rendered with `helm template` for default, `enabled=true`, legacy `enable=true`, and legacy `enable=false` and confirmed the expected resources and properties.
- Added a CI step in the `lint` job that renders the chart with the legacy `enable` keys and asserts they are still honored.
- Existing Helm and E2E tests cover the new `enabled` keys.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Fable 5.1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59504] Use `enabled` key convention in Helm `values.yaml` while honoring legacy `enable` [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #825:
URL: https://github.com/apache/spark-kubernetes-operator/pull/825#issuecomment-5681875007
Thank you for the review, @peter-toth. Addressed in 8859867.
- **1.** Kept the `or` resolution to preserve the documented `enabled: false` default and the `required` schema entry, and documented the precedence in `docs/operations.md`: a stale `enable: true` wins over `enabled: false` and must be removed to turn the feature off. This is stated both in the `dynamicConfig.enable` table row and in the NetworkPolicy section.
- **2.** Added a paragraph to the NetworkPolicy section of `docs/operations.md` noting that `operatorDeployment.networkPolicy.enable` is deprecated but still honored.
- **3.** Agreed that this is pre-existing and out of scope here. I'll file a separate JIRA for `operatorRbac.configManagement.create`.
- **4.** Thanks for verifying the presence-check variant. I'll stay with `or` for this PR for the reasons in (1).
- **5.** Filed SPARK-59533 to track the removal in chart `2.0.0` and linked it from the `values.yaml` comments, the `values.schema.json` descriptions, and `docs/operations.md`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59504] Use `enabled` key convention in Helm `values.yaml` while honoring legacy `enable` [spark-kubernetes-operator]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #825:
URL: https://github.com/apache/spark-kubernetes-operator/pull/825#discussion_r4016813882
##########
build-tools/helm/spark-kubernetes-operator/values.yaml:
##########
@@ -94,7 +94,9 @@ operatorDeployment:
# operator pod is denied. Note that this requires a CNI plugin with NetworkPolicy support,
# and that egress traffic is not restricted.
networkPolicy:
- enable: false
+ # The legacy key `enable` is deprecated and will be removed in chart 2.0.0 (SPARK-59533).
Review Comment:
**Finding 7.** The deprecation is announced in four files, but `helm install` / `helm upgrade` prints nothing for a values file that still says `enable: true`. Docs only reach people who go looking. When SPARK-59533 lands, that user's NetworkPolicy quietly stops being created, and a lost ingress restriction is not something you want to discover later.
Now that `enable` is out of the chart defaults, `hasKey` is an exact presence signal. I verified it on this head: `hasKey .Values.operatorDeployment.networkPolicy "enable"` renders `false` on defaults and `true` under `--set operatorDeployment.networkPolicy.enable=false`, and the same holds for `operatorConfiguration.dynamicConfig`. So a `templates/NOTES.txt` covers both toggles:
```gotemplate
{{- if hasKey .Values.operatorDeployment.networkPolicy "enable" }}
WARNING: `operatorDeployment.networkPolicy.enable` is deprecated, use `enabled` instead.
It is still honored and will be removed in chart 2.0.0 (SPARK-59533).
{{- end }}
```
The chart has no `NOTES.txt` today and `.github/.licenserc.yaml` does not exempt one, so it needs the ASF header. Putting the header inside a `{{/* ... */}}` comment keeps it out of the install output while leaving the text in the file for `skywalking-eyes`. `helm lint --strict` passes with that file added.
A follow-up PR is fine if you would rather keep this one to the rename.
##########
.github/workflows/build_and_test.yml:
##########
@@ -304,3 +304,11 @@ jobs:
- name: Validate helm chart linting
run: |
helm lint --strict build-tools/helm/spark-kubernetes-operator
+ - name: Validate deprecated helm values are still honored
+ run: |
+ helm template spark build-tools/helm/spark-kubernetes-operator \
+ --set operatorDeployment.networkPolicy.enable=true \
+ | grep -q 'kind: NetworkPolicy'
+ helm template spark build-tools/helm/spark-kubernetes-operator \
+ --set operatorConfiguration.dynamicConfig.enable=true \
+ | grep -q 'spark.kubernetes.operator.dynamicConfig.enabled=true'
Review Comment:
**Finding 6.** This step pins the ON direction for both legacy keys. Nothing pins the OFF direction, here or in the Helm Tests job — the `network-policy` group installs with `enabled: true`, and no test asserts the NetworkPolicy is absent.
That gap matters because the helpers return the *strings* `"true"` / `"false"`, so every callsite has to spell out `eq (include "...") "true"`. A future gated resource written as `{{- if include "spark-operator.networkPolicy.enabled" . }}` renders unconditionally, since `"false"` is a non-empty string and therefore truthy. I checked that on this head with a probe template: the naive `if` takes the truthy branch on default values.
The four callsites today all get it right. But with the helper mutated to `{{- if or $np.enabled $np.enable }}true{{ else }}true{{ end -}}` — a stuck-on toggle — `helm lint --strict` and both of the new assertions still pass. These two fail on that mutation and pass on this head:
```suggestion
| grep -q 'spark.kubernetes.operator.dynamicConfig.enabled=true'
- name: Validate helm values resolve to disabled
run: |
if helm template spark build-tools/helm/spark-kubernetes-operator \
| grep -q 'kind: NetworkPolicy'; then
echo "NetworkPolicy rendered with the default values"; exit 1
fi
if helm template spark build-tools/helm/spark-kubernetes-operator \
--set operatorDeployment.networkPolicy.enable=false \
| grep -q 'kind: NetworkPolicy'; then
echo "NetworkPolicy rendered with enable=false"; exit 1
fi
```
Worth using `if ... then exit 1; fi` rather than `! ... | grep -q`. The step has no `shell:` key, so it runs under `bash -e` without `pipefail`, and `bash` exempts a `!`-inverted command from `-e`. I confirmed that `! helm template ... | grep -q 'kind: NetworkPolicy'` exits 0 and the script keeps going even when the NetworkPolicy *is* rendered.
##########
docs/operations.md:
##########
@@ -154,13 +155,20 @@ for the operator pod:
```yaml
operatorDeployment:
networkPolicy:
- enable: true
+ enabled: true
metricsIngress:
- namespaceSelector:
matchLabels:
kubernetes.io/metadata.name: "monitoring"
```
+The legacy key `operatorDeployment.networkPolicy.enable` is deprecated in favor of `enabled`
Review Comment:
**Finding 8.** The content is right, the position splits the section. This paragraph lands between the `enabled: true` example and the list that explains it, so "When enabled, all ingress traffic to the operator pod is denied except:" now reads as a continuation of the deprecation note rather than of the example above it.
Moving the paragraph to the end of the section, after the CNI note at `docs/operations.md:181-183`, keeps the example next to its explanation and still puts the legacy key in front of anyone reading the section.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59504] Use `enabled` key convention in Helm `values.yaml` while honoring legacy `enable` [spark-kubernetes-operator]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on PR #825:
URL: https://github.com/apache/spark-kubernetes-operator/pull/825#issuecomment-5682742275
Yeah, this the current behaviour of my general review skill to offer opening follow-ups to the PR author, not specific to this repo at all. Let me try to fine tune it to be more pro-active next time. Thanks for the feedback.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59504] Use `enabled` key convention in Helm `values.yaml` while honoring legacy `enable` [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #825:
URL: https://github.com/apache/spark-kubernetes-operator/pull/825#issuecomment-5682581855
Thank you always, @peter-toth.
BTW, as you know, #44 introduced the bug you found, F3. And, you are able to file a JIRA issue for your finding instead of asking me .
- #44
<img width="768" height="252" alt="Screenshot 2026-09-15 at 07 57 37" src="https://github.com/user-attachments/assets/3029b3c7-fd13-4bc4-b088-837367dc7108" />
-----
This repository is owned by the Apache Spark community including you. So, it's a little weird for me because you tried to enforce me again and again to do something independent from this context. Do you want to avoid creating a JIRA issue for this repository, @peter-toth ?
<img width="812" height="171" alt="Screenshot 2026-09-15 at 07 58 08" src="https://github.com/user-attachments/assets/ee2f7a38-62aa-475f-b96e-2344059042fe" />
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59504] Use `enabled` key convention in Helm `values.yaml` while honoring legacy `enable` [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #825:
URL: https://github.com/apache/spark-kubernetes-operator/pull/825#issuecomment-5683117512
Thank you always for helping this, @peter-toth !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59504] Use `enabled` key convention in Helm `values.yaml` while honoring legacy `enable` [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun closed pull request #825: [SPARK-59504] Use `enabled` key convention in Helm `values.yaml` while honoring legacy `enable`
URL: https://github.com/apache/spark-kubernetes-operator/pull/825
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59504] Use `enabled` key convention in Helm `values.yaml` while honoring legacy `enable` [spark-kubernetes-operator]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #825:
URL: https://github.com/apache/spark-kubernetes-operator/pull/825#discussion_r4013109468
##########
build-tools/helm/spark-kubernetes-operator/templates/_helpers.tpl:
##########
@@ -106,14 +106,34 @@ List of Spark workload namespaces. If not provied in values, use the same namesp
{{- end }}
{{- end }}
+{{/*
+Whether the operator pod NetworkPolicy is enabled. The legacy key
+{operatorDeployment.networkPolicy.enable} is deprecated but still honored: the feature is
+enabled when either key is true.
+*/}}
+{{- define "spark-operator.networkPolicy.enabled" -}}
+{{- $np := .Values.operatorDeployment.networkPolicy -}}
+{{- if or $np.enabled $np.enable }}true{{ else }}false{{ end -}}
Review Comment:
**Finding 1.** `or` makes the legacy key a one-way latch. A user whose base values file still carries `enable: true` cannot turn the feature off through the new key:
```
$ helm template spark build-tools/helm/spark-kubernetes-operator \
--set operatorDeployment.networkPolicy.enable=true \
--set operatorDeployment.networkPolicy.enabled=false | grep -c 'kind: NetworkPolicy'
1
$ helm template spark build-tools/helm/spark-kubernetes-operator \
--set operatorConfiguration.dynamicConfig.enable=true \
--set operatorConfiguration.dynamicConfig.enabled=false | grep 'dynamicConfig.enabled='
spark.kubernetes.operator.dynamicConfig.enabled=true
```
That is the layered-values shape people actually use, a checked-in base values file plus `--set` overrides in a pipeline. The override silently does nothing, and no output names the key that won.
The rule is stated in `values.yaml`, so this is documented behaviour rather than a bug. It is not in `docs/operations.md` though, which is where someone debugging a stuck toggle will look. At minimum add a sentence there for both toggles saying the legacy key wins while it is `true`. Finding 4 sketches a shape that removes the asymmetry instead.
##########
build-tools/helm/spark-kubernetes-operator/values.yaml:
##########
@@ -94,7 +94,9 @@ operatorDeployment:
# operator pod is denied. Note that this requires a CNI plugin with NetworkPolicy support,
# and that egress traffic is not restricted.
networkPolicy:
- enable: false
+ # The legacy key `enable` is deprecated and will be removed in chart 2.0.0. It is still
Review Comment:
**Finding 5.** Chart `1.8.0` shipped with operator `1.0.0` (`git show 1.0.0:build-tools/helm/spark-kubernetes-operator/Chart.yaml`) and the chart is now at `1.9.0-dev`, so the chart minor tracks the operator minor. On that cadence `2.0.0` is roughly ten releases out, and nothing tracks the removal. A follow-up JIRA linked from these comments would keep it from being forgotten.
##########
docs/operations.md:
##########
@@ -154,7 +155,7 @@ for the operator pod:
```yaml
operatorDeployment:
networkPolicy:
- enable: true
+ enabled: true
Review Comment:
**Finding 2.** The deprecation of `operatorDeployment.networkPolicy.enable` is documented in `values.yaml` and `values.schema.json`, but nowhere under `docs/`. The values table above has no `operatorDeployment.networkPolicy.*` rows at all, so unlike `dynamicConfig.enable` (which this PR does add a row for, at line 137) there is nothing telling a 1.0.0 user that their `enable: true` still works.
One sentence after this block covers it:
```markdown
The legacy key `operatorDeployment.networkPolicy.enable` is deprecated in favour of
`enabled` and will be removed in chart `2.0.0`. It is still honored: the NetworkPolicy
is created when either key is `true`.
```
##########
build-tools/helm/spark-kubernetes-operator/templates/operator-rbac.yaml:
##########
@@ -173,7 +173,7 @@ metadata:
{{- template "spark-operator.operatorRbacRules" $ }}
---
{{- end }}
-{{- if and .Values.operatorConfiguration.dynamicConfig.enable (eq .Values.operatorConfiguration.dynamicConfig.source "configMap") }}
+{{- if and (eq (include "spark-operator.dynamicConfig.enabled" .) "true") (eq .Values.operatorConfiguration.dynamicConfig.source "configMap") }}
Review Comment:
**Finding 3.** Pre-existing, and a follow-up rather than something for this PR. `operatorRbac.configManagement.create` is documented at `docs/operations.md:108` as a toggle defaulting to `true`, but `grep -rn configManagement` finds it only in `values.yaml`, `values.schema.json`, and the `roleName` / `roleBindingName` references at lines 180, 200 and 206. No template reads `create`, so setting it to `false` does not suppress the Role and RoleBinding gated on this line.
The doc entry also credits it with covering leader election. That is actually handled by the `coordination.k8s.io/leases` rule the ClusterRole adds when `replicas > 1` (lines 111-115 of this file).
Since this PR is auditing `values.yaml` toggles, worth a separate JIRA to either wire `create` in or drop it.
##########
build-tools/helm/spark-kubernetes-operator/templates/_helpers.tpl:
##########
@@ -106,14 +106,34 @@ List of Spark workload namespaces. If not provied in values, use the same namesp
{{- end }}
{{- end }}
+{{/*
+Whether the operator pod NetworkPolicy is enabled. The legacy key
+{operatorDeployment.networkPolicy.enable} is deprecated but still honored: the feature is
+enabled when either key is true.
+*/}}
+{{- define "spark-operator.networkPolicy.enabled" -}}
Review Comment:
**Finding 4.** An alternative that keeps the legacy key working and still lets `enabled` win when it is set explicitly. Helm strips null values during coalescing, so defaulting `enabled` to null in `values.yaml` makes `.Values...enabled` absent unless the user sets it. That is the presence signal the `or` shape lacks.
```gotemplate
{{- define "spark-operator.networkPolicy.enabled" -}}
{{- $np := .Values.operatorDeployment.networkPolicy -}}
{{- if not (kindIs "invalid" $np.enabled) }}{{ $np.enabled }}
{{- else if not (kindIs "invalid" $np.enable) }}{{ $np.enable }}
{{- else }}false{{ end -}}
{{- end }}
```
with `values.yaml`:
```yaml
networkPolicy:
# Default: false. The legacy key `enable` is deprecated and will be removed in chart
# 2.0.0. It is honored when `enabled` is not set.
enabled:
```
I rendered this variant on all six key combinations, and `helm lint --strict` passes:
| values | `or` (this PR) | presence check |
|---|---|---|
| defaults | off | off |
| `enabled=true` | on | on |
| `enable=true` | on | on |
| `enable=false` | off | off |
| `enable=true`, `enabled=false` | **on** | off |
| `enable=false`, `enabled=true` | on | on |
The cost is that `enabled` has to leave `networkPolicy.required` in `values.schema.json`, because Helm drops the null key before schema validation runs. Without that, `helm lint --strict` fails with `at '/operatorDeployment/networkPolicy': missing property 'enabled'`.
Honest counter-argument: `enabled:` reading as null in `values.yaml` is less obvious than `enabled: false`, and dropping the `required` entry weakens validation for the length of the deprecation window. If you would rather keep the documented default, `or` is the right call and finding 1's doc sentence is enough on its own.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59504] Use `enabled` key convention in Helm `values.yaml` while honoring legacy `enable` [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #825:
URL: https://github.com/apache/spark-kubernetes-operator/pull/825#issuecomment-5683133368
Merged to main
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59504] Use `enabled` key convention in Helm `values.yaml` while honoring legacy `enable` [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #825:
URL: https://github.com/apache/spark-kubernetes-operator/pull/825#issuecomment-5682427592
Thank you for the second pass, @peter-toth. Addressed in 4541e30.
- **3.** Filed SPARK-59537 for `operatorRbac.configManagement.create`.
- **6.** Added a `Validate helm values resolve to disabled` step next to the ON-direction one. It asserts the NetworkPolicy is absent on the default values and on `networkPolicy.enable=false`, and that `dynamicConfig.enabled=true` is not emitted on `dynamicConfig.enable=false`, using `if ... then exit 1` as you suggested. I confirmed it fails on the stuck-on helper mutation and passes on this head.
- **7.** Agreed on the value of an install-time signal. I'd like to keep this PR to the rename and handle the `NOTES.txt` warning in a follow-up PR, together with SPARK-59533.
- **8.** Moved the deprecation paragraph to the end of the NetworkPolicy section, after the CNI note.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org