feat!: migrate repository & sigstore to typed credentials#2586
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (35)
📝 WalkthroughWalkthroughThis PR migrates credential handling across Open Component Model Go bindings from untyped ChangesUnified Credential Typing Migration
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Suggested Reviewers
🚥 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)
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 |
✅ Deploy Preview for ocm-website canceled.
|
|
I will update sigstore stuff when I am back - PR will land tomorrow |
69c08f7 to
c333ffc
Compare
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>
95e6258 to
d5cd76f
Compare
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>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bindings/go/repository/interface.go (1)
107-114:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate stale credential docs to typed terminology.
Line 110 and Line 113 still mention a “credentials map”, but the interface now takes
runtime.Typed. Please align the comments to avoid incorrect implementation assumptions.Suggested doc update
- // The credentials map must contain necessary authentication information to access the resource. + // The typed credentials must contain necessary authentication information to access the resource. ... - // The credentials map must contain necessary authentication information to access the resource. + // The typed credentials must contain necessary authentication information to access the resource.🤖 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/repository/interface.go` around lines 107 - 114, Update the comments for UploadResource and DownloadResource to stop referring to a "credentials map" and instead describe the credentials parameter as typed runtime credentials; specifically, change the phrasing on the UploadResource comment (above UploadResource(ctx context.Context, res *descriptor.Resource, content blob.ReadOnlyBlob, credentials runtime.Typed) ...) and the DownloadResource comment (above DownloadResource(..., credentials runtime.Typed) ...) to say the credentials parameter is a runtime.Typed containing necessary authentication information to access the resource, so implementers aren’t misled into expecting a map.
🧹 Nitpick comments (1)
bindings/go/sigstore/spec/credentials/sigstore/v1/convert_test.go (1)
20-120: ⚡ Quick winAdd regression cases for deprecated
typevalues and nil input.The table covers deprecated property keys but not deprecated
typeenums (for exampleOIDCIdentityToken/v1) and not nil input. Adding both cases would lock in backward-compat behavior and panic safety.🤖 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/sigstore/spec/credentials/sigstore/v1/convert_test.go` around lines 20 - 120, Add two regression test cases to the tests table in convert_test.go: one that supplies a runtime.Raw (or runtime.Typed) entry whose Type is the deprecated type enum (e.g., the older "OIDCIdentityToken/v1" or whichever deprecated constant your conversion supports) with Data containing the corresponding JSON and expect it to convert to the same SigstoreCredentials want value; and another case where input is nil (input: nil) and wantErr is true to ensure the converter does not panic; place these cases alongside the existing entries (refer to the tests slice, runtime.Raw usage, and fakeTyped case) so backward-compat and nil-safety are covered.
🤖 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/sigstore/doc.go`:
- Around line 57-65: Update the credential doc headings to use the v1 identity
versions: replace occurrences of "SigstoreSigner/v1alpha1" with
"SigstoreSigner/v1" and "SigstoreVerifier/v1alpha1" with "SigstoreVerifier/v1"
in the doc comment block so the documented identity types match the migrated v1
types and avoid misconfigured credential consumers.
In `@bindings/go/sigstore/spec/credentials/sigstore/v1/convert.go`:
- Around line 37-63: Check for nil and existing SigstoreCredentials before doing
any scheme conversion: in ConvertToSigstoreCredentials first return a clear
error if creds is nil to avoid creds.GetType() panics, then attempt a type
assertion on creds to *SigstoreCredentials and if it succeeds return it directly
(bypassing convertScheme). Only after those early checks proceed with the
existing flow that uses creds.GetType(), convertScheme.TypeForPrototype,
convertScheme.NewObject, and convertScheme.Convert, and keep the existing switch
handling of *v1.DirectCredentials and other types.
---
Outside diff comments:
In `@bindings/go/repository/interface.go`:
- Around line 107-114: Update the comments for UploadResource and
DownloadResource to stop referring to a "credentials map" and instead describe
the credentials parameter as typed runtime credentials; specifically, change the
phrasing on the UploadResource comment (above UploadResource(ctx
context.Context, res *descriptor.Resource, content blob.ReadOnlyBlob,
credentials runtime.Typed) ...) and the DownloadResource comment (above
DownloadResource(..., credentials runtime.Typed) ...) to say the credentials
parameter is a runtime.Typed containing necessary authentication information to
access the resource, so implementers aren’t misled into expecting a map.
---
Nitpick comments:
In `@bindings/go/sigstore/spec/credentials/sigstore/v1/convert_test.go`:
- Around line 20-120: Add two regression test cases to the tests table in
convert_test.go: one that supplies a runtime.Raw (or runtime.Typed) entry whose
Type is the deprecated type enum (e.g., the older "OIDCIdentityToken/v1" or
whichever deprecated constant your conversion supports) with Data containing the
corresponding JSON and expect it to convert to the same SigstoreCredentials want
value; and another case where input is nil (input: nil) and wantErr is true to
ensure the converter does not panic; place these cases alongside the existing
entries (refer to the tests slice, runtime.Raw usage, and fakeTyped case) so
backward-compat and nil-safety are covered.
🪄 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: eb862703-10bb-4478-baad-f7d3bd7d8a9e
⛔ Files ignored due to path filters (3)
bindings/go/repository/go.sumis excluded by!**/*.sumbindings/go/sigstore/go.sumis excluded by!**/*.sumbindings/go/sigstore/integration/go.sumis excluded by!**/*.sum
📒 Files selected for processing (37)
bindings/go/repository/component/fallback/v1/repository.gobindings/go/repository/component/fallback/v1/repository_test.gobindings/go/repository/component/resolvers/pathmatcher.gobindings/go/repository/component/resolvers/pathmatcher_test.gobindings/go/repository/go.modbindings/go/repository/interface.gobindings/go/sigstore/doc.gobindings/go/sigstore/go.modbindings/go/sigstore/integration/go.modbindings/go/sigstore/integration/integration_test.gobindings/go/sigstore/signing/handler/handler.gobindings/go/sigstore/signing/handler/handler_test.gobindings/go/sigstore/signing/handler/internal/credentials/credentials.gobindings/go/sigstore/signing/v1alpha1/config.gobindings/go/sigstore/signing/v1alpha1/schemas/VerifyConfig.schema.jsonbindings/go/sigstore/spec/credentials/sigstore/scheme.gobindings/go/sigstore/spec/credentials/sigstore/v1/convert.gobindings/go/sigstore/spec/credentials/sigstore/v1/convert_test.gobindings/go/sigstore/spec/credentials/sigstore/v1/schemas/SigstoreCredentials.schema.jsonbindings/go/sigstore/spec/credentials/sigstore/v1/sigstore_credentials.gobindings/go/sigstore/spec/credentials/sigstore/v1/zz_generated.deepcopy.gobindings/go/sigstore/spec/credentials/sigstore/v1/zz_generated.ocm_jsonschema.gobindings/go/sigstore/spec/credentials/sigstore/v1/zz_generated.ocm_type.gobindings/go/sigstore/spec/identity/signer/v1/register.gobindings/go/sigstore/spec/identity/signer/v1/schemas/SigstoreSignerIdentity.schema.jsonbindings/go/sigstore/spec/identity/signer/v1/type.gobindings/go/sigstore/spec/identity/signer/v1/type_test.gobindings/go/sigstore/spec/identity/signer/v1/zz_generated.deepcopy.gobindings/go/sigstore/spec/identity/signer/v1/zz_generated.ocm_jsonschema.gobindings/go/sigstore/spec/identity/signer/v1/zz_generated.ocm_type.gobindings/go/sigstore/spec/identity/verifier/v1/register.gobindings/go/sigstore/spec/identity/verifier/v1/schemas/SigstoreVerifierIdentity.schema.jsonbindings/go/sigstore/spec/identity/verifier/v1/type.gobindings/go/sigstore/spec/identity/verifier/v1/type_test.gobindings/go/sigstore/spec/identity/verifier/v1/zz_generated.deepcopy.gobindings/go/sigstore/spec/identity/verifier/v1/zz_generated.ocm_jsonschema.gobindings/go/sigstore/spec/identity/verifier/v1/zz_generated.ocm_type.go
💤 Files with no reviewable changes (1)
- bindings/go/sigstore/signing/handler/internal/credentials/credentials.go
On-behalf-of: SAP <matthias.bruns@sap.com> Signed-off-by: Matthias Bruns <git@matthiasbruns.com>
94530e0 to
9b13f4f
Compare
|
versions are now alpha1 instead of v1 |
58c16af to
a56b8c6
Compare
a56b8c6 to
042042b
Compare
On-behalf-of: SAP <matthias.bruns@sap.com> Signed-off-by: Matthias Bruns <git@matthiasbruns.com>
…nent-model#2586) #### What this PR does / why we need it This PR migrates the following bindings to runtime.Typed credentials - repository - sigstore Since we touch central interfaces, this is a breaking change. For `sigstore`, this PR already introduces typed credentials and identities and updates all code paths where possible. (identity is mostly there for documentation, since we delayed the migration to typed identities) The migration path can be observed here: open-component-model#2519 #### Which issue(s) this PR fixes Contributes: - open-component-model/ocm-project#1055 #### Testing - breaking change only tested with binding tests - go.work disabled, task test is green --------- Signed-off-by: Matthias Bruns <git@matthiasbruns.com> Signed-off-by: Gerald Morrison (SAP) <gerald.morrison@sap.com> Co-authored-by: Gerald Morrison (SAP) <gerald.morrison@sap.com> 6c9693c
) ## Summary Gate 6 of the phased [ADR 0018 / issue #1047](open-component-model/ocm-project#1047) credentials migration. Builds on gates 1–5 (PRs #2580, #2586, #2594, #2598, #2602). > **Note:** this branch also contains #2613 (plugin `CredentialsFromHeader` refactor). Once that merges, this PR will show only the helm changes. ### helm binding changes - `cmd/main.go`, `input/method.go`: `ProcessResource`/`ProcessSource` → `runtime.Typed` - `digest/digest.go`: `ProcessResourceDigest` → `runtime.Typed`; single `ConvertCredentials` call replaces two separate conversions - `repository/resource/resource_repository.go`: `DownloadResource`/`UploadResource` → `runtime.Typed`; `var _ repository.ResourceRepository` assertion restored - `transformation/get_helm_chart.go`: use upstream typed `ResourceRepository` interface; delete `transformation/credentials.go` - `spec/credentials/v1/convert.go`: new `ConvertCredentials(runtime.Typed) (*HelmHTTPCredentials, *OCICredentials, error)` — single scheme-based conversion returning both types - `spec/credentials/scheme.go`: package-level `Scheme` for helm credentials - `spec/credentials/v1/helm_credentials.go`: remove deprecated exported constants and `FromDirectCredentials` (now private) - `go.mod`: `plugin` → v0.0.16, `blob` → v0.0.13, `repository` → v0.0.9; no replace directives ## Test plan - [ ] `cd bindings/go/helm && go build ./... && go test ./...` (cmd/* requires `task build` for plugin binary) - [ ] `grep -rn "map\[string\]string" bindings/go/helm/` returns no credential parameter usages Refs: #1047 Signed-off-by: Jakob Möller <contact@jakob-moeller.com>
…entials (gate 7) (#2616) #### What this PR does / why we need it Gate 7 of the typed credentials migration (ocm-project#1047). Migrates `bindings/go/transfer` and `kubernetes/controller` to use `runtime.Typed` credentials instead of `map[string]string`. **bindings/go/transfer:** - Bump `blob→v0.0.13`, `credentials→v0.0.12`, `oci→v0.0.43`, `repository→v0.0.9` - Update OCI identity import path: `oci/spec/credentials/identity/v1` → `oci/spec/identity/v1` **kubernetes/controller:** - `resolveResourceCredentials` and `VerifyResource` return `runtime.Typed` instead of `map[string]string` - Replace `map[string]string` RSA credential literals with `rsacredentialsv1.RSACredentials` structs - Update import aliases for `oci/spec/credentials` and `oci/spec/identity/v1` - `GetComponentVersionRepository` call sites updated to pass `runtime.Typed` credentials #### Which issue(s) this PR fixes Contributes: - open-component-model/ocm-project#1047 - open-component-model/ocm-project#1055 - open-component-model/ocm-project#1056 - open-component-model/ocm-project#1057 #### Binding release order | Gate | PRs | Modules | Status | |------|-----|---------|--------| | 1 | ✅ #2580 | blob, signing, rsa | merged | | 2 | ✅ #2586 | repository, sigstore | merged | | 3 | ✅ #2594 | oci | merged | | 4 | ✅ #2598 | constructor | merged | | 5 | ✅ #2602 | plugin, input/dir, input/file, input/utf8 | merged | | 6 | ✅ #2612 | helm | merged | | 7 | **this PR** | transfer, controller | 👈 | | 8 | PR 8 | cli | — | --------- Signed-off-by: Jakob Möller <contact@jakob-moeller.com> Signed-off-by: Matthias Bruns <git@matthiasbruns.com> Co-authored-by: Matthias Bruns <git@matthiasbruns.com>
On-behalf-of: SAP <matthias.bruns@sap.com> <!-- markdownlint-disable MD041 --> #### What this PR does / why we need it Final PR of the breaking change chain for the credential migration. This PR updated the CLI. #### Which issue(s) this PR fixes Fixed: open-component-model/ocm-project#1056 | Gate | PRs | Modules | Status | |------|-----|---------|--------| | 1 | ✅ #2580 | blob, signing, rsa | merged | | 2 | ✅ #2586 | repository, sigstore | merged | | 3 | ✅ #2594 | oci | merged | | 4 | ✅ #2598 | constructor | merged | | 5 | ✅ #2602 | plugin, input/dir, input/file, input/utf8 | merged | | 6 | ✅ #2612 | helm | merged | | 7 | ✅ #2616 | transfer, controller | merged | | 8 | **this PR** | cli | 👈 | #### Testing ##### How to test the changes ##### Verification - [ ] I have added/updated tests for my changes (see [Test Requirements](../CONTRIBUTING.md#test-requirements)) - [x] Tests pass locally (`task test` and `task test/integration` if applicable) - [ ] If touching multiple modules, `go work` is enabled (see `go.work`) - [x] My changes do not decrease test coverage - [ ] I have tested the changes locally by running `ocm` --------- Signed-off-by: Matthias Bruns <git@matthiasbruns.com>
What this PR does / why we need it
This PR migrates the following bindings to runtime.Typed credentials
Since we touch central interfaces, this is a breaking change.
For
sigstore, this PR already introduces typed credentials and identities and updates all code paths where possible. (identity is mostly there for documentation, since we delayed the migration to typed identities)The migration path can be observed here: #2519
Which issue(s) this PR fixes
Contributes:
Testing