feat: typed credentials for Helm binding (Phase 2)#2408
Conversation
✅ Deploy Preview for ocm-website ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
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:
📝 WalkthroughWalkthroughReplaces untyped map-based Helm HTTP credentials and legacy consumer identity usage with typed ChangesTyped Helm Credentials & Identity Migration
Sequence DiagramsequenceDiagram
participant Consumer as Consumer
participant Input as Input Layer
participant Resolver as Credential Resolver
participant Conv as Transformation
participant Download as Download Layer
participant TLS as TLS/Auth
Consumer->>Input: Request resource with legacy credentials (map)
Input->>Conv: FromDirectCredentials(map) → *HelmHTTPCredentials
Input->>Resolver: ResolveTyped(consumerIdentity)
Resolver-->>Input: runtime.Typed credentials (HelmHTTPCredentials / DirectCredentials)
Conv->>Download: WithCredentials(*HelmHTTPCredentials)
Download->>Download: read Credentials.Username / Password / Keyring / CertFile / KeyFile
Download->>TLS: constructTLSOptions(CertFile, KeyFile)
TLS-->>Download: TLS config
Download-->>Input: fetched chart blob
Input-->>Consumer: returned Helm blob
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bindings/go/helm/internal/download/options.go (1)
94-99:⚠️ Potential issue | 🟡 MinorUpdate outdated documentation example in
doc.go.The
doc.gofile shows an example using the legacymap[string]stringsignature forWithCredentials, which no longer matches the current implementation. The documentation at lines 21-28 should be updated to reflect thatWithCredentialsnow requires*helmcredsv1.HelmHTTPCredentials(typically created viahelmcredsv1.FromDirectCredentials(...)).The actual codebase call site in
resource_repository.go:104is correctly updated to the typed credentials. Update only the documentation example to be current.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindings/go/helm/internal/download/options.go` around lines 94 - 99, The doc example in doc.go is outdated: update the example that calls WithCredentials to pass a *helmcredsv1.HelmHTTPCredentials instance (created via helmcredsv1.FromDirectCredentials(...) or equivalent) instead of a legacy map[string]string; locate the WithCredentials usage in the documentation and replace the map literal with code that constructs typed credentials (referencing WithCredentials and helmcredsv1.FromDirectCredentials to mirror the implementation used in resource_repository.go).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bindings/go/helm/internal/download/tls.go`:
- Around line 75-89: The cert/key existence checks in the TLS setup (when
opt.Credentials is used) currently treat any os.Stat error as "does not exist";
update the blocks that handle opt.Credentials.CertFile and
opt.Credentials.KeyFile to distinguish not-found from other errors by using
os.IsNotExist(err): if os.IsNotExist(err) return a clear "does not exist" error
for certFile/keyFile, otherwise wrap and return the original os.Stat error
(e.g., using fmt.Errorf with %w) so permission/IO errors are preserved and
surfaced.
---
Outside diff comments:
In `@bindings/go/helm/internal/download/options.go`:
- Around line 94-99: The doc example in doc.go is outdated: update the example
that calls WithCredentials to pass a *helmcredsv1.HelmHTTPCredentials instance
(created via helmcredsv1.FromDirectCredentials(...) or equivalent) instead of a
legacy map[string]string; locate the WithCredentials usage in the documentation
and replace the map literal with code that constructs typed credentials
(referencing WithCredentials and helmcredsv1.FromDirectCredentials to mirror the
implementation used in resource_repository.go).
🪄 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: CHILL
Plan: Pro
Run ID: 257abb92-23e2-4d77-9d02-5b8e32eac157
⛔ Files ignored due to path filters (1)
bindings/go/helm/go.sumis excluded by!**/*.sum
📒 Files selected for processing (20)
bindings/go/helm/go.modbindings/go/helm/input/blob.gobindings/go/helm/internal/download/download.gobindings/go/helm/internal/download/download_test.gobindings/go/helm/internal/download/options.gobindings/go/helm/internal/download/tls.gobindings/go/helm/repository/resource/resource_repository.gobindings/go/helm/spec/credentials/identity/v1/register.gobindings/go/helm/spec/credentials/identity/v1/schemas/HelmChartRepositoryIdentity.schema.jsonbindings/go/helm/spec/credentials/identity/v1/type.gobindings/go/helm/spec/credentials/identity/v1/type_test.gobindings/go/helm/spec/credentials/identity/v1/zz_generated.deepcopy.gobindings/go/helm/spec/credentials/identity/v1/zz_generated.ocm_jsonschema.gobindings/go/helm/spec/credentials/identity/v1/zz_generated.ocm_type.gobindings/go/helm/spec/credentials/v1/helm_credentials.gobindings/go/helm/spec/credentials/v1/helm_credentials_test.gobindings/go/helm/spec/credentials/v1/schemas/HelmHTTPCredentials.schema.jsonbindings/go/helm/spec/credentials/v1/zz_generated.deepcopy.gobindings/go/helm/spec/credentials/v1/zz_generated.ocm_jsonschema.gobindings/go/helm/spec/credentials/v1/zz_generated.ocm_type.go
927b18d to
30604e3
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
bindings/go/helm/spec/identity/v1/register.go (1)
1-11: Add a parent-packagescheme.gowithinit()for auto-registration to align with the established pattern inaccessandinputpackages.The
accessandinputpackages in the Helm spec tree both expose a package-levelSchemevariable and auto-register their types ininit(). Theidentitypackage only exposesMustRegisterIdentityType()without a parentscheme.gothat calls it on import. This divergence means callers must explicitly invoke the function or risk their scheme lacking the identity type, which can silently break typed credential resolution at runtime.✨ Suggested `scheme.go` (analogous to
spec/access/scheme.go)+// bindings/go/helm/spec/identity/scheme.go +package identity + +import ( + v1 "ocm.software/open-component-model/bindings/go/helm/spec/identity/v1" + "ocm.software/open-component-model/bindings/go/runtime" +) + +var Scheme = runtime.NewScheme() + +func init() { + v1.MustRegisterIdentityType(Scheme) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindings/go/helm/spec/identity/v1/register.go` around lines 1 - 11, The identity package currently only exposes MustRegisterIdentityType and lacks a package-level auto-registration like access/input; add a new parent package file (e.g., scheme.go) in the identity package that declares a package-level Scheme variable and an init() which calls MustRegisterIdentityType(Scheme) so HelmChartRepositoryIdentity (and its VersionedType/Type aliases) are auto-registered on import; ensure the variable name and init pattern match the established pattern used by spec/access and spec/input.bindings/go/helm/transformation/credentials_test.go (1)
93-117: ⚡ Quick winMissing test case:
ResolveTypedreturning(nil, nil).The test suite doesn't cover the path where
ResolveTypedsucceeds with aniltyped value and no error — the exact gap described in theresolveCredentialsMapreview comment above. Adding this case would document and lock in the expectednil, nilreturn.🧪 Suggested additional test
func TestResolveCredentialsMap_NilTyped(t *testing.T) { resolver := &stubResolver{typed: nil, err: nil} creds, err := resolveCredentialsMap(t.Context(), resolver, runtime.Identity{"type": "HelmChartRepository"}) require.NoError(t, err) assert.Nil(t, creds, "nil typed with no error should be treated as no credentials") }This test will currently fail (revealing the missing nil guard in
resolveCredentialsMap) and pass once thecase nil:arm is added.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindings/go/helm/transformation/credentials_test.go` around lines 93 - 117, Add a unit test covering the path where ResolveTyped returns (nil, nil): create a stubResolver with typed: nil and err: nil, call resolveCredentialsMap(t.Context(), resolver, runtime.Identity{"type":"HelmChartRepository"}), and assert no error and creds == nil; this documents and locks in the expected behavior for resolveCredentialsMap when the resolver returns a nil typed value and no error.bindings/go/helm/input/method.go (1)
12-12: 💤 Low valueMisleading import alias:
v2for a/v1package.Every other file in this PR imports
bindings/go/helm/spec/credentials/v1ashelmcredsv1(e.g.,digest.go,blob.go). Usingv2here contradicts both the package path and the rest of the codebase.♻️ Proposed fix
- v2 "ocm.software/open-component-model/bindings/go/helm/spec/credentials/v1" + helmcredsv1 "ocm.software/open-component-model/bindings/go/helm/spec/credentials/v1"And update the usage on line 84:
- helmBlob, chart, err := GetV1HelmBlob(ctx, helm, i.TempFolder, WithCredentials(v2.FromDirectCredentials(credentials))) + helmBlob, chart, err := GetV1HelmBlob(ctx, helm, i.TempFolder, WithCredentials(helmcredsv1.FromDirectCredentials(credentials)))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindings/go/helm/input/method.go` at line 12, The import alias "v2" for the package "ocm.software/open-component-model/bindings/go/helm/spec/credentials/v1" is misleading and inconsistent; change the import alias to helmcredsv1 and update all uses in this file from v2.<TypeOrConst> to helmcredsv1.<TypeOrConst> (e.g., replace any v2.Something references near the method handling credentials in method.go with helmcredsv1.Something) so the alias matches the package path and the rest of the codebase.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bindings/go/helm/transformation/credentials.go`:
- Around line 27-53: The switch over the variable named "typed" should first
guard against a nil value returned from ResolveTyped: if typed == nil return
nil, nil to avoid the default branch producing "unsupported credential type
<nil>"; update the function containing the switch (the code handling
ResolveTyped results) to check typed for nil before the type switch on
*helmcredsv1.HelmHTTPCredentials and *credconfigv1.DirectCredentials, and add a
unit test (e.g., a stubResolver that returns (nil, nil)) asserting the function
returns (nil, nil) to lock in the behavior.
---
Nitpick comments:
In `@bindings/go/helm/input/method.go`:
- Line 12: The import alias "v2" for the package
"ocm.software/open-component-model/bindings/go/helm/spec/credentials/v1" is
misleading and inconsistent; change the import alias to helmcredsv1 and update
all uses in this file from v2.<TypeOrConst> to helmcredsv1.<TypeOrConst> (e.g.,
replace any v2.Something references near the method handling credentials in
method.go with helmcredsv1.Something) so the alias matches the package path and
the rest of the codebase.
In `@bindings/go/helm/spec/identity/v1/register.go`:
- Around line 1-11: The identity package currently only exposes
MustRegisterIdentityType and lacks a package-level auto-registration like
access/input; add a new parent package file (e.g., scheme.go) in the identity
package that declares a package-level Scheme variable and an init() which calls
MustRegisterIdentityType(Scheme) so HelmChartRepositoryIdentity (and its
VersionedType/Type aliases) are auto-registered on import; ensure the variable
name and init pattern match the established pattern used by spec/access and
spec/input.
In `@bindings/go/helm/transformation/credentials_test.go`:
- Around line 93-117: Add a unit test covering the path where ResolveTyped
returns (nil, nil): create a stubResolver with typed: nil and err: nil, call
resolveCredentialsMap(t.Context(), resolver,
runtime.Identity{"type":"HelmChartRepository"}), and assert no error and creds
== nil; this documents and locks in the expected behavior for
resolveCredentialsMap when the resolver returns a nil typed value and no error.
🪄 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: CHILL
Plan: Pro
Run ID: d9133abb-ed51-4803-995e-600a49998383
⛔ Files ignored due to path filters (1)
bindings/go/helm/go.sumis excluded by!**/*.sum
📒 Files selected for processing (27)
bindings/go/helm/digest/digest.gobindings/go/helm/go.modbindings/go/helm/input/blob.gobindings/go/helm/input/method.gobindings/go/helm/internal/credentials.gobindings/go/helm/internal/download/doc.gobindings/go/helm/internal/download/download.gobindings/go/helm/internal/download/download_test.gobindings/go/helm/internal/download/options.gobindings/go/helm/internal/download/tls.gobindings/go/helm/repository/resource/resource_repository.gobindings/go/helm/spec/credentials/v1/helm_credentials.gobindings/go/helm/spec/credentials/v1/helm_credentials_test.gobindings/go/helm/spec/credentials/v1/schemas/HelmHTTPCredentials.schema.jsonbindings/go/helm/spec/credentials/v1/zz_generated.deepcopy.gobindings/go/helm/spec/credentials/v1/zz_generated.ocm_jsonschema.gobindings/go/helm/spec/credentials/v1/zz_generated.ocm_type.gobindings/go/helm/spec/identity/v1/register.gobindings/go/helm/spec/identity/v1/schemas/HelmChartRepositoryIdentity.schema.jsonbindings/go/helm/spec/identity/v1/type.gobindings/go/helm/spec/identity/v1/type_test.gobindings/go/helm/spec/identity/v1/zz_generated.deepcopy.gobindings/go/helm/spec/identity/v1/zz_generated.ocm_jsonschema.gobindings/go/helm/spec/identity/v1/zz_generated.ocm_type.gobindings/go/helm/transformation/credentials.gobindings/go/helm/transformation/credentials_test.gobindings/go/helm/transformation/get_helm_chart.go
✅ Files skipped from review due to trivial changes (9)
- bindings/go/helm/internal/download/doc.go
- bindings/go/helm/spec/identity/v1/schemas/HelmChartRepositoryIdentity.schema.json
- bindings/go/helm/spec/credentials/v1/zz_generated.ocm_jsonschema.go
- bindings/go/helm/spec/identity/v1/zz_generated.ocm_type.go
- bindings/go/helm/spec/identity/v1/zz_generated.deepcopy.go
- bindings/go/helm/spec/credentials/v1/zz_generated.deepcopy.go
- bindings/go/helm/go.mod
- bindings/go/helm/spec/credentials/v1/schemas/HelmHTTPCredentials.schema.json
- bindings/go/helm/internal/download/tls.go
🚧 Files skipped from review as they are similar to previous changes (7)
- bindings/go/helm/spec/credentials/v1/zz_generated.ocm_type.go
- bindings/go/helm/internal/download/download_test.go
- bindings/go/helm/repository/resource/resource_repository.go
- bindings/go/helm/internal/download/options.go
- bindings/go/helm/spec/credentials/v1/helm_credentials.go
- bindings/go/helm/internal/download/download.go
- bindings/go/helm/spec/credentials/v1/helm_credentials_test.go
865197f to
95ae8bc
Compare
|
@coderabbitai full review - show no mercy, https://github.com/open-component-model/open-component-model/blob/eaae3340c9ee91e2221d20c60ebc08205d71db70/docs/adr/0018_typed_credentials.md phase 2 is important here |
🧩 Analysis chain🏁 Script executed: Length of output: 19562 🏁 Script executed: Length of output: 67955
Phase 2 Compliance ReviewOverall, the credential side ( 🔴 Critical1.
|
| # | Severity | File | Issue |
|---|---|---|---|
| 1 | 🔴 Critical | spec/identity/v1/register.go |
RegisterWithAcceptedCredentials never called — identity→credential mapping unregistered |
| 2 | 🔴 Critical | internal/credentials.go |
Returns runtime.Identity (map), not *HelmChartRepositoryIdentity |
| 3 | 🟡 Moderate | transformation/credentials.go |
DirectCredentials empty returns {} vs HelmHTTPCredentials empty returns nil |
| 4 | 🟡 Moderate | input/method.go |
v2 alias for spec/credentials/v1 is misleading |
| 5 | 🟡 Moderate | digest/digest.go, resource_repository.go |
Missing Phase 4 migration TODO comments at map[string]string boundaries |
| 6 | 🟡 Moderate | spec/identity/v1/ |
No scheme round-trip conversion test for HelmChartRepositoryIdentity |
| 7 | 🟢 Minor | spec/credentials/v1/helm_credentials.go |
CredentialKeyPassword missing //nolint:gosec |
| 8 | 🟢 Minor | spec/credentials/v1/helm_credentials_test.go |
No FromDirectCredentials(nil) test |
| 9 | 🟢 Minor | spec/identity/v1/type.go |
Port as string — intent not documented |
| 10 | 🟢 Minor | input/blob.go |
Nil credentials propagated implicitly to download layer |
Issues 1 and 2 block full Phase 2 compliance per ADR 0018 as written. Everything else is clean and follows the OCI binding pattern well.
✅ Actions performed
Full review triggered.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
bindings/go/helm/spec/identity/v1/type_test.go (1)
16-23: ⚡ Quick winAlso assert object construction through
VersionedType.You already verify registration for both types; adding
NewObject(VersionedType)would make this test fully symmetric.♻️ Proposed addition
func TestMustRegisterIdentityType(t *testing.T) { scheme := runtime.NewScheme() MustRegisterIdentityType(scheme) assert.True(t, scheme.IsRegistered(VersionedType)) assert.True(t, scheme.IsRegistered(Type)) + objV1, err := scheme.NewObject(VersionedType) + require.NoError(t, err) + _, ok := objV1.(*HelmChartRepositoryIdentity) + assert.True(t, ok, "expected *HelmChartRepositoryIdentity for VersionedType, got %T", objV1) + obj, err := scheme.NewObject(Type) require.NoError(t, err) _, ok := obj.(*HelmChartRepositoryIdentity) assert.True(t, ok, "expected *HelmChartRepositoryIdentity, got %T", obj) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindings/go/helm/spec/identity/v1/type_test.go` around lines 16 - 23, Add a symmetric assertion that constructing via VersionedType also returns the expected concrete type: call scheme.NewObject(VersionedType), check require.NoError on the err, then assert the returned obj is a *HelmChartRepositoryIdentity (same pattern as for Type). Update the test around scheme.NewObject(Type) to include this second construction path using VersionedType, referencing VersionedType, scheme.NewObject, and HelmChartRepositoryIdentity.bindings/go/helm/internal/credentials_test.go (1)
10-12: ⚡ Quick winPrefer
helm/spec/identity/v1.Typein expectations over legacy access constant.This keeps the test aligned with the new typed identity API surface and avoids coupling to legacy naming.
♻️ Proposed change
- helmaccess "ocm.software/open-component-model/bindings/go/helm/spec/access" + helmidentityv1 "ocm.software/open-component-model/bindings/go/helm/spec/identity/v1" ocicredentialsspecv1 "ocm.software/open-component-model/bindings/go/oci/spec/identity/v1" @@ - expectedType: runtime.NewUnversionedType(helmaccess.LegacyHelmChartConsumerType), + expectedType: helmidentityv1.Type, @@ - expectedType: runtime.NewUnversionedType(helmaccess.LegacyHelmChartConsumerType), + expectedType: helmidentityv1.Type,Also applies to: 28-28, 37-37
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindings/go/helm/internal/credentials_test.go` around lines 10 - 12, Replace usages of the legacy access constant from the helm access package in the test with the new typed identity constant from helm/spec/identity/v1: add/replace the import of "ocm.software/open-component-model/bindings/go/helm/spec/identity/v1" (alias e.g. helmidentity) and update expectations in credentials_test.go (including the occurrences around the other noted locations) to use helmidentity.Type (the typed identity constant) instead of the legacy helmaccess constant so the test asserts against the new identity API surface.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@bindings/go/helm/internal/credentials_test.go`:
- Around line 10-12: Replace usages of the legacy access constant from the helm
access package in the test with the new typed identity constant from
helm/spec/identity/v1: add/replace the import of
"ocm.software/open-component-model/bindings/go/helm/spec/identity/v1" (alias
e.g. helmidentity) and update expectations in credentials_test.go (including the
occurrences around the other noted locations) to use helmidentity.Type (the
typed identity constant) instead of the legacy helmaccess constant so the test
asserts against the new identity API surface.
In `@bindings/go/helm/spec/identity/v1/type_test.go`:
- Around line 16-23: Add a symmetric assertion that constructing via
VersionedType also returns the expected concrete type: call
scheme.NewObject(VersionedType), check require.NoError on the err, then assert
the returned obj is a *HelmChartRepositoryIdentity (same pattern as for Type).
Update the test around scheme.NewObject(Type) to include this second
construction path using VersionedType, referencing VersionedType,
scheme.NewObject, and HelmChartRepositoryIdentity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 55ef6da3-90af-41f0-a9c7-285843f5101e
⛔ Files ignored due to path filters (1)
bindings/go/helm/go.sumis excluded by!**/*.sum
📒 Files selected for processing (29)
bindings/go/helm/digest/digest.gobindings/go/helm/go.modbindings/go/helm/input/blob.gobindings/go/helm/input/method.gobindings/go/helm/internal/credentials.gobindings/go/helm/internal/credentials_test.gobindings/go/helm/internal/download/doc.gobindings/go/helm/internal/download/download.gobindings/go/helm/internal/download/download_test.gobindings/go/helm/internal/download/options.gobindings/go/helm/internal/download/tls.gobindings/go/helm/repository/resource/resource_repository.gobindings/go/helm/repository/resource/resource_repository_test.gobindings/go/helm/spec/credentials/v1/helm_credentials.gobindings/go/helm/spec/credentials/v1/helm_credentials_test.gobindings/go/helm/spec/credentials/v1/schemas/HelmHTTPCredentials.schema.jsonbindings/go/helm/spec/credentials/v1/zz_generated.deepcopy.gobindings/go/helm/spec/credentials/v1/zz_generated.ocm_jsonschema.gobindings/go/helm/spec/credentials/v1/zz_generated.ocm_type.gobindings/go/helm/spec/identity/v1/register.gobindings/go/helm/spec/identity/v1/schemas/HelmChartRepositoryIdentity.schema.jsonbindings/go/helm/spec/identity/v1/type.gobindings/go/helm/spec/identity/v1/type_test.gobindings/go/helm/spec/identity/v1/zz_generated.deepcopy.gobindings/go/helm/spec/identity/v1/zz_generated.ocm_jsonschema.gobindings/go/helm/spec/identity/v1/zz_generated.ocm_type.gobindings/go/helm/transformation/credentials.gobindings/go/helm/transformation/credentials_test.gobindings/go/helm/transformation/get_helm_chart.go
as stated in the ADR, the registration will come later and the wiring will happen in cli and controller
I had to add a
fixed
fixed
fixed
fixed
fixed
fixed
wont to
wont do |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me verify the current state of the changes for items 2, 3, and 4. 🏁 Script executed: Length of output: 12592
Item 1 — Registration deferred: Understood and accepted. Per ADR 0018 phased approach, wiring Item 2 — Typed identity: Confirmed in code. Item 3 — DirectCredentials nil consistency: Confirmed fixed. The consolidated Item 4 — Import alias: Confirmed fixed. Items 5–8: Acknowledged as fixed. 🎉 Items 9 & 10 — Won't do: Accepted. Phase 2 is looking solid. Great work getting there, c̶̠͇̱͎̖͙̬̀̈́̾͋̈́̌̔̆̊́t̷̛̗̙̫͙̜͈͎̣͌͗̍̾̏͐̕͝ͅͅḩ̷̧͔͚͕̥̠̪͔͛̐̆̇̅̕̕͝ͅū̵̻̫̜̫͙̬͙̰̝͉̜̹l̷̥̪͕̪̳̻͕̃͗̆̿̀̈ḣ̶̘̻̺̊̇́͂͛͛̚͝u̵̢͇̞̥̤̳̠̥͚̲̭͔̓ͅ! 🦑 ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
730fd42 to
633600f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bindings/go/helm/internal/credentials_test.go (1)
67-80:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUnreachable error branch leaves
CredentialConsumerIdentityCompaterror behavior untested.Line 69 returns on expected errors, so Lines 76-80 never execute. This dead branch also masks that non-error assertions only check the compat call’s
errvariable.Suggested fix
- identity, err := internal.CredentialConsumerIdentity(tt.repository) + identity, err := internal.CredentialConsumerIdentity(tt.repository) + compatIdentity, compatErr := internal.CredentialConsumerIdentityCompat(tt.repository) if tt.expectedErr != "" { require.ErrorContains(t, err, tt.expectedErr) assert.Nil(t, identity) + require.ErrorContains(t, compatErr, tt.expectedErr) + assert.Nil(t, compatIdentity) return } - - compatIdentity, err := internal.CredentialConsumerIdentityCompat(tt.repository) - if tt.expectedErr != "" { - require.ErrorContains(t, err, tt.expectedErr) - assert.Nil(t, compatIdentity) - return - } + require.NoError(t, err) + require.NoError(t, compatErr) legacyIdentity, ok := identity.(runtime.Identity) require.True(t, ok, "expected identity to implement runtime.Identity") require.Equal(t, compatIdentity, legacyIdentity, "expected compat identity to match legacy identity") - - require.NoError(t, err) require.NotNil(t, identity)🤖 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 `@bindings/go/helm/internal/credentials_test.go` around lines 67 - 80, The test returns immediately after checking CredentialConsumerIdentity's expected error which prevents exercising CredentialConsumerIdentityCompat and also reuses the same err variable; change the flow to call both functions before returning and use distinct error variables (e.g., err1, err2) so you can separately assert expectedErr for both CredentialConsumerIdentity and CredentialConsumerIdentityCompat, or if expectedErr is empty assert no error and non-nil identities for both identity and compatIdentity; ensure you only return after both checks complete.
🤖 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 `@bindings/go/helm/spec/credentials/v1/helm_credentials_test.go`:
- Around line 45-50: In TestFromDirectCredentials_EmptyMap, expand the
assertions beyond Username and Password to assert that CertFile, KeyFile,
Keyring are empty and that Type equals the expected default value; update the
test for the FromDirectCredentials function to include assert.Empty(t,
creds.CertFile), assert.Empty(t, creds.KeyFile), assert.Empty(t, creds.Keyring)
and assert.Equal(t, <expectedTypeValue>, creds.Type) (use the package's defined
default Type constant or zero value) so all typed fields and Type are validated
for the empty-map path.
---
Outside diff comments:
In `@bindings/go/helm/internal/credentials_test.go`:
- Around line 67-80: The test returns immediately after checking
CredentialConsumerIdentity's expected error which prevents exercising
CredentialConsumerIdentityCompat and also reuses the same err variable; change
the flow to call both functions before returning and use distinct error
variables (e.g., err1, err2) so you can separately assert expectedErr for both
CredentialConsumerIdentity and CredentialConsumerIdentityCompat, or if
expectedErr is empty assert no error and non-nil identities for both identity
and compatIdentity; ensure you only return after both checks complete.
🪄 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: CHILL
Plan: Pro
Run ID: d4e43371-e872-4b8d-84a1-d60937e50cf0
⛔ Files ignored due to path filters (2)
bindings/go/examples/go.sumis excluded by!**/*.sumbindings/go/helm/go.sumis excluded by!**/*.sum
📒 Files selected for processing (33)
bindings/go/examples/06_oci_test.gobindings/go/examples/go.modbindings/go/helm/cmd/main.gobindings/go/helm/digest/digest.gobindings/go/helm/go.modbindings/go/helm/input/blob.gobindings/go/helm/input/method.gobindings/go/helm/internal/credentials.gobindings/go/helm/internal/credentials_test.gobindings/go/helm/internal/download/doc.gobindings/go/helm/internal/download/download.gobindings/go/helm/internal/download/download_test.gobindings/go/helm/internal/download/options.gobindings/go/helm/internal/download/tls.gobindings/go/helm/repository/resource/doc.gobindings/go/helm/repository/resource/resource_repository.gobindings/go/helm/repository/resource/resource_repository_test.gobindings/go/helm/spec/credentials/v1/helm_credentials.gobindings/go/helm/spec/credentials/v1/helm_credentials_test.gobindings/go/helm/spec/credentials/v1/schemas/HelmHTTPCredentials.schema.jsonbindings/go/helm/spec/credentials/v1/zz_generated.deepcopy.gobindings/go/helm/spec/credentials/v1/zz_generated.ocm_jsonschema.gobindings/go/helm/spec/credentials/v1/zz_generated.ocm_type.gobindings/go/helm/spec/identity/v1/register.gobindings/go/helm/spec/identity/v1/schemas/HelmChartRepositoryIdentity.schema.jsonbindings/go/helm/spec/identity/v1/type.gobindings/go/helm/spec/identity/v1/type_test.gobindings/go/helm/spec/identity/v1/zz_generated.deepcopy.gobindings/go/helm/spec/identity/v1/zz_generated.ocm_jsonschema.gobindings/go/helm/spec/identity/v1/zz_generated.ocm_type.gobindings/go/helm/transformation/credentials.gobindings/go/helm/transformation/credentials_test.gobindings/go/helm/transformation/get_helm_chart.go
✅ Files skipped from review due to trivial changes (16)
- bindings/go/helm/internal/download/doc.go
- bindings/go/helm/spec/identity/v1/register.go
- bindings/go/helm/repository/resource/doc.go
- bindings/go/helm/spec/credentials/v1/zz_generated.deepcopy.go
- bindings/go/helm/spec/credentials/v1/helm_credentials.go
- bindings/go/helm/spec/credentials/v1/zz_generated.ocm_jsonschema.go
- bindings/go/helm/spec/identity/v1/type.go
- bindings/go/helm/spec/credentials/v1/schemas/HelmHTTPCredentials.schema.json
- bindings/go/helm/internal/download/download_test.go
- bindings/go/helm/spec/identity/v1/zz_generated.ocm_jsonschema.go
- bindings/go/helm/cmd/main.go
- bindings/go/helm/repository/resource/resource_repository_test.go
- bindings/go/helm/transformation/get_helm_chart.go
- bindings/go/helm/spec/identity/v1/zz_generated.deepcopy.go
- bindings/go/helm/transformation/credentials_test.go
- bindings/go/helm/spec/identity/v1/zz_generated.ocm_type.go
🚧 Files skipped from review as they are similar to previous changes (8)
- bindings/go/helm/spec/credentials/v1/zz_generated.ocm_type.go
- bindings/go/helm/input/method.go
- bindings/go/helm/spec/identity/v1/schemas/HelmChartRepositoryIdentity.schema.json
- bindings/go/helm/spec/identity/v1/type_test.go
- bindings/go/helm/input/blob.go
- bindings/go/helm/transformation/credentials.go
- bindings/go/helm/internal/download/download.go
- bindings/go/helm/go.mod
483d764 to
ae9174e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
bindings/go/helm/digest/digest.go (1)
87-91: 💤 Low value
credentialsis always non-nil here —nilguards insideresolveHTTPDigest/resolveOCIDigestare currently dead.
FromDirectCredentialsalways returns a non-nil pointer (both for Helm and OCI), so theif credentials != nilguards inside the tworesolve*helpers can never be false as called today. The guards are forward-looking (they become meaningful once Phase 4 promotes the parameters to typed pointers), but a brief comment would help future readers understand the intent rather than treating them as unreachable branches.💡 Optional: document the forward-looking intent
if strings.HasPrefix(helm.HelmRepository, "oci://") { + // FromDirectCredentials always returns non-nil; nil guard inside resolveOCIDigest + // will become meaningful once the ProcessResourceDigest signature is migrated (see TODO above). resolvedDigest, err = p.resolveOCIDigest(ctx, helm, ocicredsv1.FromDirectCredentials(credentials)) } else { + // Same note as above for resolveHTTPDigest. resolvedDigest, err = p.resolveHTTPDigest(ctx, helm, helmcredsv1.FromDirectCredentials(credentials)) }🤖 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 `@bindings/go/helm/digest/digest.go` around lines 87 - 91, The nil checks inside resolveHTTPDigest and resolveOCIDigest are currently dead because FromDirectCredentials always returns a non-nil pointer; update those functions (resolveHTTPDigest, resolveOCIDigest) to include a brief comment by the `if credentials != nil` guards explaining that callers (e.g. the call sites that pass ocicredsv1.FromDirectCredentials(credentials) / helmcredsv1.FromDirectCredentials(credentials)) always return non-nil today but the guards are intentionally kept for future phases when parameters become typed pointers — add a TODO/note referencing FromDirectCredentials to clarify the forward-looking intent for future readers.
🤖 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 `@bindings/go/helm/repository/resource/doc.go`:
- Around line 31-35: The code snippet defines typed via
CredentialProvider.ResolveTyped but still passes creds to repo.DownloadResource,
causing a mismatch; update the example to use the resolved value (either pass
typed into DownloadResource, e.g. repo.DownloadResource(ctx, res, typed), or
rename the ResolveTyped result to creds if DownloadResource expects a
credentials variable) and remove the unused creds declaration so the flow
consistently uses ResolveTyped -> typed (or creds) with DownloadResource and
assigns its result to chartBlob.
---
Nitpick comments:
In `@bindings/go/helm/digest/digest.go`:
- Around line 87-91: The nil checks inside resolveHTTPDigest and
resolveOCIDigest are currently dead because FromDirectCredentials always returns
a non-nil pointer; update those functions (resolveHTTPDigest, resolveOCIDigest)
to include a brief comment by the `if credentials != nil` guards explaining that
callers (e.g. the call sites that pass
ocicredsv1.FromDirectCredentials(credentials) /
helmcredsv1.FromDirectCredentials(credentials)) always return non-nil today but
the guards are intentionally kept for future phases when parameters become typed
pointers — add a TODO/note referencing FromDirectCredentials to clarify the
forward-looking intent for future readers.
🪄 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: CHILL
Plan: Pro
Run ID: 67f1d496-9e48-4da4-aca4-e53d19fdbad6
⛔ Files ignored due to path filters (2)
bindings/go/examples/go.sumis excluded by!**/*.sumbindings/go/helm/go.sumis excluded by!**/*.sum
📒 Files selected for processing (33)
bindings/go/examples/06_oci_test.gobindings/go/examples/go.modbindings/go/helm/cmd/main.gobindings/go/helm/digest/digest.gobindings/go/helm/go.modbindings/go/helm/input/blob.gobindings/go/helm/input/method.gobindings/go/helm/internal/credentials.gobindings/go/helm/internal/credentials_test.gobindings/go/helm/internal/download/doc.gobindings/go/helm/internal/download/download.gobindings/go/helm/internal/download/download_test.gobindings/go/helm/internal/download/options.gobindings/go/helm/internal/download/tls.gobindings/go/helm/repository/resource/doc.gobindings/go/helm/repository/resource/resource_repository.gobindings/go/helm/repository/resource/resource_repository_test.gobindings/go/helm/spec/credentials/v1/helm_credentials.gobindings/go/helm/spec/credentials/v1/helm_credentials_test.gobindings/go/helm/spec/credentials/v1/schemas/HelmHTTPCredentials.schema.jsonbindings/go/helm/spec/credentials/v1/zz_generated.deepcopy.gobindings/go/helm/spec/credentials/v1/zz_generated.ocm_jsonschema.gobindings/go/helm/spec/credentials/v1/zz_generated.ocm_type.gobindings/go/helm/spec/identity/v1/register.gobindings/go/helm/spec/identity/v1/schemas/HelmChartRepositoryIdentity.schema.jsonbindings/go/helm/spec/identity/v1/type.gobindings/go/helm/spec/identity/v1/type_test.gobindings/go/helm/spec/identity/v1/zz_generated.deepcopy.gobindings/go/helm/spec/identity/v1/zz_generated.ocm_jsonschema.gobindings/go/helm/spec/identity/v1/zz_generated.ocm_type.gobindings/go/helm/transformation/credentials.gobindings/go/helm/transformation/credentials_test.gobindings/go/helm/transformation/get_helm_chart.go
✅ Files skipped from review due to trivial changes (16)
- bindings/go/helm/internal/download/doc.go
- bindings/go/helm/spec/identity/v1/schemas/HelmChartRepositoryIdentity.schema.json
- bindings/go/helm/spec/credentials/v1/zz_generated.ocm_jsonschema.go
- bindings/go/helm/spec/identity/v1/type.go
- bindings/go/examples/06_oci_test.go
- bindings/go/helm/spec/credentials/v1/zz_generated.ocm_type.go
- bindings/go/helm/internal/download/download_test.go
- bindings/go/helm/cmd/main.go
- bindings/go/helm/spec/identity/v1/zz_generated.ocm_type.go
- bindings/go/helm/spec/identity/v1/zz_generated.deepcopy.go
- bindings/go/helm/spec/credentials/v1/helm_credentials.go
- bindings/go/examples/go.mod
- bindings/go/helm/repository/resource/resource_repository_test.go
- bindings/go/helm/spec/credentials/v1/zz_generated.deepcopy.go
- bindings/go/helm/transformation/credentials_test.go
- bindings/go/helm/go.mod
🚧 Files skipped from review as they are similar to previous changes (9)
- bindings/go/helm/spec/identity/v1/type_test.go
- bindings/go/helm/spec/identity/v1/register.go
- bindings/go/helm/transformation/get_helm_chart.go
- bindings/go/helm/spec/credentials/v1/schemas/HelmHTTPCredentials.schema.json
- bindings/go/helm/transformation/credentials.go
- bindings/go/helm/input/method.go
- bindings/go/helm/internal/credentials.go
- bindings/go/helm/internal/download/download.go
- bindings/go/helm/internal/credentials_test.go
wont do |
On-behalf-of: SAP <matthias.bruns@sap.com> Signed-off-by: Matthias Bruns <git@matthiasbruns.com>
On-behalf-of: SAP <matthias.bruns@sap.com> Signed-off-by: Matthias Bruns <git@matthiasbruns.com>
On-behalf-of: SAP <matthias.bruns@sap.com> Signed-off-by: Matthias Bruns <git@matthiasbruns.com>
On-behalf-of: SAP <matthias.bruns@sap.com> Signed-off-by: Matthias Bruns <git@matthiasbruns.com>
On-behalf-of: SAP <matthias.bruns@sap.com> Signed-off-by: Matthias Bruns <git@matthiasbruns.com>
On-behalf-of: SAP <matthias.bruns@sap.com> Signed-off-by: Matthias Bruns <git@matthiasbruns.com>
What this PR does / why we need it
Phase 2 typed helm bindings ADR
Which issue(s) this PR fixes
Contributes: open-component-model/ocm-project#988
You can see the whole picture in #2148
Testing
How to test the changes
I ran
task init/go.workwith the updatedcredentialsbinding to see if the update breaks ocm - it does not :)Verification
task testandtask test/integrationif applicable)