feat: add lookup function for pull secrets in helm values template#76
Open
olzemal wants to merge 2 commits into
Open
feat: add lookup function for pull secrets in helm values template#76olzemal wants to merge 2 commits into
olzemal wants to merge 2 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughThe PR adds a default Docker executable, introduces pull-secret JSON parsing and registry resolution for Helm rendering, wires a new CLI flag through rendering, and adds fixtures, tests, and documentation for the new flow. ChangesDocker executable default
Pull-secret templating
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Coverage Report for CI Build 28566523526Warning No base build found for commit Coverage: 43.017%Details
Uncovered Changes
Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
olzemal
force-pushed
the
feature/73-pull-secret-for
branch
3 times, most recently
from
cc780fd to
8209971
Compare
![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: 5
🧹 Nitpick comments (1)
helmvalues/helmvalues_test.go (1)
237-304: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for the exact-ref fallback.
Resolvedocuments a finalp.Get(ref)fallback after the host/path walk, but this table never asserts it. A tagged exact-key case here would lock down that branch and the new resolver contract.Suggested addition
{ + name: "exact full ref falls back to raw key", + ref: "ghcr.io/org/myapp:v1.0.0", + secrets: PullSecrets{"ghcr.io/org/myapp:v1.0.0": "tag-cred"}, + want: "tag-cred", + }, + { name: "full ref matches host only", ref: "ghcr.io/org/myapp:v1.0.0", secrets: PullSecrets{"ghcr.io": "org-cred"}, want: "org-cred", },🤖 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 `@helmvalues/helmvalues_test.go` around lines 237 - 304, Add a test case in TestPullSecretsResolve that covers the exact-ref fallback branch in PullSecrets.Resolve. The current table exercises host and host/path matching, but not the final p.Get(ref) lookup; add a case where the ref itself is present as the secret key and assert that Resolve returns that value. Keep the new case alongside the existing PullSecrets.Resolve scenarios so the fallback contract stays locked down.
🤖 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 `@helmvalues/helmvalues.go`:
- Around line 83-88: The RenderingInput contract is still hardcoded to pull
secrets instead of supporting the generic extra-values extension point. Refactor
RenderingInput and its rendering path to accept arbitrary template inputs (for
example via an Extra/Values-style map) so callers can pass any keys, not just
PullSecrets. Update the Helm values rendering helpers and any CLI wiring that
consumes RenderingInput to merge these generic values into template execution
rather than relying on a bespoke field.
In `@helmvalues/pullsecrets_file_test.go`:
- Around line 75-87: The negative coverage for ParsePullSecretsFile is missing a
contract check for entries with incomplete pull secret objects. Add a new
failing test case in the pullsecrets_file_test suite using ParsePullSecretsFile
with a pullSecrets array containing an item missing or empty
registry/secretName, and assert it returns an error just like the existing
malformed structure case.
In `@helmvalues/pullsecrets_file.go`:
- Around line 26-36: The pull-secrets parser in pullSecretsFile currently
accepts malformed or partially populated entries, which can leave empty values
in the resulting map. Update the decoding logic in the pullSecretsFile
unmarshal/parse path to use a Decoder with DisallowUnknownFields(), then
validate each PullSecret entry has non-empty Registry and SecretName before
adding it to the PullSecrets map; if any entry is invalid, return a parse error
instead of silently proceeding.
In `@test/e2e.sh`:
- Around line 159-160: The e2e check in the OUTPUT5 assertion is too broad
because it only verifies one rendered secret and can miss regressions in the
more-specific lookup paths. Update the test around the OUTPUT5 validation to
assert the expected secret for each lookup form exercised by the fixture in the
script, using the relevant output blocks from the secret rendering logic, or at
minimum verify the expected count of rendered “- name:” entries instead of just
searching for one match.
In `@test/fixtures/arc/pull-secrets-values.yaml.tpl`:
- Around line 6-23: The three `imagePullSecrets` sections in this template
should only render when `pullSecretFor` resolves to a non-empty value. Update
the `apiserver`, `fizzbuzz`, and `helloworld` blocks to wrap each `- name:`
entry in an `if` or `with` guard keyed off the `pullSecretFor` result so
optional registry mappings remain omitted instead of emitting empty names.
---
Nitpick comments:
In `@helmvalues/helmvalues_test.go`:
- Around line 237-304: Add a test case in TestPullSecretsResolve that covers the
exact-ref fallback branch in PullSecrets.Resolve. The current table exercises
host and host/path matching, but not the final p.Get(ref) lookup; add a case
where the ref itself is present as the secret key and assert that Resolve
returns that value. Keep the new case alongside the existing PullSecrets.Resolve
scenarios so the fallback contract stays locked down.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2dcc5d01-bde8-423d-928e-99363561e85f
📒 Files selected for processing (10)
Makefilecmd/ocm-kit/main.gohelmvalues/helmvalues.gohelmvalues/helmvalues_test.gohelmvalues/pullsecrets-schema.jsonhelmvalues/pullsecrets_file.gohelmvalues/pullsecrets_file_test.gotest/e2e.shtest/fixtures/arc/pull-secrets-values.yaml.tpltest/fixtures/arc/pull-secrets.json
olzemal
force-pushed
the
feature/73-pull-secret-for
branch
from
8209971 to
c7148b3
Compare
trevex
reviewed
Jun 29, 2026
Contributor
There was a problem hiding this comment.
Might be a stupid question: Is this file actually used?
Contributor
Author
There was a problem hiding this comment.
Not currently, but package authors could use it to validate their pull-secrets.json file.
| repository: {{ $etcdImage.Host }}/{{ $etcdImage.Repository }} | ||
| tag: {{ $etcdImage.Tag }} | ||
| imagePullSecrets: | ||
| - name: {{ pullSecretFor (printf "%s/%s:%s" $etcdImage.Host $etcdImage.Repository $etcdImage.Tag) }} |
Contributor
There was a problem hiding this comment.
Hmm, if everyone keeps writing (printf "%s/%s:%s" $etcdImage.Host $etcdImage.Repository $etcdImage.Tag) maybe added "ref" to the OCI Image struct makes sense, e.g. $etcdImage.Ref.
Contributor
Author
There was a problem hiding this comment.
This could be a useful feature but is probably beyond the scope of this PR. 90% of use-cases could probably be solved by just using $etcdImage.Host anyways.
olzemal
force-pushed
the
feature/73-pull-secret-for
branch
from
c7148b3 to
549eb79
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@README.md`:
- Around line 213-221: The README example for `pullSecretFor` is too narrow
because it passes only `$image.Host`, which prevents matching repo-scoped OCI
keys. Update the example to pass the full OCI reference from `index
.OCIResources "my-image"` into `pullSecretFor` so it reflects the hierarchical
lookup behavior and correctly demonstrates repo-level secret mappings.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d0b238f7-63d8-4f1e-958e-23c3e427ddba
📒 Files selected for processing (11)
MakefileREADME.mdcmd/ocm-kit/main.gohelmvalues/helmvalues.gohelmvalues/helmvalues_test.gohelmvalues/pullsecrets-schema.jsonhelmvalues/pullsecrets_file.gohelmvalues/pullsecrets_file_test.gotest/e2e.shtest/fixtures/arc/pull-secrets-values.yaml.tpltest/fixtures/arc/pull-secrets.json
✅ Files skipped from review due to trivial changes (4)
- helmvalues/pullsecrets-schema.json
- test/fixtures/arc/pull-secrets.json
- Makefile
- test/fixtures/arc/pull-secrets-values.yaml.tpl
🚧 Files skipped from review as they are similar to previous changes (6)
- helmvalues/pullsecrets_file_test.go
- test/e2e.sh
- cmd/ocm-kit/main.go
- helmvalues/helmvalues_test.go
- helmvalues/pullsecrets_file.go
- helmvalues/helmvalues.go
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
3 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.
What
Closes #73
Checklist
Summary by CodeRabbit
--pull-secrets-fileto load pull-secret mappings from a JSON file and inject them into Helm rendering.e2e-stop-zotworkflow (still overrideable).ocm-kitCLI and added “Pull Secrets” documentation, including template usage and configuration format.