feat(helm): Make all container images configurable in values.yaml (resolves #2047). - #2305
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR generalizes Helm image handling: a new component-aware image helper and values were added; setup scripts now pass component keys; chart templates were updated to use the new helper and read component image/pullPolicy values; docs show example image overrides. ChangesComponent-based image reference refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/src/user-docs/guides-k8s-deployment.md`:
- Around line 216-218: The kubectl image reference is using a non-standard tag
"sha256-..." which will fail; update the kubectl entry so the digest is
expressed as a repository digest (repository: "bitnami/kubectl@sha256:<digest>")
or replace tag with a proper version string (tag: "1.30.0"), i.e., move the
sha256 value into the repository with an "`@sha256`:" prefix or use a semantic
version in the tag field; edit the kubectl block and adjust the repository and
tag keys accordingly.
In `@tools/deployment/package-helm/templates/_helpers.tpl`:
- Around line 141-155: The helper "clp.image.ref" always emits repository:tag
and drops digest-pinned images; update that template so after resolving $img and
$tag it checks for an optional digest field (e.g., $img.digest) and, if present,
returns "repository@digest" instead of "%s:%s", otherwise keep the existing tag
logic (including the clpPackage fallback to .root.Chart.AppVersion and the fail
when tag is required). Ensure you reference the same symbols ($img, $tag,
.component, .root.Values.image) and use the digest path when available so
digest-pinned third-party images are preserved.
In `@tools/deployment/package-helm/templates/presto-worker-deployment.yaml`:
- Around line 33-35: The initContainer named "setup-configs" uses the
component-based kubectl image via the include "clp.image.ref" but does not set
imagePullPolicy, so .Values.image.kubectl.pullPolicy is ignored; update the
initContainer spec for setup-configs to add imagePullPolicy and wire it to the
Helm value (the same place other migrated components read
.Values.image.kubectl.pullPolicy) so the pull policy is honored consistently for
the kubectl image.
In `@tools/deployment/package-helm/values.yaml`:
- Around line 25-28: The kubectl image tag is currently a digest string which
will be rendered as repository:tag by clp.image.ref and causes ImagePullBackOff;
update the kubectl tag field (in the kubectl block) to a real version tag (e.g.,
"v1.36.0" or the Bitnami-provided semver) so clp.image.ref produces a valid
repository:tag and clp.waitFor initContainers can pull the image successfully;
modify the tag key under kubectl (not repository or pullPolicy) accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 011f710c-f52f-4d44-b73d-30d427fc4cdb
📒 Files selected for processing (24)
docs/src/user-docs/guides-k8s-deployment.mdtools/deployment/package-helm/.set-up-common.shtools/deployment/package-helm/set-up-multi-dedicated-test.shtools/deployment/package-helm/set-up-multi-shared-test.shtools/deployment/package-helm/set-up-test.shtools/deployment/package-helm/templates/_helpers.tpltools/deployment/package-helm/templates/api-server-deployment.yamltools/deployment/package-helm/templates/compression-scheduler-deployment.yamltools/deployment/package-helm/templates/compression-worker-deployment.yamltools/deployment/package-helm/templates/database-statefulset.yamltools/deployment/package-helm/templates/db-table-creator-job.yamltools/deployment/package-helm/templates/garbage-collector-deployment.yamltools/deployment/package-helm/templates/log-ingestor-deployment.yamltools/deployment/package-helm/templates/mcp-server-deployment.yamltools/deployment/package-helm/templates/presto-worker-deployment.yamltools/deployment/package-helm/templates/query-scheduler-deployment.yamltools/deployment/package-helm/templates/query-worker-deployment.yamltools/deployment/package-helm/templates/queue-statefulset.yamltools/deployment/package-helm/templates/redis-statefulset.yamltools/deployment/package-helm/templates/reducer-deployment.yamltools/deployment/package-helm/templates/results-cache-indices-creator-job.yamltools/deployment/package-helm/templates/results-cache-statefulset.yamltools/deployment/package-helm/templates/webui-deployment.yamltools/deployment/package-helm/values.yaml
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/deployment/package-helm/templates/_helpers.tpl (1)
157-157:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winError message omits variant when present.
When a variant is supplied (e.g.,
component: "database",variant: "mariadb"), the fail message will readimage.database.tag is requiredinstead ofimage.database.mariadb.tag is required, which obscures the actual path invalues.yamland makes debugging harder.📝 Proposed fix to include variant in error message
+{{- $componentPath := .component -}} +{{- if hasKey . "variant" -}} + {{- $componentPath = printf "%s.%s" .component .variant -}} +{{- end -}} {{- if not $tag -}} {{- if eq .component "clpPackage" -}} {{- $tag = .root.Chart.AppVersion -}} {{- else -}} - {{- fail (printf "image.%s.tag is required" .component) -}} + {{- fail (printf "image.%s.tag is required" $componentPath) -}} {{- end -}} {{- end -}}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/deployment/package-helm/templates/_helpers.tpl` at line 157, The fail message always uses only .component; update the failing call that currently uses fail (printf "image.%s.tag is required" .component) so it includes .variant when present by constructing the key conditionally (e.g., if .variant is set, printf "image.%s.%s.tag is required" .component .variant, otherwise keep the existing format) and call fail with that resulting message; refer to the template variables .component, .variant and the existing fail(printf ...) invocation to locate where to change it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tools/deployment/package-helm/templates/_helpers.tpl`:
- Line 157: The fail message always uses only .component; update the failing
call that currently uses fail (printf "image.%s.tag is required" .component) so
it includes .variant when present by constructing the key conditionally (e.g.,
if .variant is set, printf "image.%s.%s.tag is required" .component .variant,
otherwise keep the existing format) and call fail with that resulting message;
refer to the template variables .component, .variant and the existing
fail(printf ...) invocation to locate where to change it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a885d21c-a819-412c-868d-20d294d2c12b
📒 Files selected for processing (2)
tools/deployment/package-helm/templates/_helpers.tpltools/deployment/package-helm/templates/presto-worker-deployment.yaml
junhaoliao
requested changes
Jun 3, 2026
update
Co-authored-by: Junhao Liao <junhao@junhao.ca>
junhaoliao
approved these changes
Jun 4, 2026
junhaoliao
left a comment
junhaoliao
left a comment
Member
There was a problem hiding this comment.
in my opinion there is no BREAKING behaviourial change in this PR given the same images are used, and the pull policies remain the same as before. We are only exposing the configuration interfaces to the user in the PR.
for the title, how about:
feat(helm): Make all container images configurable in `values.yaml` (resolves #2047).
values.yaml (resolves #2047).
Draft
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Third-party container images (MariaDB, MySQL, MongoDB, RabbitMQ, Redis, kubectl) were hardcoded directly in Helm template files. Only the CLP package image was configurable through values.yaml. This made it impossible to use private registry mirrors or prepare the chart for AWS Marketplace, which requires all image references to be in values.yaml.
Changes:
image.database(withmariadb/mysqlvariants),image.queue,image.redis,image.resultsCacheentries tovalues.yaml. The database image is selected based onclpConfig.database.type.clp.image.refhelper template to accept acomponentparameter (and optionalvariantfor database). TheAppVersionfallback only applies toclpPackage; all other components require an explicit tag.clp.kubectl.image.refhelper (merged into the generalizedclp.image.ref).imagePullPolicyis now configurable per component.Checklist
breaking change.
Validation performed
Summary by CodeRabbit
New Features
Documentation
Chores