fix: frontend load global extraENV & setup script #414
Conversation
|
""" WalkthroughThe changes update the Helm chart for the Changes
Sequence Diagram(s)sequenceDiagram
participant Helm as Helm Chart
participant K8s as Kubernetes
participant Frontend as Frontend Pod
Helm->>K8s: Deploy operator-wandb with updated values and ConfigMap volume
K8s->>Frontend: Create frontend pod with envFrom referencing global secret, OPERATOR_ENABLED=true, and mount setup script volume
Frontend->>Frontend: Run `02_patch_env_js.sh` to patch env.js with server flags and additional environment variables
Possibly related PRs
Suggested reviewers
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
charts/operator-wandb/templates/frontend.yaml (2)
5-6: Fix incorrect indentation for Helm label include
The{{- include "wandb.commonLabels" . | nindent 4 }}line is over-indented underlabels:, causing YAML syntax errors (wrong indentationandexpected <block end>). It should align directly underlabels:without extra spaces.Proposed diff:
labels: - {{- include "wandb.commonLabels" . | nindent 4 }} +{{- include "wandb.commonLabels" . | nindent 4 }}🧰 Tools
🪛 YAMLlint (1.37.1)
[warning] 6-6: wrong indentation: expected 4 but found 6
(indentation)
31-33: Use parameter expansion to strip the trailing comma inextract_server_flags
The current approach withlocal length="${#new_js}"-1and${new_js:0:length}won’t evaluate the arithmetic as intended. Switch to bash’s${var%,}expansion for reliable comma removal.Proposed diff:
- # remove the trailing comma and close the object - local length="${#new_js}"-1 - printf "${new_js:0:length}};\n" >> "$env_js_path" + # remove trailing comma and append closing brace + local trimmed="${new_js%,}" + printf '%s};\n' "$trimmed" >> "$env_js_path"
🧹 Nitpick comments (2)
charts/operator-wandb/templates/frontend.yaml (2)
8-17: Ensure robust JSON key/value quoting inappend_env_js
The function correctly checks writability and appends assignments, but values aren’t quoted in the JS object. If any$valuecontains special characters, the resulting JS could be invalid.You might wrap both key and value in quotes:
- printf "Object.assign(window.CONFIG, {%s: %s});\n" "$key" "$value" >> "$env_js_path" + printf 'Object.assign(window.CONFIG, {"%s":"%s"});\n' "$key" "$value" >> "$env_js_path"
37-72: Consistent bash conditional syntax inknown_additional_flags
This function mixes single‐bracket[...]and double‐bracket[[...]]tests, as well as=and==operators. For readability and fewer surprises, prefer[[ ... ]]with==throughout.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
charts/operator-wandb/templates/frontend.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
charts/operator-wandb/templates/frontend.yaml
[warning] 6-6: wrong indentation: expected 4 but found 6
(indentation)
[error] 4-4: syntax error: expected , but found ''
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms (24)
- GitHub Check: Test Chart (v1.29.14, weave-trace)
- GitHub Check: Test Chart (v1.29.14, runs-v2-bufstream)
- GitHub Check: Test Chart (v1.29.14, separate-pods)
- GitHub Check: Test Chart (v1.29.14, user-defined-secrets)
- GitHub Check: Test Chart (v1.29.14, executor-enabled)
- GitHub Check: Test Chart (v1.29.14, default)
- GitHub Check: Test Chart (v1.30.10, weave-trace)
- GitHub Check: Test Chart (v1.30.10, runs-v2-bufstream)
- GitHub Check: Test Chart (v1.30.10, separate-pods)
- GitHub Check: Test Chart (v1.30.10, user-defined-secrets)
- GitHub Check: Test Chart (v1.30.10, executor-enabled)
- GitHub Check: Test Chart (v1.30.10, default)
- GitHub Check: Test Chart (v1.31.6, weave-trace)
- GitHub Check: Test Chart (v1.31.6, runs-v2-bufstream)
- GitHub Check: Test Chart (v1.31.6, separate-pods)
- GitHub Check: Test Chart (v1.31.6, user-defined-secrets)
- GitHub Check: Test Chart (v1.31.6, executor-enabled)
- GitHub Check: Test Chart (v1.31.6, default)
- GitHub Check: Test Chart (v1.32.2, weave-trace)
- GitHub Check: Test Chart (v1.32.2, runs-v2-bufstream)
- GitHub Check: Test Chart (v1.32.2, separate-pods)
- GitHub Check: Test Chart (v1.32.2, user-defined-secrets)
- GitHub Check: Test Chart (v1.32.2, executor-enabled)
- GitHub Check: Test Chart (v1.32.2, default)
🔇 Additional comments (2)
charts/operator-wandb/templates/frontend.yaml (2)
1-7: Approve ConfigMap scaffold (except label indentation)
The newConfigMapresource and its metadata fields (apiVersion,kind,name) are correct and align with the requirement to inject a setup script into the frontend build.🧰 Tools
🪛 YAMLlint (1.37.1)
[warning] 6-6: wrong indentation: expected 4 but found 6
(indentation)
[error] 4-4: syntax error: expected , but found ''
(syntax)
74-83: Approve main execution guard
Themainfunction correctly ensures idempotent patching ofenv.jsby checking forconst SERVER_FLAGS =. The overall control flow matches the intended behavior.
dacbd
changed the title
Summary by CodeRabbit
New Features
Chores