You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "rusackas (via GitHub)" <gi...@apache.org> on 2026/05/26 02:07:17 UTC
[PR] fix(helm)!: replace dockerize initContainer with bash TCP wait [superset]
rusackas opened a new pull request, #40425:
URL: https://github.com/apache/superset/pull/40425
### SUMMARY
Drops `apache/superset:dockerize` from the chart entirely. The five `initContainers` that gate startup on Postgres / Redis now run from the same `apache/superset` image we're already pulling, using bash's built-in `/dev/tcp/host/port` redirect for the readiness probe — no external `dockerize`, `nc`, or busybox needed.
A trivy scan of the current published `apache/superset:dockerize` (image created **2024-05-09**, base **alpine 3.19.1 EOSL**) found:
| Severity | Count | Fix available |
|---|---|---|
| CRITICAL | 3 | yes |
| HIGH | 25 | yes |
| MEDIUM | 71 | yes |
| LOW | 24 | yes |
| **Total** | **123** | **123/123** |
64 of the CVEs are in the bundled `dockerize` Go binary itself (stale Go stdlib + `golang.org/x/{net,crypto}`); the rest are in the alpine base packages (`libcrypto3`, `libssl3`, `busybox`, `musl`). Rebuilding the image on a fresher base would just defer the same problem; removing the dependency eliminates it.
### Why bash `/dev/tcp` over the alternatives
- vs **`busybox + nc -z`**: introduces an external image dependency, which is what we're trying to escape.
- vs **a Chainguard image**: external registry dependency that hurts air-gapped users, and we don't need a hardened build of a non-trivial tool — we just need a TCP connect, which bash does for free.
- vs **rebuilding `apache/superset:dockerize` on a fresher base**: kicks the can; we'd be back here in another year.
Verified `/bin/bash` 5.2.15 is present in `apache/superset:latest` and supports the `/dev/tcp` redirect (the image's `/bin/sh` is dash, which does not — hence the explicit `/bin/bash`).
### Changes
- `helm/superset/values.yaml`:
- Removed the `initImage:` block.
- Rewrote all 5 `initContainers` blocks (`init`, `supersetNode`, `supersetWorker`, `supersetCeleryBeat`, `supersetCeleryFlower`) to use `{{ .Values.image.repository }}:{{ .Values.image.tag | default .Chart.AppVersion }}` and a bash `/dev/tcp` loop. The 120s timeout from `dockerize -timeout 120s` is preserved via a `SECONDS`-based deadline. Two-port waits factor out a small `wait_for` helper.
- `helm/superset/Chart.yaml`: chart version 0.15.5 → **0.16.0** (minor bump per the chart's documented [versioning policy](https://github.com/apache/superset/blob/master/helm/superset/README.md#versioning) — default behaviour changed; a documented value was removed).
- `helm/superset/README.md`: auto-regenerated by `helm-docs` pre-commit hook (the three `initImage.*` rows are gone; version badge bumped).
### Compatibility
| User profile | Impact |
|---|---|
| Default chart user | None visible. Same wait behaviour, different binary. |
| Overrides `.Values.initImage.repository/tag/pullPolicy` | **Breaking.** The value is no longer read. Overrides should be removed; the chart will pull from `.Values.image` automatically. |
| Fully overrides `.Values.supersetNode.initContainers` (etc.) with their own block | None — the override still wins. |
| Pinned `apache/superset:dockerize` in network policies / image allowlists | Can be removed once on chart 0.16.0. |
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — chart-only change.
### TESTING INSTRUCTIONS
```bash
cd helm/superset
helm dependency build
helm template . --skip-tests --skip-schema-validation \
--set supersetCeleryBeat.enabled=true \
--set supersetCeleryFlower.enabled=true \
| grep -B1 -A3 'name: wait-for'
```
Expect 5 `wait-for-*` blocks, all using `apache/superset:5.0.0` (or whatever `appVersion` resolves to), all running `/bin/bash` with a `/dev/tcp` probe.
To exercise the actual deploy:
```bash
helm install superset . --create-namespace --namespace superset
kubectl -n superset logs -l app=superset -c wait-for-postgres -f
# expect: "waiting for postgres at ... (elapsed Ns)" lines then "postgres at ... is up"
```
### ADDITIONAL INFORMATION
- [x] Has associated issue: #40424
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration
- [ ] Introduces new feature or API
- [x] Removes existing feature or API (the `initImage` values block)
### Out of scope (good follow-ups)
- Stop publishing `apache/superset:dockerize` from `.github/workflows/docker.yml` and `tag-release.yml`. Worth one release of overlap so users on older chart versions still get fresh builds while they upgrade, then a separate PR to drop the matrix entry.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(helm)!: replace dockerize initContainer with bash TCP wait [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40425:
URL: https://github.com/apache/superset/pull/40425#issuecomment-4676564017
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(helm)!: replace dockerize initContainer with bash TCP wait [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #40425:
URL: https://github.com/apache/superset/pull/40425#discussion_r3328115831
##########
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 acknowledging the constraints. Agree there's real duplication, but the mechanics of and the operator override story make a shared partial impractical here. We'll leave the diff as-is.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(helm)!: replace dockerize initContainer with bash TCP wait [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #40425:
URL: https://github.com/apache/superset/pull/40425#issuecomment-4736167459
@sha174n good catch on the newline payload — switched all the probes to `(exec 3<>/dev/tcp/...)` so it's a pure connect, no more `incomplete startup packet` noise. And yeah, the soft-ceiling caveat is real; for the connection-refused case it's fine, and I'd rather keep this dependency-free than reach for a `timeout` wrapper that isn't guaranteed present. Thanks for the review!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(helm)!: replace dockerize initContainer with bash TCP wait [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #40425:
URL: https://github.com/apache/superset/pull/40425#discussion_r3300789455
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Semantic duplication in diff</b></div>
<div id="fix">
Extract the duplicated wait_for function into a shared Helm template partial (e.g., `_wait_for.tpl`) and reference it in each initContainers section to avoid semantic duplication and reduce maintenance overhead.
</div>
</div>
<small><i>Code Review Run #29c792</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(helm)!: replace dockerize initContainer with bash TCP wait [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #40425:
URL: https://github.com/apache/superset/pull/40425#discussion_r3328182521
##########
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:
Understood — agreed the tradeoffs are clear. Closing the loop here.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(helm)!: replace dockerize initContainer with bash TCP wait [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40425:
URL: https://github.com/apache/superset/pull/40425#issuecomment-4737102613
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(helm)!: replace dockerize initContainer with bash TCP wait [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #40425:
URL: https://github.com/apache/superset/pull/40425#issuecomment-4736176566
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 5c1ef9398e2ebf635e4c40c32e89cf9f7a97f098 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6a3323018ff52c00085b8b5a |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-40425--superset-docs-preview.netlify.app](https://deploy-preview-40425--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#q1YqKMrPLShRslJS0lFKKkrMS84AstMyK_QzUnNydYtSC3ISk1N1U_KTs1OLMqtSgapSgGL5lZ4pQHVmicbGRsYGhhZpaaZGyQYGBhamSRZJpolKtQA) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-40425--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(helm)!: replace dockerize initContainer with bash TCP wait [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(helm)!: replace dockerize initContainer with bash TCP wait [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(helm)!: replace dockerize initContainer with bash TCP wait [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #40425:
URL: https://github.com/apache/superset/pull/40425#discussion_r3300789455
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Semantic duplication in diff</b></div>
<div id="fix">
Extract the duplicated wait_for function into a shared Helm template partial (e.g., `_wait_for.tpl`) and reference it in each initContainers section to avoid semantic duplication and reduce maintenance overhead.
</div>
</div>
<small><i>Code Review Run #29c792</i></small>
</div><div>
<div id="suggestion">
<div id="issue"><b>Semantic duplication in init containers</b></div>
<div id="fix">
The bash wait-for-port logic is copy-pasted verbatim across three initContainers (supersetNode, supersetWorker, supersetCeleryBeat). Extract into a shared Helm partial template to prevent divergence when one copy is updated without the others.
</div>
</div>
<small><i>Code Review Run #e49b4d</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(helm)!: replace dockerize initContainer with bash TCP wait [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40425:
URL: https://github.com/apache/superset/pull/40425#issuecomment-4539096846
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(helm)!: replace dockerize initContainer with bash TCP wait [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #40425:
URL: https://github.com/apache/superset/pull/40425#discussion_r3328116242
##########
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.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(helm)!: replace dockerize initContainer with bash TCP wait [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40425:
URL: https://github.com/apache/superset/pull/40425#issuecomment-4735303350
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(helm)!: replace dockerize initContainer with bash TCP wait [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #40425:
URL: https://github.com/apache/superset/pull/40425
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org