feat(helm): Superset 6 structured config schema with deprecation path#41777
feat(helm): Superset 6 structured config schema with deprecation path#41777lucargir wants to merge 1 commit into
Conversation
lucargir
requested review from
Antonio-RiveroMartnez,
dpgaspar,
hainenber,
michael-s-molina,
mistercrunch,
nytai,
rusackas and
villebro
as code owners
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| SQLALCHEMY_DATABASE_URI = f"{{ $driver }}://{{ include "superset.db.user" . }}:{{ include "superset.db.password" . }}@{{ include "superset.db.host" . }}:{{ include "superset.db.port" . }}/{{ include "superset.db.name" . }}{{ $sslParams }}" | ||
| {{- end }} | ||
| {{- if hasKey .Values.config "SQLALCHEMY_TRACK_MODIFICATIONS" }} | ||
| SQLALCHEMY_TRACK_MODIFICATIONS = {{ .Values.config.SQLALCHEMY_TRACK_MODIFICATIONS | lower }} |
There was a problem hiding this comment.
Suggestion: The generated Python uses lowercase boolean literals for this setting, but Python requires True/False. If users set this value, superset_config.py will fail to import with a NameError at startup. Render booleans using Python casing instead of piping through lower. [type error]
Severity Level: Critical 🚨
- ❌ Superset config import fails; web server does not start.
- ⚠️ Helm deployment pods enter crashloop due to invalid configuration.Steps of Reproduction ✅
1. Install the Helm chart in `helm/superset`, which renders
`secret-superset-config.yaml:29-34` and uses `{{- include "superset.config" . }}` to
generate the `superset_config.py` payload from `_helpers.tpl:200-671`.
2. Set `.Values.config.SQLALCHEMY_TRACK_MODIFICATIONS` to `true` or `"True"` in your
release values (documented as a generic config override in `helm/superset/README.md:30`),
so `_helpers.tpl:226-227` renders `SQLALCHEMY_TRACK_MODIFICATIONS = true` into
`superset_config.py`.
3. Start Superset; `superset/config.py:2931-2952` checks `SUPERSET_CONFIG_PATH` (or falls
back to importing `superset_config`) and executes the generated `superset_config.py`
module via `importlib.util.spec_from_file_location`.
4. During module execution, Python evaluates `SQLALCHEMY_TRACK_MODIFICATIONS = true` from
`superset_config.py`, raising `NameError` because `true` is undefined, causing the config
import to fail and preventing the Superset application from starting.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** helm/superset/templates/_helpers.tpl
**Line:** 227:227
**Comment:**
*Type Error: The generated Python uses lowercase boolean literals for this setting, but Python requires `True`/`False`. If users set this value, `superset_config.py` will fail to import with a `NameError` at startup. Render booleans using Python casing instead of piping through `lower`.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Fixed in e49092e096. SQLALCHEMY_TRACK_MODIFICATIONS now renders as a Python bool via toString | title (True/False). Added a regression test asserting SQLALCHEMY_TRACK_MODIFICATIONS = True when the value is set.
| {{ $key }} = {{ $value | toJson }} | ||
| {{- end }} | ||
|
|
||
| CELERY_IMPORTS = CeleryConfig.imports |
There was a problem hiding this comment.
Suggestion: This assumes every custom CeleryConfig defines an imports attribute; if users provide a minimal config without it, config import raises AttributeError and workers fail to start. Only assign this when imports exists or use a safe default. [api mismatch]
Severity Level: Critical 🚨
- ❌ Superset config import crashes if CeleryConfig lacks imports.
- ❌ Celery workers fail starting; background tasks never execute.Steps of Reproduction ✅
1. Configure a custom Celery settings map in Helm by setting `.Values.config.celeryConfig`
in `helm/superset/values.yaml` to a YAML object containing keys like `broker_url` and
`result_backend`, but not defining an `imports` key.
2. When `secret-superset-config.yaml:29-34` renders, `_helpers.tpl:302-314` detects
`.Values.config.celeryConfig` and, since `kindIs "string"` is false, emits a `class
CeleryConfig:` with one attribute per key in the map; no `imports` attribute is generated
unless the user provided it.
3. Immediately after, `_helpers.tpl:312` writes `CELERY_IMPORTS = CeleryConfig.imports`
and `CELERY_CONFIG = CeleryConfig` into `superset_config.py`, so the file contains an
assignment accessing a non-existent `CeleryConfig.imports` attribute.
4. On Superset startup, `superset/config.py:2931-2952` imports `superset_config` via
`importlib.util.spec_from_file_location`; Python executes `CELERY_IMPORTS =
CeleryConfig.imports`, raises `AttributeError` because `imports` is missing, and aborts
config import, preventing Celery-based background workers (superset tasks like
`superset.sql_lab` and schedulers) from starting properly.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** helm/superset/templates/_helpers.tpl
**Line:** 312:312
**Comment:**
*Api Mismatch: This assumes every custom CeleryConfig defines an `imports` attribute; if users provide a minimal config without it, config import raises `AttributeError` and workers fail to start. Only assign this when `imports` exists or use a safe default.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Fixed in e49092e096. CELERY_IMPORTS = CeleryConfig.imports is now only emitted when the custom celeryConfig actually defines an imports key (guarded with hasKey). Regression test added for a celeryConfig without imports.
| {{- if .Values.cache.sentinel }} | ||
| {{- if .Values.cache.sentinel.sentinels }} | ||
| "CACHE_REDIS_SENTINELS": {{ .Values.cache.sentinel.sentinels | toJson }}, | ||
| {{- else }} | ||
| {{- fail "CONFIGURATION ERROR: cache.sentinel.enabled is true but cache.sentinel.sentinels is not set. You must provide Sentinel host(s) in cache.sentinel.sentinels (e.g., [['sentinel-host', 26379]])." }} | ||
| {{- end }} |
There was a problem hiding this comment.
Suggestion: The sentinel block activates whenever cache.sentinel is any non-null map, even if users set enabled: false. This can trigger validation failures or sentinel-only settings when sentinel is intentionally disabled. Gate this block on cache.sentinel.enabled explicitly. [incorrect condition logic]
Severity Level: Major ⚠️
- ❌ Helm template rendering fails when sentinel map present disabled.
- ⚠️ Redis Sentinel toggles unusable without fully removing configuration.Steps of Reproduction ✅
1. Define Redis cache values in `helm/superset/values.yaml` with `.Values.cache.enabled =
true` and `.Values.cache.sentinel` set to a map containing `enabled: false` (or similar)
but omitting `sentinels`, so Sentinel is conceptually disabled but the configuration
object exists.
2. During chart rendering, `secret-superset-config.yaml:29-34` includes `superset.config`;
inside `_helpers.tpl:443-487` the `GLOBAL_ASYNC_QUERIES_CACHE_BACKEND` dict is built.
3. At `_helpers.tpl:461-466`, the condition `{{- if .Values.cache.sentinel }}` evaluates
truthy because `.Values.cache.sentinel` is a non-null map, and `{{- if
.Values.cache.sentinel.sentinels }}` evaluates falsy, so the template executes the `fail`
call with the message “CONFIGURATION ERROR: cache.sentinel.enabled is true but
cache.sentinel.sentinels is not set…”.
4. As a result, `helm install` or `helm upgrade` fails while rendering
`superset_config.py`, even though `cache.sentinel.enabled` is false and Sentinel was
intentionally disabled, preventing deployment and blocking configuration of the global
async queries cache backend.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** helm/superset/templates/_helpers.tpl
**Line:** 461:466
**Comment:**
*Incorrect Condition Logic: The sentinel block activates whenever `cache.sentinel` is any non-null map, even if users set `enabled: false`. This can trigger validation failures or sentinel-only settings when sentinel is intentionally disabled. Gate this block on `cache.sentinel.enabled` explicitly.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Fixed in e49092e096. The sentinel block is now gated on cache.sentinel.enabled (if and .Values.cache.sentinel .Values.cache.sentinel.enabled), so sentinel: {enabled: false} no longer triggers the validation fail. Test added.
| "CACHE_REDIS_SSL_CERTFILE": {{ .Values.cache.ssl.certfile | default "None" }}, | ||
| "CACHE_REDIS_SSL_KEYFILE": {{ .Values.cache.ssl.keyfile | default "None" }}, | ||
| "CACHE_REDIS_SSL_CERT_REQS": {{ .Values.cache.ssl.ssl_cert_reqs | default "required" | quote }}, | ||
| "CACHE_REDIS_SSL_CA_CERTS": {{ .Values.cache.ssl.ca_certs | default "None" }}, |
There was a problem hiding this comment.
Suggestion: These values are emitted without quoting, so string file paths render as bare Python tokens (for example /etc/ssl/cert.pem) and produce invalid syntax in superset_config.py. Emit proper quoted strings (or explicit None) for these fields. [type error]
Severity Level: Critical 🚨
- ❌ Superset config file contains invalid Redis SSL paths.
- ⚠️ Global async query cache backend cannot start with SSL.Steps of Reproduction ✅
1. Enable Redis SSL for global async queries in `helm/superset/values.yaml` by setting
`.Values.cache.enabled = true` and `.Values.cache.ssl.enabled = true` with file path
values such as `certfile: "/etc/ssl/cert.pem"`, `keyfile: "/etc/ssl/key.pem"`, and
`ca_certs: "/etc/ssl/ca.pem"`.
2. When Helm renders `secret-superset-config.yaml:29-34`, it includes `superset.config`;
inside `_helpers.tpl:474-480`, the SSL-enabled branch of the
`GLOBAL_ASYNC_QUERIES_CACHE_BACKEND` dict executes.
3. That branch emits lines like `"CACHE_REDIS_SSL_CERTFILE": /etc/ssl/cert.pem` and
`"CACHE_REDIS_SSL_CA_CERTS": /etc/ssl/ca.pem` without quotes, so the generated
`superset_config.py` contains bare path tokens inside a Python dict literal, which are not
valid Python expressions.
4. Superset loads configuration using `superset/config.py:2931-2952`, importing and
executing `superset_config.py`; Python parsing the dict with unquoted paths raises a
`SyntaxError`, causing config import to fail and preventing the global async queries Redis
cache backend from starting when SSL is configured.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** helm/superset/templates/_helpers.tpl
**Line:** 476:479
**Comment:**
*Type Error: These values are emitted without quoting, so string file paths render as bare Python tokens (for example `/etc/ssl/cert.pem`) and produce invalid syntax in `superset_config.py`. Emit proper quoted strings (or explicit `None`) for these fields.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Fixed in e49092e096. SSL certfile/keyfile/ca_certs are now quoted when set and rendered as bare None when unset, so the generated dict is always valid Python. Test asserts the quoted certfile; the rendered config is validated with ast.parse.
| {{- if .Values.config.GLOBAL_ASYNC_QUERIES_JWT_COOKIE_SECURE }} | ||
| GLOBAL_ASYNC_QUERIES_JWT_COOKIE_SECURE = {{ if .Values.config.GLOBAL_ASYNC_QUERIES_JWT_COOKIE_SECURE }}True{{ else }}False{{ end }} | ||
| {{- else if and .Values.supersetWebsockets.enabled (or (hasPrefix "wss://" $wsUrl) .Values.ingress.tls) }} | ||
| GLOBAL_ASYNC_QUERIES_JWT_COOKIE_SECURE = True |
There was a problem hiding this comment.
Suggestion: This condition checks truthiness instead of key presence, so an explicit false value is treated as “unset” and gets overwritten by auto-detection. That breaks user intent and can force secure-cookie behavior unexpectedly. Check with hasKey (or equivalent) before applying defaults. [falsy zero check]
Severity Level: Major ⚠️
- ⚠️ User-set JWT cookie secure flag silently overridden.
- ⚠️ Configuration values behave unexpectedly under WebSocket TLS.Steps of Reproduction ✅
1. Configure the Helm chart in `helm/superset/values.yaml` with
`.Values.supersetWebsockets.enabled = true` and TLS (`.Values.ingress.tls` non-empty), and
explicitly set `.Values.config.GLOBAL_ASYNC_QUERIES_JWT_COOKIE_SECURE = false`.
2. When `secret-superset-config.yaml:29-34` renders, `_helpers.tpl:570-575` handles the
value: the condition `if .Values.config.GLOBAL_ASYNC_QUERIES_JWT_COOKIE_SECURE` treats the
explicit `false` as falsy, so the template skips the user branch and enters the TLS
branch, emitting `GLOBAL_ASYNC_QUERIES_JWT_COOKIE_SECURE = True` into
`superset_config.py`.
3. Superset loads configuration via `superset/config.py:2931-2952`, importing
`superset_config` and copying its uppercase settings (including
`GLOBAL_ASYNC_QUERIES_JWT_COOKIE_SECURE`) into the main `superset.config` module and
`app.config`.
4. `superset/async_events/async_query_manager.py:150-151` reads
`app.config["GLOBAL_ASYNC_QUERIES_JWT_COOKIE_SECURE"]` to configure the JWT cookie; the
value is `True` because of auto-detection, ignoring the user’s explicit `False`, so
secure-cookie behavior cannot be disabled as intended.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** helm/superset/templates/_helpers.tpl
**Line:** 570:573
**Comment:**
*Falsy Zero Check: This condition checks truthiness instead of key presence, so an explicit `false` value is treated as “unset” and gets overwritten by auto-detection. That breaks user intent and can force secure-cookie behavior unexpectedly. Check with `hasKey` (or equivalent) before applying defaults.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Fixed in e49092e096. Now uses hasKey .Values.config "GLOBAL_ASYNC_QUERIES_JWT_COOKIE_SECURE" so an explicit false is honored instead of falling through to TLS auto-detection. Test added.
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #dc10d1
Actionable Suggestions - 5
-
helm/superset/tests/values_defaults_test.yaml - 1
- Incomplete test assertions · Line 25-27
-
helm/superset/templates/deployment.yaml - 1
- Missing checksum annotation consistency · Line 48-48
-
helm/superset/tests/deprecation_test.yaml - 1
- Wrong template targeted · Line 19-19
-
helm/superset/templates/_helpers.tpl - 2
- Incomplete admin password security check · Line 202-204
- CWE-20: Empty user causes Redis URL malformed · Line 452-452
Additional Suggestions - 5
-
helm/superset/templates/deployment-worker.yaml - 1
-
Backward compatibility gap · Line 48-51The checksum migration from `supersetNode.connections` to individual `database`, `cache`, `config`, `featureFlags` paths aligns with values.yaml deprecation notice. However, existing deployments using `supersetNode.connections` won't trigger pod restarts when those values change, potentially causing stale config deployment. Consider adding conditional fallback checksum or documentation for users to migrate.
-
-
helm/superset/templates/_helpers.tpl - 2
-
Repeated JSON parsing in helper templates · Line 127-196Each helper template (db.host, db.port, db.user, db.password, db.name, db.driver, redis.host, redis.port, redis.user, redis.password, redis.cacheDb, redis.celeryDb) independently calls `_superset.legacyConn`, parses the JSON result, and extracts one field. This results in ~15 redundant JSON parse operations per template evaluation.
-
Breaking change for users with empty admin password · Line 202-202The security validation at line 202 is a new feature that introduces a hard fail for empty passwords. Users upgrading from versions that allowed empty passwords may encounter template errors if they haven't set init.adminUser.password. This is a breaking change in behavior.
-
-
helm/superset/tests/initscript_test.yaml - 1
-
Test coverage gap · Line 31-50Test 'renders db upgrade before admin creation' asserts pattern existence but not execution order. The test name implies it verifies sequence, yet assertions only check that `superset db upgrade`, `superset init`, and `fab create-admin` each appear somewhere in the output. Use a combined regex (e.g., `superset db upgrade.*superset init.*fab create-admin`) or separate assertions with `notMatchRegex` to confirm `db upgrade` appears before `init`, and `init` before `create-admin`. Per BITO.md [6262], tests should verify actual behavior they claim to test.
-
-
helm/superset/Chart.yaml - 1
-
Version bump lacks chart changes · Line 32-32A minor version bump from 0.19.0 to 0.20.0 should reflect backward-compatible new features, bug fixes, or configuration changes to the chart itself per the README's versioning policy. No such changes are present in this diff. Verify that the intended chart modifications are included, or use a patch version bump (0.19.1) if this is a documentation-only change.
-
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
helm/superset/tests/config_test.yaml - 1
- Test assertion mismatch · Line 35-35
Review Details
-
Files reviewed - 18 · Commit Range:
76777f5..76777f5- helm/superset/Chart.yaml
- helm/superset/templates/NOTES.txt
- helm/superset/templates/_helpers.tpl
- helm/superset/templates/deployment-beat.yaml
- helm/superset/templates/deployment-flower.yaml
- helm/superset/templates/deployment-worker.yaml
- helm/superset/templates/deployment-ws.yaml
- helm/superset/templates/deployment.yaml
- helm/superset/templates/secret-env.yaml
- helm/superset/templates/secret-superset-config.yaml
- helm/superset/tests/checksum_test.yaml
- helm/superset/tests/config_test.yaml
- helm/superset/tests/deprecation_test.yaml
- helm/superset/tests/initscript_test.yaml
- helm/superset/tests/secret_env_test.yaml
- helm/superset/tests/serviceaccount_test.yaml
- helm/superset/tests/values_defaults_test.yaml
- helm/superset/values.yaml
-
Files skipped - 3
- helm/superset/README.md - Reason: Filter setting
- helm/superset/UPGRADING.md - Reason: Filter setting
- helm/superset/values.schema.json - Reason: Filter setting
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
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 documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| # | ||
| suite: deprecation path does not fail render | ||
| templates: | ||
| - secret-superset-config.yaml |
There was a problem hiding this comment.
This test references secret-superset-config.yaml but the deprecation mappings for supersetNode.connections and serviceAccountName are defined in _helpers.tpl and consumed by deployment and serviceaccount templates, not by the secret config template. The test will pass even when deprecation mapping is broken.
Code Review Run #dc10d1
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
There was a problem hiding this comment.
The actual legacy→new mapping value is asserted in config_test.yaml (e.g. legacy supersetNode.connections.* producing the correct SQLALCHEMY_DATABASE_URI). This suite intentionally verifies the separate contract that deprecated keys do not hard-fail the render, which is the deprecation-path design. Keeping both.
There was a problem hiding this comment.
The suggestion provided by the reviewer is based on the observation that the test file secret-superset-config.yaml does not consume the deprecation mappings defined in _helpers.tpl. If the intention of the test is to verify that the deprecation path does not cause a render failure, it is appropriate to ensure the test targets a template that actually utilizes those mappings. You may choose to ignore this suggestion if you believe the current test coverage is sufficient for your design goals, or you can update the test to target a template that consumes the relevant mappings.
Restructure the chart configuration onto a first-class, documented values schema and render superset_config.py from it, while keeping existing installs working via a deprecation path (old keys auto-map + a NOTES warning, never a hard fail). Highlights: - New values sections: database.*, cache.*, cluster.*, config.*, featureFlags.* (+ globalPodAnnotations). superset_config.py is rendered by a new superset.config helper; the legacy superset-config define is removed. - Backward compatible: coalescing resolver helpers map legacy supersetNode.connections.* and root serviceAccountName onto the new schema; a deprecation notice is surfaced in NOTES.txt. Nothing fails on deprecated keys. - cache.driver ports the legacy redis_driver escape hatch (REDIS_DRIVER). - serviceAccount.name is now honored (with legacy serviceAccountName fallback). - init script runs superset db upgrade + superset init in addition to admin/examples; init.initscript is deprecated/unused. - Permissive values.schema.json; deployment checksums track the new config sections; bash /dev/tcp init wait (no dockerize); SQLALCHEMY_TRACK_MODIFICATIONS defaults False. - Generated superset_config.py is always valid Python: booleans render as True/False, SSL cert paths are quoted, CELERY_IMPORTS is only emitted when defined, sentinel config is gated on enabled. - Docs: UPGRADING.md; README regenerated. Chart 0.20.0 (appVersion 6.1.0). Validated: helm lint, helm template (no <no value>), kubeconform 1.29 (strict), 69 helm-unittest cases, and ast.parse of the rendered config.
lucargir
force-pushed
the
feat/helm-superset-6
branch
from
76777f5 to
e49092e
Compare
There was a problem hiding this comment.
Code Review Agent Run #4f624c
Actionable Suggestions - 1
-
helm/superset/templates/_helpers.tpl - 1
- CWE-778: Missing healthCheck config · Line 354-380
Additional Suggestions - 5
-
helm/superset/templates/secret-env.yaml - 1
-
Missing Redis SSL env var · Line 35-36Redis SSL mode is set to 'rediss' via `superset.redis.proto` when `.Values.cache.ssl.enabled` is true, but the SSL certificate requirement mode is no longer exported as an environment variable. The old code exported `REDIS_SSL_CERT_REQS` unconditionally (line 40 of old), ensuring Redis clients know how to validate certificates. Without this env var, the Redis client defaults may not match the server's TLS requirements, causing connection failures. Add the conditional export similar to `DB_SSL_MODE` pattern on line 35.
-
-
helm/superset/tests/deprecation_test.yaml - 1
-
Weak test assertions for deprecated values · Line 21-27Tests verify rendering succeeds but don't validate that deprecated values produce correct output. Per rule 6262, tests should assert actual behavior (e.g., regex match on rendered config) rather than only confirming `hasDocuments: {count: 1}`. This prevents regressions where deprecated paths silently produce wrong values.
-
-
helm/superset/templates/_helpers.tpl - 1
-
Dead configuration variable · Line 406-406`CELERY_WORKER_HEALTH_CHECK_ENABLED` is defined at line 406 but never imported or referenced by any Python file in the repository. It is dead configuration that pollutes the generated config and may mislead operators into believing it has a runtime effect.
-
-
helm/superset/values.yaml - 1
-
Missing deprecation test for init.initscript · Line 958-962The `init.initscript` field is documented as deprecated and silently ignored (line 958-962), but unlike the other deprecated fields, there is no test in `deprecation_test.yaml` verifying that setting it does not break template rendering. Adding a test would complete the deprecation coverage pattern already established for `supersetNode.connections` and `serviceAccountName`.
-
-
helm/superset/templates/deployment-ws.yaml - 1
-
Missing co-existence test · Line 47-49The added block correctly implements globalPodAnnotations support for the websocket deployment, matching the pattern in other templates (deployment.yaml line 66, deployment-worker.yaml line 64, etc.). However, the test suite has a co-existence test for the main deployment (line 37-50) but lacks an equivalent test for this websocket deployment.
-
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
helm/superset/tests/values_defaults_test.yaml - 1
- Test duplicates existing functionality · Line 22-30
Review Details
-
Files reviewed - 18 · Commit Range:
e49092e..e49092e- helm/superset/Chart.yaml
- helm/superset/templates/NOTES.txt
- helm/superset/templates/_helpers.tpl
- helm/superset/templates/deployment-beat.yaml
- helm/superset/templates/deployment-flower.yaml
- helm/superset/templates/deployment-worker.yaml
- helm/superset/templates/deployment-ws.yaml
- helm/superset/templates/deployment.yaml
- helm/superset/templates/secret-env.yaml
- helm/superset/templates/secret-superset-config.yaml
- helm/superset/tests/checksum_test.yaml
- helm/superset/tests/config_test.yaml
- helm/superset/tests/deprecation_test.yaml
- helm/superset/tests/initscript_test.yaml
- helm/superset/tests/secret_env_test.yaml
- helm/superset/tests/serviceaccount_test.yaml
- helm/superset/tests/values_defaults_test.yaml
- helm/superset/values.yaml
-
Files skipped - 3
- helm/superset/README.md - Reason: Filter setting
- helm/superset/UPGRADING.md - Reason: Filter setting
- helm/superset/values.schema.json - Reason: Filter setting
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
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 documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| {{- /* Celery Worker Health Check - File-based health probes for Kubernetes */}} | ||
| {{- if and .Values.supersetWorker.healthCheck .Values.supersetWorker.healthCheck.enabled }} | ||
| # Celery Worker Health Check Configuration | ||
| import threading | ||
| from celery import bootsteps | ||
| from celery.signals import worker_ready, worker_shutdown, worker_init | ||
|
|
||
| _readiness_file = {{ .Values.supersetWorker.healthCheck.readinessFile | default "/tmp/celery_worker_ready" | quote }} | ||
| _liveness_file = {{ .Values.supersetWorker.healthCheck.livenessFile | default "/tmp/celery_worker_alive" | quote }} | ||
| _heartbeat_interval = {{ .Values.supersetWorker.healthCheck.livenessHeartbeatInterval | default 10 | int }} | ||
| _liveness_thread = None | ||
| _liveness_stop_event = None | ||
|
|
||
| @worker_ready.connect | ||
| def create_ready_file(sender, **kwargs): | ||
| try: | ||
| open(_readiness_file, 'w').close() | ||
| except Exception as e: | ||
| print(f"Warning: Could not create readiness file: {e}") | ||
|
|
||
| @worker_shutdown.connect | ||
| def remove_ready_file(sender, **kwargs): | ||
| global _liveness_thread, _liveness_stop_event | ||
| if _liveness_stop_event: | ||
| _liveness_stop_event.set() | ||
| if _liveness_thread: | ||
| _liveness_thread.join(timeout=5) |
There was a problem hiding this comment.
The template references .Values.supersetWorker.healthCheck.enabled (line 355) and its subkeys, but values.yaml does not define supersetWorker.healthCheck. This causes the template to generate None for all values at render time, making the health check configuration non-functional. Add the missing healthCheck section under supersetWorker in values.yaml. (See also: CWE-778)
Code Review Run #4f624c
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
|
@rusackas regarding the Helm chart, just read on #40507 that it might get deprecated in favour of the operator. If that's the case, I guess the refactor I proposed on #38597 wouldn't make sense? Happy to keep bringing up the pending refactor PRs, but would rather contribute in other aspects if the future of the chart is uncertain. |
1 participant
SUMMARY
Restructures the Helm chart configuration onto a first-class, documented values schema and renders
superset_config.pyfrom it, while keeping existing installs working through a deprecation path (old keys are auto-mapped and a warning is printed inNOTES.txt— nothing hard-fails).This is the "core" of a larger, previously-split chart effort; it is the foundation the follow-up chart PRs build on. It intentionally preserves recent chart behavior on
master(bash/dev/tcpinit wait,redis_driver,SQLALCHEMY_TRACK_MODIFICATIONS=False).Highlights
database.*,cache.*,cluster.*,config.*,featureFlags.*(+globalPodAnnotations).superset_config.pyis rendered by a newsuperset.confighelper; the legacysuperset-configdefine is removed.supersetNode.connections.*and rootserviceAccountNameonto the new schema; a deprecation notice is surfaced inNOTES.txt. Existingvalues.yamlfiles keep working on upgrade.cache.driverports the legacyredis_driverescape hatch (REDIS_DRIVER).serviceAccount.nameis now honored (with legacyserviceAccountNamefallback), fixing a gap where the new key was never read.superset db upgrade+superset init(schema migration) in addition to admin/examples;init.initscriptis deprecated/unused.values.schema.json(permissive) so bad values failhelm installearly without rejecting existing values files.UPGRADING.mdmigration guide;README.mdregenerated. Chart bumped to 0.20.0 (appVersion unchanged,6.1.0).helm upgradecontinues to work. Seehelm/superset/UPGRADING.md.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — Helm chart change, no UI.
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION