feat!: migrate constructor binding to runtime.Typed credentials#2598
Conversation
👷 Deploy Preview for ocm-website processing.
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR migrates credential handling in the Go constructor bindings from untyped ChangesTyped Credentials Migration
🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
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)
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 |
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>
649f004 to
1ed0d01
Compare
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/constructor/interface.go`:
- Line 50: The method comment for ProcessResource (and any other related
comments in this file that still reference a “credentials map”) is stale after
the signature migration; update those doc comments to refer to the typed
credentials parameter runtime.Typed and briefly describe its role/shape instead
of a map. Specifically, revise the comment above ProcessResource (and other
occurrences that mention credentials) to mention the runtime.Typed typed
credentials argument passed to the handler, and ensure related symbols like
constructor.Resource and ResourceInputMethodResult are described consistently
with the new signature.
🪄 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: 5d259cda-9423-4d19-8386-bb5b0928c11e
⛔ Files ignored due to path filters (1)
bindings/go/constructor/go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
bindings/go/constructor/construct.gobindings/go/constructor/construct_resource_test.gobindings/go/constructor/construct_source_test.gobindings/go/constructor/go.modbindings/go/constructor/input_method_registry_test.gobindings/go/constructor/interface.go
Signed-off-by: Jakob Möller <contact@jakob-moeller.com> On-behalf-of: @SAP <jakob.moeller@sap.com>
2f5bcd0
into
open-component-model:main
…als v0.0.12 compat) (#2603) ## Summary - `credentials v0.0.12` unified the `Resolver` interface to a single `Resolve(ctx, identity) (runtime.Typed, error)` method, removing the separate `ResolveTyped` method that existed in v0.0.11 - `constructor v0.0.8` (released as part of gate 4, PR #2598) still calls `provider.ResolveTyped` and depends on `credentials v0.0.11` - This causes a build failure when any module depends on both `constructor v0.0.8` and `credentials v0.0.12` This fix updates `construct.go` to call `provider.Resolve` and bumps the credentials dependency to `v0.0.12`. ## Dependency on Must be released as `constructor/v0.0.9` before PR #2602 (gate 5 plugin migration) can drop its `replace` directive. Signed-off-by: Jakob Möller <contact@jakob-moeller.com>
…credentials (gate 5) (#2602) ## Summary Gate 5 of the typed credentials migration (#1047). Migrates `plugin`, `constructor`, and `input/*` modules from `map[string]string` to `runtime.Typed` credentials. This continues the series started in gates 1-4 (blob, signing, rsa, repository, sigstore, oci, constructor published as PR #2598). ### Modules changed | Module | Change | |--------|--------| | `bindings/go/constructor` | `Resolver.Resolve` return type `map[string]string` → `runtime.Typed`; bump credentials v0.0.11→v0.0.12 | | `bindings/go/input/dir` | `ProcessResource`/`ProcessSource` credentials param `map[string]string` → `runtime.Typed` | | `bindings/go/input/file` | same | | `bindings/go/input/utf8` | same | | `bindings/go/plugin` | All contract interfaces, registry implementations, handlers, converters, test plugins | ### Key design decisions - **Client side** (`toCredentials`): `nil` credentials → sends `"{}"` JSON to preserve valid Authorization header format - **Server side** (handlers): deserialize Authorization header into `&runtime.Raw{}` since concrete type is unknown at HTTP boundary - **`plugin/go.mod`**: uses `replace` directive pointing to local `../constructor` (gate 5 constructor not yet released as module tag) ### Related issues - Part of #1047 (typed credentials migration) - Supersedes draft PR #2599 ## Test plan - [x] `go build ./...` passes in all 5 affected modules - [x] `go test ./...` passes in `constructor`, `input/dir`, `input/file`, `input/utf8` - [x] `go test ./...` passes in `plugin` (96 files changed, all registry/handler/flow tests pass) - [ ] Integration tests with downstream modules (gates 6-8 not yet migrated) --------- 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
Migrates the
constructorbinding to useruntime.Typedcredentials, following the established pattern from:Updates the following interfaces from
map[string]string→runtime.Typed:ResourceInputMethod.ProcessResourceSourceInputMethod.ProcessSourceResourceDigestProcessor.ProcessResourceDigestResourceRepository.DownloadResourceAlso migrates
resolveCredentialsto callResolveTypedand bumps thecredentialsdependency fromv0.0.10→v0.0.11.Which issue(s) this PR fixes
Contributes:
Part of the migration path: #2519
Dependencies
This PR depends on #2594 (oci) being merged first.
Testing
go test ./...)runtime.Typed