Skip to content

upgrade ocm sdk-go - #448

Merged
clyang82 merged 1 commit into
openshift-online:mainfrom
skeeey:upgrade-sdk
Dec 19, 2025
Merged

upgrade ocm sdk-go#448
clyang82 merged 1 commit into
openshift-online:mainfrom
skeeey:upgrade-sdk

Conversation

@skeeey

@skeeey skeeey commented Dec 19, 2025

Copy link
Copy Markdown

refer to open-cluster-management-io/sdk-go#178

and upgrade golang to 1.25.0

@coderabbitai

coderabbitai Bot commented Dec 19, 2025

Copy link
Copy Markdown

Walkthrough

Bumps Go toolchain to 1.25.0, removes the action parameter from resource subscription/handler callbacks and related internal types, updates imports, and inserts pre-check and disk-cleanup steps into e2e CI workflows.

Changes

Cohort / File(s) Summary
Go toolchain & deps
.tekton/integration-test.yaml, .tekton/unit-test.yaml, Dockerfile, Dockerfile.e2e, go.mod
Updated Go version from 1.24.x → 1.25.0 in CI scripts and Docker builder images; bumped Go version in go.mod and updated patch versions of open-cluster-management modules.
Subscription / handler signature changes
cmd/maestro/server/event_server.go, pkg/client/cloudevents/grpcsource/watcherstore.go, test/performance/pkg/hub/store/createonly.go, test/helper.go
Removed the action parameter from subscription callbacks and HandleReceivedResource method signatures; replaced action-based branching with status-based determination (treat as Modified or Deleted based on ManifestWork status); adjusted logging and imports.
Internal type cleanup
cmd/maestro/server/grpc_broker.go
Deleted internal non-exported types resourceHandler and subscriber and associated declarations (internal cleanup).
E2E workflow maintenance steps
.github/workflows/e2e.yml
Inserted pre-check and disk-cleanup steps (df -h /, cleanup commands, re-check df -h /) before the Checkout step in e2e GitHub Actions jobs.
Container builder image update
Containerfile.rhtap
Updated builder base image tag from rhel_9_1.24rhel_9_1.25.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Verify all implementations and call sites of HandleReceivedResource and subscription callbacks were updated to the new signature.
  • Search for leftover references to types.ResourceAction and the deleted resourceHandler / subscriber types.
  • Review status-determination logic in pkg/client/cloudevents/grpcsource/watcherstore.go to confirm deletion and modification cases are covered.
  • Validate CI/Dockerfile and go.mod changes compile with Go 1.25.0 and updated module versions.
  • Inspect added e2e disk-cleanup steps for potential side effects on runners (permissions, expected directories).

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title 'upgrade ocm sdk-go' accurately reflects the primary objective of the PR, which is to upgrade the OCM SDK-Go dependency along with Go toolchain updates.
Description check ✅ Passed The description directly relates to the changeset by referencing the upstream SDK-Go PR and mentioning the Go version upgrade, which aligns with the actual changes made.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 785b4e1 and a5201fa.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (10)
  • .tekton/integration-test.yaml (1 hunks)
  • .tekton/unit-test.yaml (1 hunks)
  • Dockerfile (1 hunks)
  • Dockerfile.e2e (1 hunks)
  • cmd/maestro/server/event_server.go (1 hunks)
  • cmd/maestro/server/grpc_broker.go (0 hunks)
  • go.mod (2 hunks)
  • pkg/client/cloudevents/grpcsource/watcherstore.go (2 hunks)
  • test/helper.go (2 hunks)
  • test/performance/pkg/hub/store/createonly.go (1 hunks)
💤 Files with no reviewable changes (1)
  • cmd/maestro/server/grpc_broker.go
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/maestro/server/event_server.go (2)
pkg/api/resource_types.go (1)
  • Resource (14-27)
