feat!: update rsa, blob and signing to use runtime.Typed#2580
Conversation
…f map[string]string On-behalf-of: SAP <matthias.bruns@sap.com> Signed-off-by: Matthias Bruns <git@matthiasbruns.com>
✅ Deploy Preview for ocm-website ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
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 (4)
📝 WalkthroughWalkthroughThis PR migrates signing and blob transformation interfaces from untyped ChangesTyped Credentials Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
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 |
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>
cbd1235 to
79ef436
Compare
On-behalf-of: SAP <matthias.bruns@sap.com> Signed-off-by: Matthias Bruns <git@matthiasbruns.com>
79ef436 to
05d988f
Compare
On-behalf-of: SAP <matthias.bruns@sap.com> Signed-off-by: Matthias Bruns <git@matthiasbruns.com>
97bce45 to
1dbdb50
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>
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 `@bindings/go/rsa/signing/handler/handler_test.go`:
- Around line 224-226: The test closures capture the shared outer variable
rootPEM causing races when sibling subtests run in parallel; fix by copying the
value into a per-subtest local variable before creating the closure (e.g.,
inside each case: localRoot := rootPEM and then have creds return
&rsacredentialsv1.RSACredentials{PublicKeyPEMFile: localRoot}). Apply this
pattern to every creds closure and any other closures in the same test that
reference rootPEM (and analogous shared vars) so each subtest closes over its
own copy instead of the shared variable.
🪄 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: 81acc3bf-1bd7-462c-b450-16227dade1f8
⛔ Files ignored due to path filters (1)
bindings/go/rsa/go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
bindings/go/blob/transformer/transformer.gobindings/go/rsa/go.modbindings/go/rsa/signing/handler/handler.gobindings/go/rsa/signing/handler/handler_test.gobindings/go/rsa/signing/handler/internal/credentials/credentials.gobindings/go/rsa/spec/credentials/v1/rsa_credentials.gobindings/go/rsa/spec/credentials/v1/rsa_credentials_test.gobindings/go/rsa/spec/credentials/v1/schemas/RSACredentials.schema.jsonbindings/go/rsa/spec/credentials/v1/scheme.gobindings/go/signing/interface.go
On-behalf-of: SAP <matthias.bruns@sap.com> Signed-off-by: Matthias Bruns <git@matthiasbruns.com>
efb8781 to
087820c
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>
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 `@bindings/go/rsa/spec/credentials/v1/convert.go`:
- Around line 21-40: In ConvertToRSACredentials, replace the two fmt.Errorf
calls that currently use "%v" with "%w" to wrap the original error (the calls
around convertScheme.NewObject and convertScheme.Convert), i.e., pass err as the
wrapped error; also remove the unnecessary blank line before the final closing
brace of the function so there is no trailing empty line inside the function
block.
🪄 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: 9758266e-a245-4707-9465-0d96c18f21cc
📒 Files selected for processing (6)
bindings/go/rsa/signing/handler/handler.gobindings/go/rsa/spec/credentials/v1/convert.gobindings/go/rsa/spec/credentials/v1/convert_test.gobindings/go/rsa/spec/credentials/v1/rsa_credentials.gobindings/go/rsa/spec/credentials/v1/rsa_credentials_test.gobindings/go/rsa/spec/credentials/v1/scheme.go
💤 Files with no reviewable changes (1)
- bindings/go/rsa/spec/credentials/v1/rsa_credentials_test.go
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>
…ent-model#2580) #### What this PR does / why we need it This PR migrates the following bindings to runtime.Typed credentials - blob - rsa - signing Since we touch central interfaces, this is a breaking change. 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 Fully tested locally with `go.work` enabled in open-component-model/ocm-project#1047 --------- Signed-off-by: Matthias Bruns <git@matthiasbruns.com> c7e5118
Updates all credential-passing interfaces in the constructor binding from map[string]string to runtime.Typed, following the same pattern established by the OCI (open-component-model#2594), blob, rsa, and signing (open-component-model#2580) migrations. - ProcessResource, ProcessSource, ProcessResourceDigest, DownloadResource now accept runtime.Typed instead of map[string]string - resolveCredentials now calls ResolveTyped and returns runtime.Typed - Bumps credentials dependency from v0.0.10 to v0.0.11 Contributes to: open-component-model/ocm-project#1055 On-behalf-of: @SAP <jakob.moeller@sap.com> Signed-off-by: Jakob Möller <contact@jakob-moeller.com> On-behalf-of: @SAP <jakob.moeller@sap.com>
#### What this PR does / why we need it Migrates the `constructor` binding to use `runtime.Typed` credentials, following the established pattern from: - #2580 (blob, rsa, signing) - #2594 (oci) Updates the following interfaces from `map[string]string` → `runtime.Typed`: - `ResourceInputMethod.ProcessResource` - `SourceInputMethod.ProcessSource` - `ResourceDigestProcessor.ProcessResourceDigest` - `ResourceRepository.DownloadResource` Also migrates `resolveCredentials` to call `ResolveTyped` and bumps the `credentials` dependency from `v0.0.10` → `v0.0.11`. #### Which issue(s) this PR fixes Contributes: - open-component-model/ocm-project#1055 Part of the migration path: #2519 #### Dependencies This PR depends on #2594 (oci) being merged first. #### Testing - All existing constructor binding tests pass (`go test ./...`) - Breaking change — callers must update mock implementations to accept `runtime.Typed` --------- Signed-off-by: Jakob Möller <contact@jakob-moeller.com>
) ## 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.
The migration path can be observed here: #2519
Which issue(s) this PR fixes
Contributes:
Testing
Fully tested locally with
go.workenabled in open-component-model/ocm-project#1047