Conditionally set backendImages credentials#1024
Open
cypres wants to merge 2 commits into
Open
Conversation
For deployments that only use public images, and forego a key for NGC, it was difficuly to avoid image pull secrets.
Even if a secret was not specified the config map would still contain
```
apiVersion: v1
data:
config.yaml: |
workflow:
backend_images:
credential:
secretKey: .dockerconfigjson
secretName: nvcr-secret
```
With the default values it was not easy to remove that configuration due
to how Helm patches values.
You would need to first set credential: null to delete the inherited map, then a second later file sets credential: {}
Instead of changing the default values, which would be a breaking change, add a template that only adds the credential structure when needed.
Signed-off-by: Hans Arnholm <harnholm@nvidia.com>
Contributor
📝 WalkthroughWalkthroughThis PR extends Helm chart deployment infrastructure to conditionally render secrets. It adds Bazel build targets to expose deployment values, introduces a helper function to determine when secrets should be rendered based on configuration and Kubernetes state, and applies that helper to filter workflow backend image credentials in the config template. ChangesConditional Secret Rendering in Helm Charts
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Trivy (0.69.3)Trivy execution failed: 2026-05-16T23:12:16Z FATAL Fatal error run error: fs scan error: scan error: scan failed: failed analysis: post analysis error: post analysis error: cloudformation scan error: fs filter error: fs filter error: walk error range error: stat smartylint.json: no such file or directory: range error: stat smartylint.json: no such file or directory Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 1
🧹 Nitpick comments (1)
deployments/BUILD (1)
19-23: ⚡ Quick winConsider excluding temporary and hidden files from the glob.
The glob pattern includes all files under
values/without exclusions, which may inadvertently capture editor backups, temporary files, or hidden system files (e.g.,.swp,*~,.DS_Store). This can lead to build cache pollution and unnecessary rebuilds.🧹 Proposed exclusions
filegroup( name = "values", - srcs = glob(["values/**"]), + srcs = glob( + ["values/**"], + exclude = [ + "**/*~", + "**/*.swp", + "**/*.bak", + "**/.DS_Store", + ], + ), visibility = ["//visibility:public"], )🤖 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 `@deployments/BUILD` around lines 19 - 23, The filegroup named "values" uses srcs = glob(["values/**"]) which currently captures hidden and temp files; update the glob call in the filegroup to add an exclude list that omits common editor and system artifacts (e.g., patterns for *.swp, *~, .DS_Store and hidden dotfiles) so only intended files are included; modify the srcs = glob(...) invocation to pass an excludes=[...] argument with patterns for editor backups and hidden files to prevent build cache pollution.
🤖 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 `@deployments/charts/service/templates/configs.yaml`:
- Around line 42-44: When the secret-ref check returns "false", remove the
credential key instead of assigning an empty dict; replace the line that does
'{{- $_ := set $backendImages "credential" dict }}' with a call that deletes the
key (e.g. use the Sprig unset function: '{{- $_ := unset $backendImages
"credential" }}'), then keep the subsequent '{{- $_ := set $workflow
"backend_images" $backendImages }}' to persist the updated map so credential is
absent from the emitted config.yaml.
---
Nitpick comments:
In `@deployments/BUILD`:
- Around line 19-23: The filegroup named "values" uses srcs =
glob(["values/**"]) which currently captures hidden and temp files; update the
glob call in the filegroup to add an exclude list that omits common editor and
system artifacts (e.g., patterns for *.swp, *~, .DS_Store and hidden dotfiles)
so only intended files are included; modify the srcs = glob(...) invocation to
pass an excludes=[...] argument with patterns for editor backups and hidden
files to prevent build cache pollution.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e173ab87-eeaf-4615-beff-692969820db1
📒 Files selected for processing (4)
deployments/BUILDdeployments/charts/BUILDdeployments/charts/service/templates/_helpers.tpldeployments/charts/service/templates/configs.yaml
The _helpers.tpl `osmo.config-secret-ref-enabled` gate is the load- bearing fix: it prevents `secret-nvcr-secret` volume + volumeMount from rendering when nvcr-secret isn't the imagePullSecret and the secret doesn't exist. That alone fixes the FailedMount + pod-start failure mode this PR targets. The configs.yaml `deepCopy`+`set` logic was a secondary measure stripping the rendered backend_images.credential block in the same case. Without it, the OSMO config loader inside the pod sees the credential reference, attempts to read /etc/osmo/secrets/<name>/ .dockerconfigjson, gets an OSError because the volume was skipped, logs it, and proceeds with an empty RegistryCredential. Pod-spec generator then skips creating an OSMO-side docker auth (checks osmo_cred.registry / .username / .auth — all empty), and workflow pods pull anonymously. Functionally equivalent to the strip; the only delta is one log line per service. Tradeoffs in keeping the strip: - `lookup` only works at install time, not `helm template` / dry- run, so the rendered output differs by render mode. - The deepCopy + dict manipulation is verbose helm-template logic to maintain. - The skill is the canonical entry point for minimal deploys; it already gates `--set backend_images.credential.*` on the caller opting into NGC plumbing (vpan/microk8s-public-image-fixes), so the configs.yaml strip is unreachable from that path. Keep the _helpers.tpl gate, drop the configs.yaml strip. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
vvnpn-nv
approved these changes
May 18, 2026
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
For deployments that only use public images, and forego a key for NGC, it was difficuly to avoid image pull secrets.
Even if a secret was not specified the config map would still contain
With the default values it was not easy to remove that configuration due to how Helm patches values.
You would need to first set credential: null to delete the inherited map, then a second later file sets credential: {}
Instead of changing the default values, which would be a breaking change, add a template that only adds the credential structure when needed.
Issue #None
Checklist
Summary by CodeRabbit
Release Notes
New Features
Tests