feat: Add image tag to environment #527
Draft
Conversation
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
robertquitt
temporarily deployed
to
Helm Charts
GitHub Actions
Inactive
robertquitt
had a problem deploying
to
Helm Charts
GitHub Actions
Failure
robertquitt
temporarily deployed
to
Helm Charts
GitHub Actions
Inactive
robertquitt
temporarily deployed
to
Helm Charts
GitHub Actions
Inactive
robertquitt
had a problem deploying
to
Helm Charts
GitHub Actions
Failure
robertquitt
temporarily deployed
to
Helm Charts
GitHub Actions
Inactive
robertquitt
temporarily deployed
to
Helm Charts
GitHub Actions
Inactive
robertquitt
temporarily deployed
to
Helm Charts
GitHub Actions
Inactive
robertquitt
temporarily deployed
to
Helm Charts
GitHub Actions
Inactive
robertquitt
temporarily deployed
to
Helm Charts
GitHub Actions
Inactive
robertquitt
had a problem deploying
to
Helm Charts
GitHub Actions
Failure
robertquitt
temporarily deployed
to
Helm Charts
GitHub Actions
Inactive
robertquitt
temporarily deployed
to
Helm Charts
GitHub Actions
Inactive
robertquitt
had a problem deploying
to
Helm Charts
GitHub Actions
Failure
robertquitt
temporarily deployed
to
Helm Charts
GitHub Actions
Inactive
robertquitt
had a problem deploying
to
Helm Charts
GitHub Actions
Failure
robertquitt
had a problem deploying
to
Helm Charts
GitHub Actions
Failure
robertquitt
temporarily deployed
to
Helm Charts
GitHub Actions
Inactive
robertquitt
temporarily deployed
to
Helm Charts
GitHub Actions
Inactive
robertquitt
had a problem deploying
to
Helm Charts
GitHub Actions
Failure
robertquitt
temporarily deployed
to
Helm Charts
GitHub Actions
Inactive
robertquitt
had a problem deploying
to
Helm Charts
GitHub Actions
Failure
robertquitt
temporarily deployed
to
Helm Charts
GitHub Actions
Inactive
robertquitt
temporarily deployed
to
Helm Charts
GitHub Actions
Inactive
robertquitt
temporarily deployed
to
Helm Charts
GitHub Actions
Inactive
robertquitt
temporarily deployed
to
Helm Charts
GitHub Actions
Inactive
robertquitt
temporarily deployed
to
Helm Charts
GitHub Actions
Inactive
robertquitt
temporarily deployed
to
Helm Charts
GitHub Actions
Inactive
Contributor
WalkthroughThis PR updates the operator-wandb Helm chart: bumps chart version, injects GORILLA_IMAGE_TAG into multiple ConfigMaps, adds new metric-observer and filemeta templates, tweaks a small template formatting, updates values.envFrom references, and adds a dedicated test-config for local deployments. (39 words) Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
robertquitt
force-pushed
the
rquitt/add-server-version
branch
from
3cb28f4 to
7c1fce0
Compare
robertquitt
had a problem deploying
to
Helm Charts
GitHub Actions
Failure
robertquitt
had a problem deploying
to
Helm Charts
GitHub Actions
Failure
robertquitt
had a problem deploying
to
Helm Charts
GitHub Actions
Failure
robertquitt
had a problem deploying
to
Helm Charts
GitHub Actions
Failure
robertquitt
had a problem deploying
to
Helm Charts
GitHub Actions
Failure
robertquitt
had a problem deploying
to
Helm Charts
GitHub Actions
Failure
robertquitt
had a problem deploying
to
Helm Charts
GitHub Actions
Failure
robertquitt
had a problem deploying
to
Helm Charts
GitHub Actions
Failure
robertquitt
had a problem deploying
to
Helm Charts
GitHub Actions
Failure
robertquitt
had a problem deploying
to
Helm Charts
GitHub Actions
Failure
robertquitt
had a problem deploying
to
Helm Charts
GitHub Actions
Failure
robertquitt
had a problem deploying
to
Helm Charts
GitHub Actions
Failure
robertquitt
had a problem deploying
to
Helm Charts
GitHub Actions
Failure
robertquitt
had a problem deploying
to
Helm Charts
GitHub Actions
Failure
robertquitt
had a problem deploying
to
Helm Charts
GitHub Actions
Failure
robertquitt
had a problem deploying
to
Helm Charts
GitHub Actions
Failure
robertquitt
had a problem deploying
to
Helm Charts
GitHub Actions
Failure
robertquitt
had a problem deploying
to
Helm Charts
GitHub Actions
Failure
robertquitt
had a problem deploying
to
Helm Charts
GitHub Actions
Failure
robertquitt
had a problem deploying
to
Helm Charts
GitHub Actions
Failure
robertquitt
had a problem deploying
to
Helm Charts
GitHub Actions
Failure
robertquitt
had a problem deploying
to
Helm Charts
GitHub Actions
Failure
robertquitt
had a problem deploying
to
Helm Charts
GitHub Actions
Failure
robertquitt
had a problem deploying
to
Helm Charts
GitHub Actions
Failure
robertquitt
had a problem deploying
to
Helm Charts
GitHub Actions
Failure
robertquitt
had a problem deploying
to
Helm Charts
GitHub Actions
Failure
![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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (16)
test-configs/operator-wandb/__snapshots__/dedicated.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/default.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/fmb.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/oidc-secret-from-k8s.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/runs-v2-bufstream.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/separate-pods.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/snap-enable-backfill.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/snap-image-digest-override.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/snap-image-tag-override.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/snap-oidc-settings.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/snap-priority-classes.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/snap-tolerations-and-selectors.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/url-encoded-password.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/user-defined-secrets.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/weave-trace-with-worker.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/weave-trace.snapis excluded by!**/*.snap
📒 Files selected for processing (12)
charts/operator-wandb/Chart.yaml(1 hunks)charts/operator-wandb/templates/_env.tpl(2 hunks)charts/operator-wandb/templates/api.yaml(1 hunks)charts/operator-wandb/templates/executor.yaml(1 hunks)charts/operator-wandb/templates/filemeta.yaml(1 hunks)charts/operator-wandb/templates/filestream.yaml(1 hunks)charts/operator-wandb/templates/flat-run-fields-updater.yaml(1 hunks)charts/operator-wandb/templates/glue.yaml(1 hunks)charts/operator-wandb/templates/metric-observer.yaml(1 hunks)charts/operator-wandb/templates/parquet.yaml(1 hunks)charts/operator-wandb/values.yaml(2 hunks)test-configs/operator-wandb/dedicated.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- charts/operator-wandb/templates/filemeta.yaml
- charts/operator-wandb/templates/executor.yaml
- charts/operator-wandb/templates/metric-observer.yaml
- charts/operator-wandb/values.yaml
- charts/operator-wandb/templates/_env.tpl
- charts/operator-wandb/templates/parquet.yaml
- charts/operator-wandb/templates/glue.yaml
- charts/operator-wandb/Chart.yaml
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: zacharyblasczyk
Repo: wandb/helm-charts PR: 0
File: :0-0
Timestamp: 2025-02-19T00:26:45.489Z
Learning: In PR #351 of helm-charts repository, a template syntax error was fixed by removing an unclosed `with` block in the imagePullSecrets section of the deployment template.
📚 Learning: 2025-02-19T00:26:45.489Z
Learnt from: zacharyblasczyk
Repo: wandb/helm-charts PR: 0
File: :0-0
Timestamp: 2025-02-19T00:26:45.489Z
Learning: In PR #351 of helm-charts repository, a template syntax error was fixed by removing an unclosed `with` block in the imagePullSecrets section of the deployment template.
Applied to files:
charts/operator-wandb/templates/api.yamlcharts/operator-wandb/templates/filestream.yaml
📚 Learning: 2025-02-19T00:26:45.489Z
Learnt from: zacharyblasczyk
Repo: wandb/helm-charts PR: 0
File: :0-0
Timestamp: 2025-02-19T00:26:45.489Z
Learning: In Helm templates, when using include functions, proper template structure must be maintained. In PR #351 of helm-charts repository, a template syntax error was fixed by removing an incorrectly structured imagePullSecrets section that was using the include function without proper template formatting.
Applied to files:
charts/operator-wandb/templates/filestream.yaml
📚 Learning: 2025-09-04T13:54:32.322Z
Learnt from: zacharyblasczyk
Repo: wandb/helm-charts PR: 479
File: charts/operator-wandb/values.yaml:776-776
Timestamp: 2025-09-04T13:54:32.322Z
Learning: When analyzing Helm templates in the wandb/helm-charts project, always verify the execution context of named templates by checking where and how they are called, as this determines the correct `.Values` path structure.
Applied to files:
charts/operator-wandb/templates/filestream.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (27)
- GitHub Check: Test Chart (v1.31.6, user-defined-secrets)
- GitHub Check: Test Chart (v1.30.10, fmb)
- GitHub Check: Test Chart (v1.30.10, weave-trace-with-worker)
- GitHub Check: Test Chart (v1.31.6, weave-trace)
- GitHub Check: Test Chart (v1.30.10, weave-trace)
- GitHub Check: Test Chart (v1.30.10, url-encoded-password)
- GitHub Check: Test Chart (v1.30.10, user-defined-secrets)
- GitHub Check: Test Chart (v1.30.10, oidc-secret-from-k8s)
- GitHub Check: Test Chart (v1.30.10, runs-v2-bufstream)
- GitHub Check: Test Chart (v1.30.10, separate-pods)
- GitHub Check: Test Chart (v1.31.6, weave-trace-with-worker)
- GitHub Check: Test Chart (v1.30.10, default)
- GitHub Check: Test Chart (v1.31.6, url-encoded-password)
- GitHub Check: Test Chart (v1.31.6, separate-pods)
- GitHub Check: Test Chart (v1.32.2, runs-v2-bufstream)
- GitHub Check: Test Chart (v1.31.6, default)
- GitHub Check: Test Chart (v1.31.6, fmb)
- GitHub Check: Test Chart (v1.31.6, oidc-secret-from-k8s)
- GitHub Check: Test Chart (v1.32.2, oidc-secret-from-k8s)
- GitHub Check: Test Chart (v1.31.6, runs-v2-bufstream)
- GitHub Check: Test Chart (v1.32.2, weave-trace-with-worker)
- GitHub Check: Test Chart (v1.32.2, default)
- GitHub Check: Test Chart (v1.32.2, separate-pods)
- GitHub Check: Test Chart (v1.32.2, url-encoded-password)
- GitHub Check: Test Chart (v1.32.2, weave-trace)
- GitHub Check: Test Chart (v1.32.2, user-defined-secrets)
- GitHub Check: Test Chart (v1.32.2, fmb)
🔇 Additional comments (3)
charts/operator-wandb/templates/api.yaml (1)
41-41: ✓ GORILLA_IMAGE_TAG addition looks good.The template syntax is correct and the value is properly defined in the test configuration.
charts/operator-wandb/templates/flat-run-fields-updater.yaml (1)
13-13: ✓ Correct use ofindexfunction for hyphenated key access.The
indexfunction properly handles the hyphenated key name. Value is defined in the test config.charts/operator-wandb/templates/filestream.yaml (1)
8-8:filestream.image.tagis already defined in values.yaml with a sensible default.The value exists at line 1129 of the main values.yaml with default
tag: latestunder thefilestream.imagesection. The ConfigMap template correctly references this value.
Comment on lines
+56
to
+81
| executor: | ||
| install: true | ||
|
|
||
| filemeta: | ||
| install: true | ||
|
|
||
| frontend: | ||
| install: true | ||
|
|
||
| flat-run-fields-updater: | ||
| install: true | ||
| pubSub: | ||
| subscription: "flat-run-fields-updater" | ||
| image: | ||
| tag: "0.75.2" | ||
|
|
||
| filestream: | ||
| install: true | ||
| pubSub: | ||
| subscription: "filestream-gorilla" | ||
|
|
||
| metric-observer: | ||
| install: true | ||
| pubSub: | ||
| subscription: "metric-observer" | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== 1. Checking test config file structure ==="
cat -n test-configs/operator-wandb/dedicated.yaml | sed -n '50,85p'
echo ""
echo "=== 2. Searching for GORILLA_IMAGE_TAG in templates ==="
rg "GORILLA_IMAGE_TAG" charts/operator-wandb/templates/ -A 2 -B 2 --type yaml
echo ""
echo "=== 3. Checking main values.yaml for image defaults ==="
rg "executor:|filemeta:|filestream:|metric-observer:" charts/operator-wandb/values.yaml -A 10 --type yaml | head -80
echo ""
echo "=== 4. Checking if api has image.tag in test config ==="
rg -A 5 "^api:" test-configs/operator-wandb/dedicated.yaml --type yamlRepository: wandb/helm-charts
Length of output: 5712
Define missing image.tag values for installed components.
Templates explicitly reference GORILLA_IMAGE_TAG for executor, filemeta, filestream, and metric-observer (via {{ .Values.executor.image.tag }}, etc.). The test configuration marks these components for installation but omits image.tag definitions, while api and flat-run-fields-updater correctly provide them. This will cause template rendering failures or undefined values in the generated ConfigMaps.
Add image.tag for:
executor(line 56)filemeta(line 59)filestream(line 72)metric-observer(line 77)
🤖 Prompt for AI Agents
In test-configs/operator-wandb/dedicated.yaml around lines 56 to 81 the
installer flags for executor, filemeta, filestream and metric-observer are true
but their image.tag values are missing which causes templates referencing {{
.Values.<component>.image.tag }} to fail; add an image.tag entry under each of
these components (executor at ~line 56, filemeta ~line 59, filestream ~line 72,
metric-observer ~line 77) with the appropriate GORILLA image tag (e.g., match
the existing tags used elsewhere like "0.75.2" or the intended release) so the
templates resolve correctly.
Contributor
Author
There was a problem hiding this comment.
Should I add a check for non-null image.tag? I imagine the tag should always be present in the real config, right?
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== 1. Checking main values.yaml for image.tag defaults for affected components ==="
rg -A 15 "^executor:" charts/operator-wandb/values.yaml --type yaml
echo ""
rg -A 15 "^filemeta:" charts/operator-wandb/values.yaml --type yaml
echo ""
rg -A 15 "^filestream:" charts/operator-wandb/values.yaml --type yaml
echo ""
rg -A 15 "^metric-observer:" charts/operator-wandb/values.yaml --type yaml
echo ""
echo "=== 2. Checking for existing validation patterns in templates ==="
rg "required|fail" charts/operator-wandb/templates/ --type yaml -C 2 | head -40
echo ""
echo "=== 3. Checking how api.image.tag is defined and used ==="
rg -A 10 "^api:" charts/operator-wandb/values.yaml --type yaml | head -20
cat charts/operator-wandb/templates/api.yaml | grep -A 2 -B 2 "GORILLA_IMAGE_TAG"
echo ""
echo "=== 4. Looking for validation helpers or _helpers.tpl patterns ==="
fd -e tpl -e yaml . charts/operator-wandb/templates/ --exec basename {} | sort | uniq | head -10
Length of output: 3621
@robertquitt Yes, adding validation would be a good idea! Based on the codebase analysis:
Current inconsistent state:
filemetaandmetric-observerhavetag: latestdefaults in main values.yamlexecutorandfilestreamhave no image section at all in main values.yaml- Templates directly reference
{{ .Values.<component>.image.tag }}without any validation
Recommendations:
- Add missing defaults to main values.yaml for consistency:
executor:
install: false
image:
repository: wandb/megabinary
tag: latest
# ... rest of config
filestream:
install: false
image:
repository: wandb/megabinary
tag: latest
# ... rest of config- Add validation in templates to fail-fast when a component is installed but tag is missing. Example pattern:
{{- if .Values.executor.install }}
{{- if not .Values.executor.image.tag }}
{{- fail "executor.image.tag is required when executor.install is true" }}
{{- end }}
# ... rest of template
{{- end }}- Why this matters for your PR: Since you're adding
GORILLA_IMAGE_TAGto track deployment versions, having explicit tags (not "latest") in production is critical. The validation ensures users are aware they need to set it.
The validation approach is cleaner than relying on defaults, especially since "latest" isn't appropriate for production tracking. It makes the requirement explicit rather than silently passing an unhelpful default value.
Would you like me to help draft the validation checks for all four components?
🧠 Learnings used
Learnt from: zacharyblasczyk
Repo: wandb/helm-charts PR: 479
File: charts/operator-wandb/values.yaml:776-776
Timestamp: 2025-09-04T13:54:32.322Z
Learning: The `wandb.executor.taskQueue` template in charts/operator-wandb/templates/_redis.tpl gets executed in the base-chart context, so values are accessed at `.Values.workerConcurrency` rather than `.Values.executor.workerConcurrency`.
Learnt from: zacharyblasczyk
Repo: wandb/helm-charts PR: 0
File: :0-0
Timestamp: 2025-02-19T00:26:45.489Z
Learning: In PR #351 of helm-charts repository, a template syntax error was fixed by removing an unclosed `with` block in the imagePullSecrets section of the deployment template.
Learnt from: zacharyblasczyk
Repo: wandb/helm-charts PR: 0
File: :0-0
Timestamp: 2025-02-19T00:26:45.489Z
Learning: In Helm templates, when using include functions, proper template structure must be maintained. In PR #351 of helm-charts repository, a template syntax error was fixed by removing an incorrectly structured imagePullSecrets section that was using the include function without proper template formatting.
Contributor
Author
|
Back to draft, going to try out stamping the version into the image instead |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
We want the images to have an idea of what release version has been deployed. The image tag should be a reasonable proxy for that, I think. If there's a better way to do this, please let me know
Summary by CodeRabbit
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.