bito-code-review[bot] commented on code in PR #40425:
URL: https://github.com/apache/superset/pull/40425#discussion_r3300982838
##########
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:
<!-- Bito Reply -->
The suggestion to extract the duplicated wait_for function into a shared
Helm template partial aims to reduce semantic duplication and maintenance
overhead. However, the user argues that keeping the duplication in
`values.yaml` is intentional for several reasons:
1. **Mechanical Limitations**: `values.yaml` is plain YAML and cannot
directly use `{{ include }}` partials, requiring a workaround that may not be
ideal.
2. **Readability**: The current setup allows users to see the actual bash
commands directly in `values.yaml`, which would be obscured by using a partial.
3. **Maintenance Overhead**: The duplicated bash code is stable and unlikely
to change, so the benefit of refactoring is minimal.
The user concludes that the current approach of explicit, override-friendly
defaults in `values.yaml` is preferable for this specific case.
**helm/superset/values.yaml**
```
- 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"
```
--
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]
