chore: update go version to 1.26.1#1932
Merged
Skarlso merged 3 commits intoMar 10, 2026
Merged
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughUpdated Go toolchain from 1.25.7 → 1.26.1 across CI, many module Changes
Sequence Diagram(s)Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
369-369: Use the generator module’s version file instead of another hard-coded Go pin.Within this PR, this is the one
setup-gochange that still hardcodes1.26.1, so the next upgrade can leave code generation on a different toolchain than the module metadata.actions/setup-gosupportsgo-version-file, and in v6 it will also honor atoolchaindirective fromgo.mod, which keeps this job aligned with the generator module automatically. (github.com)♻️ Suggested change
- name: Setup Go (required by some generators) uses: actions/setup-go@4b73464bb391d4059bd26b0524d20df3927bd417 # v6 with: - go-version: '1.26.1' + go-version-file: '${{ github.workspace }}/bindings/go/generator/go.mod' cache-dependency-path: | ${{ github.workspace }}/bindings/go/generator/go.sum🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml at line 369, Replace the hard-coded setup for actions/setup-go that uses go-version: '1.26.1' with the module-driven setting: use the go-version-file (or allow v6 to pick up the toolchain from go.mod) so the CI uses the generator module's version file automatically; locate the setup step that includes the go-version key in the actions/setup-go invocation and switch it to use go-version-file (pointing to the module's version file) or remove the explicit go-version so the toolchain in go.mod is honored.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/ci.yml:
- Line 369: Replace the hard-coded setup for actions/setup-go that uses
go-version: '1.26.1' with the module-driven setting: use the go-version-file (or
allow v6 to pick up the toolchain from go.mod) so the CI uses the generator
module's version file automatically; locate the setup step that includes the
go-version key in the actions/setup-go invocation and switch it to use
go-version-file (pointing to the module's version file) or remove the explicit
go-version so the toolchain in go.mod is honored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 08dd7965-a111-49d8-8266-8559e406fbd2
📒 Files selected for processing (29)
.github/workflows/ci.ymlbindings/go/blob/go.modbindings/go/cel/go.modbindings/go/configuration/go.modbindings/go/constructor/go.modbindings/go/credentials/go.modbindings/go/ctf/go.modbindings/go/dag/go.modbindings/go/descriptor/normalisation/go.modbindings/go/descriptor/runtime/go.modbindings/go/descriptor/v2/go.modbindings/go/generator/go.modbindings/go/helm/go.modbindings/go/input/dir/go.modbindings/go/input/file/go.modbindings/go/input/utf8/go.modbindings/go/oci/go.modbindings/go/oci/integration/go.modbindings/go/plugin/go.modbindings/go/repository/go.modbindings/go/rsa/go.modbindings/go/runtime/go.modbindings/go/signing/go.modbindings/go/transform/go.modcli/go.modcli/integration/go.modconformance/scenarios/sovereign/components/notes/go.modkubernetes/controller/Dockerfilekubernetes/controller/go.mod
ca2ea3f to
077dfc4
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
366-371: LGTM with a minor observation.The Go version update to 1.26.1 is correct. Note that this job hardcodes the Go version while other jobs use
go-version-fileto read from the module'sgo.mod. This creates a maintenance point that must be updated manually during future Go version bumps.Consider referencing a
go.modfile (e.g.,bindings/go/generator/go.mod) viago-version-filefor consistency, though this may not be feasible given the job's purpose.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 366 - 371, The workflow step "Setup Go (required by some generators)" currently hardcodes go-version: '1.26.1'; update this to use the actions/setup-go input go-version-file and point it to the module's go.mod (e.g., bindings/go/generator/go.mod) so the Go version is read from the file instead of being manually maintained, keeping the step that sets cache-dependency-path intact and ensuring the uses: actions/setup-go@... invocation remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 366-371: The workflow step "Setup Go (required by some
generators)" currently hardcodes go-version: '1.26.1'; update this to use the
actions/setup-go input go-version-file and point it to the module's go.mod
(e.g., bindings/go/generator/go.mod) so the Go version is read from the file
instead of being manually maintained, keeping the step that sets
cache-dependency-path intact and ensuring the uses: actions/setup-go@...
invocation remains unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b8a63d8f-7a11-4ec6-854e-76f0a5f069fc
📒 Files selected for processing (30)
.github/workflows/ci.ymlbindings/go/blob/go.modbindings/go/cel/go.modbindings/go/configuration/go.modbindings/go/constructor/go.modbindings/go/credentials/go.modbindings/go/ctf/go.modbindings/go/dag/go.modbindings/go/descriptor/normalisation/go.modbindings/go/descriptor/runtime/go.modbindings/go/descriptor/v2/go.modbindings/go/generator/go.modbindings/go/helm/go.modbindings/go/input/dir/go.modbindings/go/input/file/go.modbindings/go/input/utf8/go.modbindings/go/oci/go.modbindings/go/oci/integration/go.modbindings/go/plugin/go.modbindings/go/repository/go.modbindings/go/rsa/go.modbindings/go/runtime/go.modbindings/go/signing/go.modbindings/go/transform/go.modcli/go.modcli/integration/go.modconformance/scenarios/sovereign/components/notes/Dockerfileconformance/scenarios/sovereign/components/notes/go.modkubernetes/controller/Dockerfilekubernetes/controller/go.mod
🚧 Files skipped from review as they are similar to previous changes (23)
- cli/go.mod
- bindings/go/descriptor/v2/go.mod
- bindings/go/generator/go.mod
- bindings/go/transform/go.mod
- cli/integration/go.mod
- bindings/go/oci/go.mod
- bindings/go/credentials/go.mod
- bindings/go/blob/go.mod
- bindings/go/oci/integration/go.mod
- bindings/go/helm/go.mod
- kubernetes/controller/Dockerfile
- bindings/go/configuration/go.mod
- kubernetes/controller/go.mod
- bindings/go/ctf/go.mod
- bindings/go/descriptor/runtime/go.mod
- bindings/go/descriptor/normalisation/go.mod
- bindings/go/repository/go.mod
- bindings/go/plugin/go.mod
- bindings/go/runtime/go.mod
- bindings/go/cel/go.mod
- bindings/go/input/utf8/go.mod
- bindings/go/signing/go.mod
- bindings/go/dag/go.mod
077dfc4 to
2a6be7e
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
369-369: Avoid a second source of truth for the Go toolchain.This hard-coded version can drift from the module metadata.
actions/setup-gosupportsgo-version-fileand prefers atoolchaindirective when one exists, so pointing this step at the generator module’sgo.modwould keep CI aligned with the manifest used elsewhere. (github.com)Suggested change
- name: Setup Go (required by some generators) uses: actions/setup-go@4b73464bb391d4059bd26b0524d20df3927bd417 # v6 with: - go-version: '1.26.1' + go-version-file: '${{ github.workspace }}/bindings/go/generator/go.mod' cache-dependency-path: | ${{ github.workspace }}/bindings/go/generator/go.sum🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml at line 369, Replace the hard-coded go-version value with the setup-go action's file-based/toolchain option so CI reads the Go version from the module manifest; specifically, update the actions/setup-go step (currently using the go-version key) to use go-version-file or toolchain pointing at the generator module's go.mod so the workflow follows the module's toolchain declaration instead of a separate literal version.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/ci.yml:
- Line 369: Replace the hard-coded go-version value with the setup-go action's
file-based/toolchain option so CI reads the Go version from the module manifest;
specifically, update the actions/setup-go step (currently using the go-version
key) to use go-version-file or toolchain pointing at the generator module's
go.mod so the workflow follows the module's toolchain declaration instead of a
separate literal version.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 88418a79-30f1-45d0-ba0f-154348959f93
📒 Files selected for processing (31)
.env.github/workflows/ci.ymlbindings/go/blob/go.modbindings/go/cel/go.modbindings/go/configuration/go.modbindings/go/constructor/go.modbindings/go/credentials/go.modbindings/go/ctf/go.modbindings/go/dag/go.modbindings/go/descriptor/normalisation/go.modbindings/go/descriptor/runtime/go.modbindings/go/descriptor/v2/go.modbindings/go/generator/go.modbindings/go/helm/go.modbindings/go/input/dir/go.modbindings/go/input/file/go.modbindings/go/input/utf8/go.modbindings/go/oci/go.modbindings/go/oci/integration/go.modbindings/go/plugin/go.modbindings/go/repository/go.modbindings/go/rsa/go.modbindings/go/runtime/go.modbindings/go/signing/go.modbindings/go/transform/go.modcli/go.modcli/integration/go.modconformance/scenarios/sovereign/components/notes/Dockerfileconformance/scenarios/sovereign/components/notes/go.modkubernetes/controller/Dockerfilekubernetes/controller/go.mod
✅ Files skipped from review due to trivial changes (2)
- bindings/go/descriptor/runtime/go.mod
- .env
🚧 Files skipped from review as they are similar to previous changes (19)
- bindings/go/repository/go.mod
- bindings/go/transform/go.mod
- bindings/go/oci/integration/go.mod
- bindings/go/helm/go.mod
- kubernetes/controller/Dockerfile
- bindings/go/input/utf8/go.mod
- conformance/scenarios/sovereign/components/notes/Dockerfile
- bindings/go/plugin/go.mod
- conformance/scenarios/sovereign/components/notes/go.mod
- bindings/go/constructor/go.mod
- cli/go.mod
- bindings/go/generator/go.mod
- bindings/go/cel/go.mod
- bindings/go/oci/go.mod
- bindings/go/input/dir/go.mod
- bindings/go/configuration/go.mod
- cli/integration/go.mod
- bindings/go/rsa/go.mod
- bindings/go/descriptor/v2/go.mod
2a6be7e to
28a5c17
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
bindings/go/cel/expression/fieldpath/builder.go (1)
19-19: Consider a more efficient alternative (optional).The
nolint:staticchecksuppresses a performance warning about usingfmt.Sprintffor simple integer formatting. While the "consistency" justification is valid, usingstrconv.FormatIntorstrconv.Itoawould be more efficient if this path is hot.♻️ Optional: More efficient formatting
- b.WriteString(fmt.Sprintf("[%d]", *segment.Index)) //nolint:staticcheck // leave it consistent with the rest + b.WriteByte('[') + b.WriteString(strconv.Itoa(*segment.Index)) + b.WriteByte(']')This eliminates the allocation from
fmt.Sprintf. However, if consistency across the codebase is preferred, keeping the current approach with the nolint is acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindings/go/cel/expression/fieldpath/builder.go` at line 19, Replace the fmt.Sprintf call inside the builder that currently does b.WriteString(fmt.Sprintf("[%d]", *segment.Index)) to avoid the allocation: build the bracketed integer more efficiently by converting *segment.Index with strconv.Itoa (or strconv.FormatInt(int64(*segment.Index), 10) if the type is not int) and writing the '[' + digits + ']' via b.WriteByte/WriteString calls; update the call around segment.Index and b.WriteString to use strconv to improve performance.
🤖 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/signing/go.mod`:
- Line 3: The go.mod currently sets "go 1.26.1" which forces downstream modules
to require Go 1.26.1; if the intent was only to standardize the builder
toolchain, replace the "go 1.26.1" directive with a toolchain directive (e.g.,
add a "toolchain go1.26.1" line and revert "go" to the previously supported
version or a minimal compatible version) so published modules aren't constrained
— locate the "go 1.26.1" token in bindings/go/signing/go.mod and either lower
the "go" directive to the minimal required language version or add "toolchain
go1.26.1" to pin the builder while keeping the module's "go" version compatible.
---
Nitpick comments:
In `@bindings/go/cel/expression/fieldpath/builder.go`:
- Line 19: Replace the fmt.Sprintf call inside the builder that currently does
b.WriteString(fmt.Sprintf("[%d]", *segment.Index)) to avoid the allocation:
build the bracketed integer more efficiently by converting *segment.Index with
strconv.Itoa (or strconv.FormatInt(int64(*segment.Index), 10) if the type is not
int) and writing the '[' + digits + ']' via b.WriteByte/WriteString calls;
update the call around segment.Index and b.WriteString to use strconv to improve
performance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 60eac404-3cfc-4ba1-ab69-5ba1bc996618
📒 Files selected for processing (32)
.env.github/workflows/ci.ymlbindings/go/blob/go.modbindings/go/cel/expression/fieldpath/builder.gobindings/go/cel/go.modbindings/go/configuration/go.modbindings/go/constructor/go.modbindings/go/credentials/go.modbindings/go/ctf/go.modbindings/go/dag/go.modbindings/go/descriptor/normalisation/go.modbindings/go/descriptor/runtime/go.modbindings/go/descriptor/v2/go.modbindings/go/generator/go.modbindings/go/helm/go.modbindings/go/input/dir/go.modbindings/go/input/file/go.modbindings/go/input/utf8/go.modbindings/go/oci/go.modbindings/go/oci/integration/go.modbindings/go/plugin/go.modbindings/go/repository/go.modbindings/go/rsa/go.modbindings/go/runtime/go.modbindings/go/signing/go.modbindings/go/transform/go.modcli/go.modcli/integration/go.modconformance/scenarios/sovereign/components/notes/Dockerfileconformance/scenarios/sovereign/components/notes/go.modkubernetes/controller/Dockerfilekubernetes/controller/go.mod
✅ Files skipped from review due to trivial changes (2)
- bindings/go/credentials/go.mod
- kubernetes/controller/go.mod
🚧 Files skipped from review as they are similar to previous changes (20)
- conformance/scenarios/sovereign/components/notes/Dockerfile
- kubernetes/controller/Dockerfile
- bindings/go/oci/go.mod
- bindings/go/input/utf8/go.mod
- .env
- bindings/go/descriptor/normalisation/go.mod
- bindings/go/descriptor/runtime/go.mod
- bindings/go/generator/go.mod
- bindings/go/plugin/go.mod
- bindings/go/input/file/go.mod
- bindings/go/constructor/go.mod
- bindings/go/oci/integration/go.mod
- cli/integration/go.mod
- bindings/go/repository/go.mod
- cli/go.mod
- .github/workflows/ci.yml
- bindings/go/helm/go.mod
- bindings/go/ctf/go.mod
- bindings/go/dag/go.mod
- bindings/go/cel/go.mod
| module ocm.software/open-component-model/bindings/go/signing | ||
|
|
||
| go 1.25.7 | ||
| go 1.26.1 |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the target file and nearby go.mod files
cat -n bindings/go/signing/go.modRepository: open-component-model/open-component-model
Length of output: 1247
🏁 Script executed:
# Find all go.mod files in the repository to understand versioning strategy
find . -name "go.mod" -type f | head -20Repository: open-component-model/open-component-model
Length of output: 706
🏁 Script executed:
# Check if there are any other go.mod files in the bindings directory
cat -n bindings/go/go.mod 2>/dev/null || echo "No bindings/go/go.mod found"Repository: open-component-model/open-component-model
Length of output: 112
🏁 Script executed:
# Search for any Go 1.26-specific features (generics, ranges on maps/int, etc.)
rg "range.*map|func\(.*\[.*\].*\)\(" bindings/go/signing/ || echo "No obvious Go 1.18+ generics found"Repository: open-component-model/open-component-model
Length of output: 119
🌐 Web query:
Go 1.26 release notes toolchain directive go.mod minimum version
💡 Result:
- Go 1.26 change (release notes):
go mod initnow writes a lowergoversion by default. If you rungo mod initwith Go 1.26.x, it createsgo.modwithgo 1.25.0; if you run it with a 1.26 pre-release (RC/beta), it usesgo 1.24.0. This is meant to encourage new modules to remain compatible with currently supported Go versions. You can adjust afterward withgo get go@version. [1] - What the
godirective means (minimum version): Thegoline ingo.modsets the minimum Go version required to use/compile the module; since Go 1.21, this is mandatory/enforced (older toolchains refuse modules that require a newer Go version). [2] - What the
toolchaindirective means: Thetoolchainline declares a suggested toolchain to use when working in the main module; it can trigger automatic toolchain selection/switching when the default toolchain is older. It does not impose a requirement on dependent modules. If omitted, it’s treated as implicitly matching thegoline (effectivelytoolchain go<version-from-go-line>). [2][3][4]
Sources: [1] Go 1.26 release notes; [2] go.mod file reference; [3] Go toolchains docs; [4] Go blog: toolchain management in Go 1.21.
🏁 Script executed:
# Check go directive versions across multiple go.mod files
for file in ./bindings/go/blob/go.mod ./bindings/go/cel/go.mod ./bindings/go/credentials/go.mod ./bindings/go/descriptor/v2/go.mod ./bindings/go/runtime/go.mod; do
echo "=== $file ==="; head -3 "$file" 2>/dev/null | tail -1; doneRepository: open-component-model/open-component-model
Length of output: 322
🏁 Script executed:
# Check if go 1.26.1 appears in other binding modules
rg "^go 1\.26" bindings/go/ --type goRepository: open-component-model/open-component-model
Length of output: 67
🏁 Script executed:
# Check if there's a root go.mod with version guidance
[ -f go.mod ] && head -5 go.mod || echo "No root go.mod"Repository: open-component-model/open-component-model
Length of output: 99
Reconsider uniform go directive across published modules; use toolchain for builder version instead.
This appears to be a repo-wide change affecting all bindings modules. Setting go 1.26.1 as the minimum required version for each published module constrains all downstream consumers to Go 1.26.1, even if the code uses no Go 1.26-specific features. Go 1.26 release notes explicitly recommend lower minimum versions in new modules to avoid over-constraining compatibility.
If the intent is to standardize the build environment (CI/local development), use toolchain go1.26.1 without bumping the go directive. The toolchain directive only affects the main module's build, not dependent modules. If the intent is to genuinely require Go 1.26.1 for all consumers, that decision should be documented and applied intentionally across the project.
Suggested approach (if compatibility should be preserved)
-go 1.26.1
+go 1.25.0
+toolchain go1.26.1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| go 1.26.1 | |
| go 1.25.0 | |
| toolchain go1.26.1 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bindings/go/signing/go.mod` at line 3, The go.mod currently sets "go 1.26.1"
which forces downstream modules to require Go 1.26.1; if the intent was only to
standardize the builder toolchain, replace the "go 1.26.1" directive with a
toolchain directive (e.g., add a "toolchain go1.26.1" line and revert "go" to
the previously supported version or a minimal compatible version) so published
modules aren't constrained — locate the "go 1.26.1" token in
bindings/go/signing/go.mod and either lower the "go" directive to the minimal
required language version or add "toolchain go1.26.1" to pin the builder while
keeping the module's "go" version compatible.
28a5c17 to
4dc1778
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
cli/cmd/configuration/ocm_config.go (1)
165-166: Inconsistent path handling: cleaned path used for validation, but original returned.The code applies
filepath.Clean(env)for theos.Statcheck but returns the original uncleanedenvvalue. This could lead to inconsistent behavior if the environment variable contains non-canonical paths (e.g., with..components or trailing slashes).Consider returning the cleaned path for consistency:
func getFromEnvironment() string { if env := os.Getenv(OCMConfigEnvironmentKey); env != "" { - if _, err := os.Stat(filepath.Clean(env)); err == nil { - return env + cleanedPath := filepath.Clean(env) + if _, err := os.Stat(cleanedPath); err == nil { + return cleanedPath } } return "" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/cmd/configuration/ocm_config.go` around lines 165 - 166, The code calls filepath.Clean(env) when checking existence with os.Stat but returns the original env, causing inconsistent path values; update the return to use the cleaned path instead: compute cleaned := filepath.Clean(env), use cleaned in the os.Stat call and return cleaned (reference symbols: filepath.Clean, os.Stat, env variable) so callers always receive the canonicalized path.bindings/go/generator/ocmtypegen/main.go (2)
234-234: Minor:goModPathis already cleaned.The
goModPathparameter originates from line 207 where it's already passed throughfilepath.Clean. This second call is redundant but doesn't affect correctness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindings/go/generator/ocmtypegen/main.go` at line 234, The call to filepath.Clean is redundant when opening the module file because goModPath has already been cleaned earlier; update the os.Open invocation in main.go (where file, err := os.Open(filepath.Clean(goModPath)) appears) to simply use os.Open(goModPath) to remove the unnecessary second cleanup while keeping all error handling and variable names (file, err) unchanged.
207-207: Minor: Redundantfilepath.Cleanafterfilepath.Join.
filepath.Joinalready normalizes the resulting path. However, the explicitfilepath.Cleanhere is harmless and documents intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindings/go/generator/ocmtypegen/main.go` at line 207, The assignment to goModPath uses filepath.Clean(filepath.Join(dir, "go.mod")), which redundantly calls filepath.Clean because filepath.Join already normalizes paths; simplify by removing the redundant Clean and assign goModPath := filepath.Join(dir, "go.mod") (keep variable name goModPath and parameter dir intact) to make the code clearer while preserving behavior.
🤖 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/plugin/manager/registries/input/registry.go`:
- Line 288: The nolint comment and call to plugins.StartLogStreamer(...) are
inconsistent: replace the passed context with the registry's manager context
(use r.ctx instead of context.TODO()) so log streaming uses the manager
lifecycle, i.e., update the call to plugins.StartLogStreamer(r.ctx, plugin) and
keep the existing comment; alternatively, if the intent is for an independent
context, update the comment to state that context.TODO() is intentional and why
(affecting plugins.StartLogStreamer and the registry variable r.ctx).
---
Nitpick comments:
In `@bindings/go/generator/ocmtypegen/main.go`:
- Line 234: The call to filepath.Clean is redundant when opening the module file
because goModPath has already been cleaned earlier; update the os.Open
invocation in main.go (where file, err := os.Open(filepath.Clean(goModPath))
appears) to simply use os.Open(goModPath) to remove the unnecessary second
cleanup while keeping all error handling and variable names (file, err)
unchanged.
- Line 207: The assignment to goModPath uses filepath.Clean(filepath.Join(dir,
"go.mod")), which redundantly calls filepath.Clean because filepath.Join already
normalizes paths; simplify by removing the redundant Clean and assign goModPath
:= filepath.Join(dir, "go.mod") (keep variable name goModPath and parameter dir
intact) to make the code clearer while preserving behavior.
In `@cli/cmd/configuration/ocm_config.go`:
- Around line 165-166: The code calls filepath.Clean(env) when checking
existence with os.Stat but returns the original env, causing inconsistent path
values; update the return to use the cleaned path instead: compute cleaned :=
filepath.Clean(env), use cleaned in the os.Stat call and return cleaned
(reference symbols: filepath.Clean, os.Stat, env variable) so callers always
receive the canonicalized path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3781307e-3300-4147-878f-b5373a20f549
📒 Files selected for processing (43)
.env.github/workflows/ci.ymlbindings/go/blob/go.modbindings/go/cel/expression/fieldpath/builder.gobindings/go/cel/go.modbindings/go/configuration/go.modbindings/go/constructor/go.modbindings/go/credentials/go.modbindings/go/ctf/go.modbindings/go/dag/go.modbindings/go/descriptor/normalisation/go.modbindings/go/descriptor/runtime/go.modbindings/go/descriptor/v2/go.modbindings/go/generator/go.modbindings/go/generator/ocmtypegen/main.gobindings/go/helm/go.modbindings/go/input/dir/go.modbindings/go/input/file/go.modbindings/go/input/utf8/go.modbindings/go/oci/go.modbindings/go/oci/integration/go.modbindings/go/oci/internal/lister/lister.gobindings/go/oci/tar/oci_layout_tar_reader.gobindings/go/plugin/client/sdk/plugin.gobindings/go/plugin/go.modbindings/go/plugin/manager/registries/input/registry.gobindings/go/repository/go.modbindings/go/rsa/go.modbindings/go/runtime/go.modbindings/go/signing/go.modbindings/go/transform/go.modcli/cmd/configuration/ocm_config.gocli/cmd/describe/types/render.gocli/go.modcli/integration/go.modcli/internal/render/progress/bar/format.gocli/internal/render/progress/tracker.goconformance/scenarios/sovereign/components/notes/Dockerfileconformance/scenarios/sovereign/components/notes/cmd/sovereign-notes-v1/main.goconformance/scenarios/sovereign/components/notes/cmd/sovereign-notes/main.goconformance/scenarios/sovereign/components/notes/go.modkubernetes/controller/Dockerfilekubernetes/controller/go.mod
✅ Files skipped from review due to trivial changes (4)
- conformance/scenarios/sovereign/components/notes/cmd/sovereign-notes/main.go
- bindings/go/constructor/go.mod
- conformance/scenarios/sovereign/components/notes/cmd/sovereign-notes-v1/main.go
- bindings/go/cel/expression/fieldpath/builder.go
🚧 Files skipped from review as they are similar to previous changes (24)
- bindings/go/plugin/go.mod
- bindings/go/oci/integration/go.mod
- bindings/go/runtime/go.mod
- bindings/go/input/dir/go.mod
- bindings/go/credentials/go.mod
- .github/workflows/ci.yml
- conformance/scenarios/sovereign/components/notes/go.mod
- bindings/go/descriptor/v2/go.mod
- bindings/go/generator/go.mod
- bindings/go/dag/go.mod
- bindings/go/configuration/go.mod
- bindings/go/helm/go.mod
- cli/integration/go.mod
- bindings/go/blob/go.mod
- kubernetes/controller/Dockerfile
- conformance/scenarios/sovereign/components/notes/Dockerfile
- bindings/go/descriptor/normalisation/go.mod
- bindings/go/transform/go.mod
- bindings/go/rsa/go.mod
- bindings/go/input/utf8/go.mod
- bindings/go/signing/go.mod
- .env
- cli/go.mod
- bindings/go/ctf/go.mod
| // start log streaming once the plugin is up and running. | ||
| // use the baseCtx here from the manager here so the streaming isn't stopped when the request is stopped. | ||
| go plugins.StartLogStreamer(context.TODO(), plugin) | ||
| go plugins.StartLogStreamer(context.TODO(), plugin) //nolint:gosec // G118 - using manager's base context so log streaming outlives the request |
Contributor
There was a problem hiding this comment.
Mismatch between nolint comment and actual code.
The comment states "using manager's base context" but the code passes context.TODO(). The registry has r.ctx available (the manager's base context from line 37). If the intent is to use the manager's context for proper lifecycle management:
- go plugins.StartLogStreamer(context.TODO(), plugin) //nolint:gosec // G118 - using manager's base context so log streaming outlives the request
+ go plugins.StartLogStreamer(r.ctx, plugin) //nolint:gosec // G118 - using manager's base context so log streaming outlives the requestIf context.TODO() is intentional (log streaming should be completely independent), update the comment to reflect the actual behavior.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| go plugins.StartLogStreamer(context.TODO(), plugin) //nolint:gosec // G118 - using manager's base context so log streaming outlives the request | |
| go plugins.StartLogStreamer(r.ctx, plugin) //nolint:gosec // G118 - using manager's base context so log streaming outlives the request |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bindings/go/plugin/manager/registries/input/registry.go` at line 288, The
nolint comment and call to plugins.StartLogStreamer(...) are inconsistent:
replace the passed context with the registry's manager context (use r.ctx
instead of context.TODO()) so log streaming uses the manager lifecycle, i.e.,
update the call to plugins.StartLogStreamer(r.ctx, plugin) and keep the existing
comment; alternatively, if the intent is for an independent context, update the
comment to state that context.TODO() is intentional and why (affecting
plugins.StartLogStreamer and the registry variable r.ctx).
Contributor
Author
|
The code generation will fail because it will not use the latest and right version until it's merged. :/ |
On-behalf-of: Gergely Brautigam <gergely.brautigam@sap.com> Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
4dc1778 to
22dd6b2
Compare
matthiasbruns
previously approved these changes
Mar 9, 2026
On-behalf-of: Gergely Brautigam <gergely.brautigam@sap.com> Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
jakobmoellerdev
approved these changes
Mar 9, 2026
matthiasbruns
approved these changes
Mar 9, 2026
frewilhelm
pushed a commit
to frewilhelm/open-component-model
that referenced
this pull request
Mar 12, 2026
morri-son
pushed a commit
to morri-son/open-component-model
that referenced
this pull request
Mar 18, 2026
Signed-off-by: Gerald Morrison (SAP) <gerald.morrison@sap.com>
This was referenced Apr 14, 2026
3 tasks
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 this PR does / why we need it
Update the go version on the monorepo.
Which issue(s) this PR fixes
Testing
How to test the changes
Verification
ocmSummary by CodeRabbit
Chores
Style
No functional or API changes expected; builds now target the newer toolchain.