rusackas commented on code in PR #40425:
URL: https://github.com/apache/superset/pull/40425#discussion_r3300982186
##########
helm/superset/values.yaml:
##########
@@ -303,15 +298,28 @@ supersetNode:
# @default -- a container waiting for postgres
initContainers:
- name: wait-for-postgres
- image: "{{ .Values.initImage.repository }}:{{ .Values.initImage.tag }}"
- imagePullPolicy: "{{ .Values.initImage.pullPolicy }}"
+ image: "{{ .Values.image.repository }}:{{ .Values.image.tag | default
.Chart.AppVersion }}"
+ imagePullPolicy: "{{ .Values.image.pullPolicy }}"
envFrom:
- secretRef:
name: "{{ tpl .Values.envFromSecret . }}"
command:
- - /bin/sh
+ - /bin/bash
- -c
- - dockerize -wait "tcp://$DB_HOST:$DB_PORT" -timeout 120s
+ - |
+ # bash's /dev/tcp redirect performs a TCP connect; no external
+ # `dockerize`, `nc`, or busybox needed. SECONDS-based deadline
+ # mirrors the prior `dockerize -timeout 120s` behaviour.
+ SECONDS=0
+ until (echo > /dev/tcp/"$DB_HOST"/"$DB_PORT") 2>/dev/null; do
+ if [ "$SECONDS" -ge 120 ]; then
+ echo "timeout waiting for postgres at $DB_HOST:$DB_PORT after
120s" >&2
+ exit 1
+ fi
+ echo "waiting for postgres at $DB_HOST:$DB_PORT (elapsed
${SECONDS}s)"
+ sleep 2
+ done
+ echo "postgres at $DB_HOST:$DB_PORT is up"
resources:
limits:
memory: "256Mi"
Review Comment:
Thanks for the suggestion, but I'd push back here. The duplication is real
(~15 lines of shared bash across 5 blocks), but the reason these initContainers
live in `values.yaml` rather than in composed template partials is that they're
an **override surface** — operators are expected to read and replace them.
Three reasons to keep the diff as-is:
1. **`values.yaml` can't directly `{{ include }}` partials.** It's plain
YAML, parsed before templates render. The only way to share the bash via a
`_wait_for.tpl` partial would be to write `command: {{ include
"superset.waitForScript" . }}` in `values.yaml` and rely on the templates'
second-pass `tpl (toYaml .Values.supersetNode.initContainers)` rendering. That
works mechanically, but…
2. **It breaks the readability contract of `values.yaml`.** A user reading
the chart defaults today sees the actual bash that will run. Under the partial
pattern, they'd see an opaque `{{ include ... }}` and have to grep
`_helpers.tpl` (or wherever the partial lives) to learn what their init
container actually does. That's a strictly worse experience for the audience
`values.yaml` is meant to serve.
3. **No real reduction in maintenance overhead.** The duplicated bash is 15
lines that haven't changed in years on the dockerize side, and is unlikely to.
Operators who want to customise the script today copy/paste the literal block;
under the partial pattern they'd either still copy/paste (no gain) or have to
learn the partial indirection mechanism (lose).
The tradeoff in this chart has consistently favoured **explicit,
override-friendly defaults in `values.yaml`** over DRY-via-partials, and I
think that's the right call here too.
--
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]