pkg/db/context.go (1)
  • NewContext (13-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
  • GitHub Check: Red Hat Konflux / maestro-e2e-on-pull-request
  • GitHub Check: Red Hat Konflux / maestro-on-pull-request
  • GitHub Check: e2e
  • GitHub Check: e2e-grpc-broker
  • GitHub Check: e2e-with-istio
  • GitHub Check: upgrade
  • GitHub Check: e2e-broadcast-subscription
🔇 Additional comments (9)
test/performance/pkg/hub/store/createonly.go (1)

26-29: LGTM - Signature updated to match SDK interface.

The removal of the action parameter aligns with the SDK upgrade. Since this is a no-op implementation used for performance testing, the change has no functional impact.

test/helper.go (1)

350-352: LGTM - Test helper updated for new SDK signature.

The subscription callback has been updated to match the new SDK interface, removing the action parameter. The logic remains straightforward - directly updating the resource status in the test store.

pkg/client/cloudevents/grpcsource/watcherstore.go (1)

120-128: Improved event type determination from status conditions.

The refactored logic now derives the watch event type directly from the ManifestWork's status conditions rather than relying on an external action parameter. This approach is more accurate and self-contained.

The logic correctly identifies deletion events via the ResourceDeleted condition and defaults to Modified for all other cases.

Dockerfile.e2e (1)

1-1: LGTM - Builder image updated consistently.

The Go builder image has been updated to align with the toolchain upgrade across all build configurations.

Dockerfile (1)

1-1: LGTM - Consistent Go version upgrade.

Builder image updated to match the Go 1.25.0 toolchain upgrade applied across all build configurations.

.tekton/integration-test.yaml (1)

51-51: LGTM - Integration tests updated to Go 1.25.0.

This completes the consistent Go toolchain upgrade across all build and test configurations.

cmd/maestro/server/event_server.go (1)

95-112: Simplified status update handling—all scenarios are properly covered.

The callback signature has been updated to remove the action parameter, and status updates are now processed uniformly through handleStatusUpdate. This function properly handles all scenarios: standard status modifications via resource validation and status conversion, plus delete cases through the ResourceDeleted condition check. The change reduces code branching while preserving delete handling through condition-based logic.

go.mod (2)

58-58: Replace pseudo-versions with stable tagged versions before merge.

Both open-cluster-management.io/api and open-cluster-management.io/sdk-go are pinned to pseudo-versions (commit-hash based). Pseudo-versions are development placeholders and should be resolved to proper semantic versions before merging to main.

Verify that the upstream changes referenced in the PR (open-cluster-management-io/sdk-go#178) have been released as tagged versions, then update go.mod accordingly.

Also applies to: 60-60


3-3: Review Go 1.25.0 compatibility.

Go 1.25 maintains compatibility for almost all programs. The main breaking changes are a compiler bug fix for nil pointer checks (affecting incorrect error-handling patterns) and stricter x509 certificate validation. OCM projects require Go 1.23+, so they should be compatible. Monitor for any nil pointer panics or certificate validation failures in testing.

Comment thread .tekton/unit-test.yaml

# Install golang with a given version
export GOVERSION=1.24.4
export GOVERSION=1.25.0

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Go 1.25.0 is a stable release with important runtime and compiler changes—verify container behavior and certificate handling.

Go 1.25.0 was officially released in August 2025 and maintains the Go 1 promise of compatibility. However, several behavioral changes warrant attention:

  1. Compiler fix: A compiler bug introduced in Go 1.21 that incorrectly delayed nil pointer checks is fixed in 1.25, which may cause previously passing code to panic. Review error handling patterns in the codebase.

  2. Container runtime: Go 1.25 now considers CPU bandwidth limits of cgroups, adjusting GOMAXPROCS accordingly. If this project runs in Kubernetes or Docker, verify the new behavior aligns with deployment constraints.

  3. Certificate handling: CreateCertificate now uses truncated SHA-256 for SubjectKeyId instead of SHA-1, reversible via GODEBUG=x509sha256skid=0.

@skeeey
skeeey force-pushed the upgrade-sdk branch 2 times, most recently from ffbe2cb to d366495 Compare December 19, 2025 01:41

@clyang82 clyang82 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Signed-off-by: Wei Liu <liuweixa@redhat.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
go.mod (1)

3-3: Consider upgrading to Go 1.25.5 for security fixes.

Go 1.25.5 (released 2025-12-02) includes two security fixes to the crypto/x509 package, as well as bug fixes to the mime and os packages. Since this PR is already upgrading the Go toolchain, consider updating to the latest patch version for improved security posture.

🔎 Proposed change
-go 1.25.0
+go 1.25.5

Note: You'll also need to update the corresponding Docker base images in Dockerfile.e2e and other Docker-related files to golang:1.25.5.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between d366495 and 04c2f02.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (12)
  • .github/workflows/e2e.yml (4 hunks)
  • .tekton/integration-test.yaml (1 hunks)
  • .tekton/unit-test.yaml (1 hunks)
  • Containerfile.rhtap (1 hunks)
  • Dockerfile (1 hunks)
  • Dockerfile.e2e (1 hunks)
  • cmd/maestro/server/event_server.go (1 hunks)
  • cmd/maestro/server/grpc_broker.go (0 hunks)
  • go.mod (2 hunks)
  • pkg/client/cloudevents/grpcsource/watcherstore.go (2 hunks)
  • test/helper.go (2 hunks)
  • test/performance/pkg/hub/store/createonly.go (1 hunks)
💤 Files with no reviewable changes (1)
  • cmd/maestro/server/grpc_broker.go
🚧 Files skipped from review as they are similar to previous changes (6)
  • .github/workflows/e2e.yml
  • test/helper.go
  • Dockerfile
  • Containerfile.rhtap
  • .tekton/unit-test.yaml
  • .tekton/integration-test.yaml
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/maestro/server/event_server.go (2)
pkg/api/resource_types.go (1)
  • Resource (14-27)
pkg/db/context.go (1)
  • NewContext (13-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
  • GitHub Check: Red Hat Konflux / maestro-e2e-on-pull-request
  • GitHub Check: Red Hat Konflux / maestro-on-pull-request
  • GitHub Check: e2e-broadcast-subscription
  • GitHub Check: e2e-grpc-broker
  • GitHub Check: e2e-with-istio
  • GitHub Check: upgrade
  • GitHub Check: e2e
🔇 Additional comments (4)
Dockerfile.e2e (1)

1-1: LGTM! Base image update aligns with Go toolchain upgrade.

The update from golang:1.24 to golang:1.25 is consistent with the Go toolchain upgrade to 1.25.0 in go.mod.

cmd/maestro/server/event_server.go (1)

95-112: LGTM! Subscription callback refactored to remove action parameter.

The callback signature simplification aligns with the SDK upgrade. The ownership check using statusDispatcher.Dispatch correctly filters resources, and error handling is appropriate.

test/performance/pkg/hub/store/createonly.go (1)

26-29: LGTM! Test stub updated to match new SDK interface.

The signature change removes the action parameter, aligning with the SDK upgrade. Since this is a no-op stub for performance testing, the change has no functional impact.

pkg/client/cloudevents/grpcsource/watcherstore.go (1)

120-128: LGTM! Logic simplified with status-based watch type determination.

The refactored method removes the action parameter and determines the watch type based on the resource's deletion status. The logic correctly defaults to watch.Modified and switches to watch.Deleted only when the ResourceDeleted condition is true.

@clyang82
clyang82 merged commit 7fea9a3 into openshift-online:main Dec 19, 2025
11 checks passed
@skeeey
skeeey deleted the upgrade-sdk branch December 24, 2025 06:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants