codeant-ai-for-open-source[bot] commented on code in PR #38597:
URL: https://github.com/apache/superset/pull/38597#discussion_r2981130232
##########
helm/superset/templates/_helpers.tpl:
##########
@@ -148,27 +682,124 @@ RESULTS_BACKEND = RedisCache(
{{- end }}
+{{/*
+Component-specific selector labels
+These use the standardized Kubernetes recommended labels pattern:
+ - app.kubernetes.io/name: Application name
+ - app.kubernetes.io/instance: Release name
+ - app.kubernetes.io/component: Component identifier
+See:
https://kubernetes.io/docs/concepts/overview/working-with-objects/common-labels/
+*/}}
+
+{{- define "supersetNode.selectorLabels" -}}
+app.kubernetes.io/name: {{ include "superset.name" . }}
+app.kubernetes.io/instance: {{ .Release.Name }}
+app.kubernetes.io/component: web
+{{- end }}
+
{{- define "supersetCeleryBeat.selectorLabels" -}}
-app: {{ include "superset.name" . }}-celerybeat
-release: {{ .Release.Name }}
+app.kubernetes.io/name: {{ include "superset.name" . }}
+app.kubernetes.io/instance: {{ .Release.Name }}
+app.kubernetes.io/component: celerybeat
{{- end }}
{{- define "supersetCeleryFlower.selectorLabels" -}}
-app: {{ include "superset.name" . }}-flower
-release: {{ .Release.Name }}
+app.kubernetes.io/name: {{ include "superset.name" . }}
+app.kubernetes.io/instance: {{ .Release.Name }}
+app.kubernetes.io/component: flower
{{- end }}
-{{- define "supersetNode.selectorLabels" -}}
-app: {{ include "superset.name" . }}
-release: {{ .Release.Name }}
+{{- define "supersetWorker.selectorLabels" -}}
+app.kubernetes.io/name: {{ include "superset.name" . }}
+app.kubernetes.io/instance: {{ .Release.Name }}
+app.kubernetes.io/component: worker
{{- end }}
{{- define "supersetWebsockets.selectorLabels" -}}
-app: {{ include "superset.name" . }}-ws
-release: {{ .Release.Name }}
+app.kubernetes.io/name: {{ include "superset.name" . }}
+app.kubernetes.io/instance: {{ .Release.Name }}
+app.kubernetes.io/component: websocket
{{- end }}
-{{- define "supersetWorker.selectorLabels" -}}
-app: {{ include "superset.name" . }}-worker
-release: {{ .Release.Name }}
+
+{{- define "superset.defaultInitContainers" -}}
+{{- $waitCommands := "" }}
+{{- /* Database - Superset always requires a database */}}
+{{- /* Auto-detect host from release name if not explicitly set */}}
+{{- $dbHost := .Values.database.host }}
+{{- if not $dbHost }}
+{{- if .Values.cluster.databaseServiceName }}
+{{- $dbHost = .Values.cluster.databaseServiceName }}
+{{- else }}
+{{- $dbHost = printf "%s-postgresql" .Release.Name }}
+{{- end }}
+{{- end }}
+{{- $dbPort := .Values.database.port | default 5432 }}
+{{- $waitCommands = printf "-wait tcp://%s:%d" $dbHost (int $dbPort) }}
+{{- /* Redis - only wait if cache is enabled */}}
+{{- if .Values.cache.enabled }}
+{{- $redisHost := .Values.cache.host }}
+{{- if not $redisHost }}
+{{- if .Values.cluster.redisServiceName }}
+{{- $redisHost = .Values.cluster.redisServiceName }}
+{{- else }}
+{{- $redisHost = printf "%s-redis-headless" .Release.Name }}
+{{- end }}
+{{- end }}
+{{- $redisPort := .Values.cache.port | default 6379 }}
+{{- $waitCommands = printf "%s -wait tcp://%s:%d" $waitCommands $redisHost
(int $redisPort) }}
Review Comment:
**Suggestion:** Redis wait logic falls back to `<release>-redis-headless`
whenever `cache.host` is empty, even when `cache.cacheUrl`/`cache.celeryUrl`
are provided. This makes init containers wait for an in-cluster Redis service
that may not exist when using URL-based external Redis configuration. [logic
error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Init containers block on wrong Redis hostname.
- ⚠️ Worker/beat/flower/web deployments start slowly or fail.
```
</details>
```suggestion
{{- if and (not $redisHost) (not .Values.cache.cacheUrl) (not
.Values.cache.celeryUrl) }}
{{- if .Values.cluster.redisServiceName }}
{{- $redisHost = .Values.cluster.redisServiceName }}
{{- else }}
{{- $redisHost = printf "%s-redis-headless" .Release.Name }}
{{- end }}
{{- end }}
{{- if $redisHost }}
{{- $redisPort := .Values.cache.port | default 6379 }}
{{- $waitCommands = printf "%s -wait tcp://%s:%d" $waitCommands $redisHost
(int $redisPort) }}
{{- end }}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Use default init-container path (`templates/deployment*.yaml`,
`templates/init-job.yaml`, `templates/upgrade-job.yaml`) which includes
`superset.defaultInitContainers`.
2. Configure URL-based Redis (`cache.cacheUrl`/`cache.celeryUrl`) with empty
`cache.host`.
3. `_helpers.tpl:740-750` still falls back to `{{ .Release.Name
}}-redis-headless` and
appends Redis wait target.
4. Config generation in `_helpers.tpl:206-207` and `:243-244` uses the
provided URLs, so
app points to external Redis while init waits on fallback host, causing init
timeouts.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** helm/superset/templates/_helpers.tpl
**Line:** 742:750
**Comment:**
*Logic Error: Redis wait logic falls back to `<release>-redis-headless`
whenever `cache.host` is empty, even when `cache.cacheUrl`/`cache.celeryUrl`
are provided. This makes init containers wait for an in-cluster Redis service
that may not exist when using URL-based external Redis configuration.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38597&comment_hash=f0a762875e1bc4e74b68223e2be756505ccd0321393da48c16405d192e410aa4&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38597&comment_hash=f0a762875e1bc4e74b68223e2be756505ccd0321393da48c16405d192e410aa4&reaction=dislike'>👎</a>
##########
helm/superset/templates/_helpers.tpl:
##########
@@ -148,27 +682,124 @@ RESULTS_BACKEND = RedisCache(
{{- end }}
+{{/*
+Component-specific selector labels
+These use the standardized Kubernetes recommended labels pattern:
+ - app.kubernetes.io/name: Application name
+ - app.kubernetes.io/instance: Release name
+ - app.kubernetes.io/component: Component identifier
+See:
https://kubernetes.io/docs/concepts/overview/working-with-objects/common-labels/
+*/}}
+
+{{- define "supersetNode.selectorLabels" -}}
+app.kubernetes.io/name: {{ include "superset.name" . }}
+app.kubernetes.io/instance: {{ .Release.Name }}
+app.kubernetes.io/component: web
+{{- end }}
+
{{- define "supersetCeleryBeat.selectorLabels" -}}
-app: {{ include "superset.name" . }}-celerybeat
-release: {{ .Release.Name }}
+app.kubernetes.io/name: {{ include "superset.name" . }}
+app.kubernetes.io/instance: {{ .Release.Name }}
+app.kubernetes.io/component: celerybeat
{{- end }}
{{- define "supersetCeleryFlower.selectorLabels" -}}
-app: {{ include "superset.name" . }}-flower
-release: {{ .Release.Name }}
+app.kubernetes.io/name: {{ include "superset.name" . }}
+app.kubernetes.io/instance: {{ .Release.Name }}
+app.kubernetes.io/component: flower
{{- end }}
-{{- define "supersetNode.selectorLabels" -}}
-app: {{ include "superset.name" . }}
-release: {{ .Release.Name }}
+{{- define "supersetWorker.selectorLabels" -}}
+app.kubernetes.io/name: {{ include "superset.name" . }}
+app.kubernetes.io/instance: {{ .Release.Name }}
+app.kubernetes.io/component: worker
{{- end }}
{{- define "supersetWebsockets.selectorLabels" -}}
-app: {{ include "superset.name" . }}-ws
-release: {{ .Release.Name }}
+app.kubernetes.io/name: {{ include "superset.name" . }}
+app.kubernetes.io/instance: {{ .Release.Name }}
+app.kubernetes.io/component: websocket
{{- end }}
-{{- define "supersetWorker.selectorLabels" -}}
-app: {{ include "superset.name" . }}-worker
-release: {{ .Release.Name }}
+
+{{- define "superset.defaultInitContainers" -}}
+{{- $waitCommands := "" }}
+{{- /* Database - Superset always requires a database */}}
+{{- /* Auto-detect host from release name if not explicitly set */}}
+{{- $dbHost := .Values.database.host }}
+{{- if not $dbHost }}
+{{- if .Values.cluster.databaseServiceName }}
+{{- $dbHost = .Values.cluster.databaseServiceName }}
+{{- else }}
+{{- $dbHost = printf "%s-postgresql" .Release.Name }}
+{{- end }}
+{{- end }}
+{{- $dbPort := .Values.database.port | default 5432 }}
+{{- $waitCommands = printf "-wait tcp://%s:%d" $dbHost (int $dbPort) }}
Review Comment:
**Suggestion:** The default init-container wait logic always falls back to
`<release>-postgresql` when `database.host` is empty, even if `database.uri` is
set. Because `database.uri` is documented to override host/port fields, this
causes pods to block waiting on a non-existent in-cluster PostgreSQL service
for external DB URI deployments. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Pods stuck Init waiting nonexistent fallback PostgreSQL service.
- ⚠️ Web, worker, beat, init, upgrade startup delayed.
```
</details>
```suggestion
{{- if and (not $dbHost) (not .Values.database.uri) }}
{{- if .Values.cluster.databaseServiceName }}
{{- $dbHost = .Values.cluster.databaseServiceName }}
{{- else }}
{{- $dbHost = printf "%s-postgresql" .Release.Name }}
{{- end }}
{{- end }}
{{- if $dbHost }}
{{- $dbPort := .Values.database.port | default 5432 }}
{{- $waitCommands = printf "-wait tcp://%s:%d" $dbHost (int $dbPort) }}
{{- end }}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Render/use default init containers path in
`templates/deployment.yaml:100-102`,
`templates/init-job.yaml:71-73`, and other components that include
`superset.defaultInitContainers`.
2. Set `database.uri` and leave `database.host` empty (documented override at
`values.yaml:243-245` and `README.md:102`).
3. In `templates/_helpers.tpl:729-738`, helper still computes `$dbHost`
fallback as `{{
.Release.Name }}-postgresql` and always appends `-wait
tcp://<fallback>:5432`.
4. Init container command at `templates/_helpers.tpl:769-770` runs
`dockerize` against
that fallback host, causing repeated init wait/timeout when no in-cluster
PostgreSQL
service exists.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** helm/superset/templates/_helpers.tpl
**Line:** 730:738
**Comment:**
*Logic Error: The default init-container wait logic always falls back
to `<release>-postgresql` when `database.host` is empty, even if `database.uri`
is set. Because `database.uri` is documented to override host/port fields, this
causes pods to block waiting on a non-existent in-cluster PostgreSQL service
for external DB URI deployments.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38597&comment_hash=15222368d902891fb0beed67e37029c44114004863280d5260da99f16b14f66c&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38597&comment_hash=15222368d902891fb0beed67e37029c44114004863280d5260da99f16b14f66c&reaction=dislike'>👎</a>
--
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]
