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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
