gerkElznik commented on code in PR #1126:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/1126#discussion_r3430434439
##########
helm/flink-kubernetes-operator/templates/controller/configmap.yaml:
##########
@@ -25,24 +25,14 @@ metadata:
data:
config.yaml: |+
{{- if .Values.defaultConfiguration.append }}
- {{- $.Files.Get "conf/flink-conf.yaml" | nindent 4 -}}
+ {{- $.Files.Get "conf/config.yaml" | nindent 4 -}}
{{- end }}
-{{- if hasKey (.Values.defaultConfiguration) "config.yaml" }}
- {{- index (.Values.defaultConfiguration) "config.yaml" | nindent 4 -}}
-{{- end }}
-{{- if .Values.watchNamespaces }}
- kubernetes.operator.watched.namespaces: {{ join ","
.Values.watchNamespaces }}
-{{- end }}
-{{- if index .Values "operatorHealth" }}
- kubernetes.operator.health.probe.enabled: true
- kubernetes.operator.health.probe.port: {{ .Values.operatorHealth.port }}
-{{- end }}
- flink-conf.yaml: |+
-{{- if .Values.defaultConfiguration.append }}
- {{- $.Files.Get "conf/flink-conf.yaml" | nindent 4 -}}
-{{- end }}
Review Comment:
Good question — there are three separate edits in that hunk:
1. **The `watchNamespaces` / `operatorHealth` injections aren't actually
dropped.**
The old template emitted *two* config-file keys (`config.yaml` **and**
`flink-conf.yaml`) and appended both blocks to **each**, so they rendered
twice.
Collapsing to a single `config.yaml` key removes the duplicate — one copy
still
renders just below the resolver (lines 37–43); `helm template` confirms
`kubernetes.operator.health.probe.*` and the `watched.namespaces` line
still
emit, once.
2. **The `hasKey "config.yaml"` block is replaced by the `with`-based
resolver
above it.** That's the core of the ticket: `hasKey` keys off presence,
and since
the chart hardcodes a non-empty `defaultConfiguration.flink-conf.yaml`
that a
downstream `-f values.yaml` can't unset, the `config.yaml` branch was
unreachable. `with` keys off value-truthiness (like the existing `log4j`/
`logback` blocks), so the user's `config.yaml` actually wins.
3. **The `flink-conf.yaml:` key is removed so the ConfigMap emits a single,
always-`config.yaml` file** (the only name Flink 2.x reads), mounted
statically.
The one behavioral consequence of (3) I'd like your read on before acting:
runtime
patching of the operator ConfigMap now has to target the `config.yaml` key.
In
particular `e2e-tests/test_dynamic_flink_conf.sh` patches the
`flink-conf.yaml`
key and would no longer be picked up — its twin `test_dynamic_config.sh`
already
covers the same dynamic-reload path via `config.yaml`. I'm inclined to drop
`test_dynamic_flink_conf.sh` (and its two `ci.yml` matrix rows) plus add a
short
migration note, but since that removes legacy coverage I'd rather confirm
with you
first.
(Aside, unrelated to this hunk: I've rebased onto latest `main` — the earlier
`test_application_operations.sh` smoke failure was a pipeline.jars/Flink
1.20.4
app-mode error on a stale base.)
--
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]
