SANDBOX-1465: update kube & openshift dependencies to 4.20 #718
Conversation
WalkthroughThis PR updates the Go toolchain to 1.24, aligns many module dependencies, bumps controller-gen/Kubebuilder annotations in CRD manifests to v0.18.0, clears ignored vulnerabilities in .govulncheck.yaml, adjusts some controllers/tests to use RequeueAfter semantics, and adds two go.mod replace directives. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
| toolchain go1.23.12 | ||
| toolchain go1.24.11 | ||
|
|
||
| replace ( |
rsoaresd
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (17)
.govulncheck.yaml(1 hunks)README.adoc(1 hunks)config/crd/bases/toolchain.dev.openshift.com_idlers.yaml(1 hunks)config/crd/bases/toolchain.dev.openshift.com_memberoperatorconfigs.yaml(1 hunks)config/crd/bases/toolchain.dev.openshift.com_memberstatuses.yaml(1 hunks)config/crd/bases/toolchain.dev.openshift.com_nstemplatesets.yaml(1 hunks)config/crd/bases/toolchain.dev.openshift.com_spacebindingrequests.yaml(1 hunks)config/crd/bases/toolchain.dev.openshift.com_spacerequests.yaml(1 hunks)config/crd/bases/toolchain.dev.openshift.com_toolchainclusters.yaml(1 hunks)config/crd/bases/toolchain.dev.openshift.com_useraccounts.yaml(1 hunks)config/crd/bases/toolchain.dev.openshift.com_workspaces.yaml(1 hunks)controllers/idler/idler_controller.go(1 hunks)controllers/idler/idler_controller_test.go(0 hunks)controllers/memberoperatorconfig/memberoperatorconfig_controller.go(1 hunks)controllers/useraccount/useraccount_controller.go(2 hunks)go.mod(5 hunks)openshift-ci/Dockerfile.tools(1 hunks)
💤 Files with no reviewable changes (1)
- controllers/idler/idler_controller_test.go
⏰ 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). (3)
- GitHub Check: GolangCI Lint
- GitHub Check: Test with Coverage
- GitHub Check: Build & push operator bundles & dashboard image for e2e tests
🔇 Additional comments (15)
config/crd/bases/toolchain.dev.openshift.com_memberstatuses.yaml (1)
6-6: LGTM!Controller-gen version annotation update from v0.17.3 to v0.18.0 is consistent with the controller-tools upgrade.
config/crd/bases/toolchain.dev.openshift.com_toolchainclusters.yaml (1)
6-6: LGTM!Controller-gen version annotation update is consistent with the controller-tools upgrade.
config/crd/bases/toolchain.dev.openshift.com_memberoperatorconfigs.yaml (1)
6-6: LGTM!Controller-gen version annotation update is consistent with the controller-tools upgrade.
config/crd/bases/toolchain.dev.openshift.com_idlers.yaml (1)
6-6: LGTM!Controller-gen version annotation update is consistent with the controller-tools upgrade.
config/crd/bases/toolchain.dev.openshift.com_spacebindingrequests.yaml (1)
6-6: LGTM!Controller-gen version annotation update is consistent with the controller-tools upgrade.
config/crd/bases/toolchain.dev.openshift.com_nstemplatesets.yaml (1)
6-6: LGTM! CRD regeneration verified.All CRD files have been consistently regenerated with controller-gen v0.18.0. The version annotation update in the metadata is correct across all 9 CRDs in config/crd/bases/, confirming proper regeneration as part of the controller-tools upgrade.
config/crd/bases/toolchain.dev.openshift.com_useraccounts.yaml (1)
6-6: LGTM!The Kubebuilder controller-gen version annotation update aligns with the controller-tools upgrade in go.mod (v0.17.3 → v0.18.0).
README.adoc (1)
13-13: LGTM!The Go version requirement update is consistent with the go.mod and Dockerfile.tools updates in this PR.
config/crd/bases/toolchain.dev.openshift.com_spacerequests.yaml (1)
6-6: LGTM!Kubebuilder annotation update is consistent with the controller-tools upgrade across all CRD files.
controllers/useraccount/useraccount_controller.go (2)
8-9: LGTM!Minor import formatting change, likely from auto-formatting.
68-68: LGTM!The comment update accurately reflects the requeue semantics. The change aligns with the PR description noting that
result.Requeueis deprecated.config/crd/bases/toolchain.dev.openshift.com_workspaces.yaml (1)
6-6: LGTM!Kubebuilder annotation update is consistent with the controller-tools upgrade.
go.mod (1)
128-130: LGTM!Go toolchain version updates are consistent with the README and Dockerfile changes.
openshift-ci/Dockerfile.tools (1)
12-13: SHA256 hash for Go 1.24.11 is correct. The provided hash matches the official Go distribution and maintains supply chain security..govulncheck.yaml (1)
1-1: This file was newly created in the initial commit with an empty vulnerabilities list. There were no previously ignored vulnerabilities, so no verification is needed.Likely an incorrect or invalid review comment.
| // Note: | ||
| // The Controller will requeue the Request to be processed again if the returned error is non-nil or | ||
| // Result.Requeue is true, otherwise upon completion it will remove the work from the queue. | ||
| // Result.Requeue > 0 is true, otherwise upon completion it will remove the work from the queue. |
There was a problem hiding this comment.
Incorrect comment syntax.
The comment states "Result.Requeue > 0 is true", but Result.Requeue is a boolean field. The comparison operator > cannot be applied to a boolean type.
Apply this fix to correct the comment:
-// Result.Requeue > 0 is true, otherwise upon completion it will remove the work from the queue.
+// Result.Requeue is true or Result.RequeueAfter > 0, otherwise upon completion it will remove the work from the queue.📝 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.
| // Result.Requeue > 0 is true, otherwise upon completion it will remove the work from the queue. | |
| // Result.Requeue is true or Result.RequeueAfter > 0, otherwise upon completion it will remove the work from the queue. |
🤖 Prompt for AI Agents
controllers/memberoperatorconfig/memberoperatorconfig_controller.go around line
48: the inline comment incorrectly suggests using "Result.Requeue > 0" which
implies numeric comparison on a boolean; change the comment to state that
Result.Requeue is a boolean (e.g. "Result.Requeue is true" or "Result.Requeue ==
true") and/or rephrase to "when Result.Requeue is true the item will be
requeued" so it no longer uses the '>' operator.
| replace ( | ||
| github.com/codeready-toolchain/api => github.com/rsoaresd/api v0.0.0-20251217181659-99c33cce84d0 | ||
| github.com/codeready-toolchain/toolchain-common => github.com/rsoaresd/toolchain-common v0.0.0-20251217181906-1b70d2afca0e | ||
| ) |
There was a problem hiding this comment.
Remove temporary replace directives before merging.
The replace directives point to personal forks (rsoaresd/api and rsoaresd/toolchain-common) and must be removed before this PR is merged. These temporary replacements would break production builds if merged.
Ensure these are only used for development/testing of coordinated changes across repositories, and revert to the official module paths before merge.
#!/bin/bash
# Verify if the referenced commits exist in the official repositories
echo "Checking if commits are available in official repositories..."
# Check api repository
echo "API commit: 99c33cce84d0"
gh api repos/codeready-toolchain/api/commits/99c33cce84d0 --jq '.sha' 2>&1 || echo "Commit not found in official api repo"
# Check toolchain-common repository
echo "Toolchain-common commit: 1b70d2afca0e"
gh api repos/codeready-toolchain/toolchain-common/commits/1b70d2afca0e --jq '.sha' 2>&1 || echo "Commit not found in official toolchain-common repo"🤖 Prompt for AI Agents
In go.mod around lines 132 to 135, remove the temporary replace directives that
point to personal forks (github.com/rsoaresd/api and
github.com/rsoaresd/toolchain-common) and restore the module references to the
official upstream modules (i.e., delete the entire replace (...) block or revert
it to the canonical modules/versions used by the project); if those upstream
commits are not yet available, document the dependency requirement in the PR and
use a local development workflow (or a separate branch) instead of committing
replace directives to be merged.
| @@ -1,4 +1,4 @@ | |||
| OWNER_AND_BRANCH_LOCATION=codeready-toolchain/toolchain-cicd/master | |||
| OWNER_AND_BRANCH_LOCATION=rsoaresd/toolchain-cicd/update_4_20_cicd | |||
|
/retest flaky test |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #718 +/- ##
===========================================
- Coverage 82.53% 63.29% -19.24%
===========================================
Files 48 48
Lines 3596 3594 -2
===========================================
- Hits 2968 2275 -693
- Misses 477 1171 +694
+ Partials 151 148 -3
🚀 New features to boost your workflow:
|
| // Note: | ||
| // The Controller will requeue the Request to be processed again if the returned error is non-nil or | ||
| // Result.Requeue is true, otherwise upon completion it will remove the work from the queue. | ||
| // Result.RequeueAfter > 0 is true, otherwise upon completion it will remove the work from the queue. |
There was a problem hiding this comment.
btw, idler controller still uses the Result.Requeue - shouldn't it be removed?
There was a problem hiding this comment.
Yes, thanks!! It was a leftover
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
make/run-cicd-script.mk (1)
11-11: Update comment to reflect current branch.The comment references "master" but line 1 currently points to the
update_4_20_cicdbranch. While this entire change is temporary, the comment should be accurate or generic (e.g., "using version from configured branch").
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
controllers/idler/idler_controller.go(1 hunks)controllers/memberoperatorconfig/memberoperatorconfig_controller.go(1 hunks)controllers/useraccount/useraccount_controller.go(2 hunks)make/run-cicd-script.mk(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- controllers/memberoperatorconfig/memberoperatorconfig_controller.go
- controllers/idler/idler_controller.go
⏰ 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). (1)
- GitHub Check: Build & push operator bundles & dashboard image for e2e tests
🔇 Additional comments (1)
controllers/useraccount/useraccount_controller.go (1)
68-68: LGTM! Correctly migrated from deprecatedResult.RequeuetoRequeueAfter.The changes properly address the deprecation of
Result.Requeueby usingRequeueAfter: time.Second, which maintains the postponed reconciliation behavior while aligning with controller-runtime v0.21.0 best practices. The documentation comment on line 68 has also been updated to reflect this change.Also applies to: 140-142
| @@ -1,4 +1,4 @@ | |||
| OWNER_AND_BRANCH_LOCATION=codeready-toolchain/toolchain-cicd/master | |||
| OWNER_AND_BRANCH_LOCATION=rsoaresd/toolchain-cicd/update_4_20_cicd | |||
There was a problem hiding this comment.
Revert to main repository before merge.
This change points to a personal fork and feature branch (rsoaresd/toolchain-cicd/update_4_20_cicd) instead of the canonical repository. While appropriate for testing coordinated dependency updates, this must be reverted to codeready-toolchain/toolchain-cicd/master before merging to prevent breaking CI/CD for all users.
🔎 Apply this diff to revert to the main repository:
-OWNER_AND_BRANCH_LOCATION=rsoaresd/toolchain-cicd/update_4_20_cicd
+OWNER_AND_BRANCH_LOCATION=codeready-toolchain/toolchain-cicd/master📝 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.
| OWNER_AND_BRANCH_LOCATION=rsoaresd/toolchain-cicd/update_4_20_cicd | |
| OWNER_AND_BRANCH_LOCATION=codeready-toolchain/toolchain-cicd/master |
🤖 Prompt for AI Agents
In make/run-cicd-script.mk around line 1, the OWNER_AND_BRANCH_LOCATION points
to a personal fork/branch (rsoaresd/toolchain-cicd/update_4_20_cicd); change it
back to the canonical repository and branch by setting OWNER_AND_BRANCH_LOCATION
to codeready-toolchain/toolchain-cicd/master, commit the change on the feature
branch, and push so the PR uses the main repo for CI.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexeykazakov, MatousJobanek, rsoaresd The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3 participants
Description
Update dependencies
NOTE THAT
result.Requeueis deprecatedRelated PRs
codeready-toolchain/api#495
codeready-toolchain/toolchain-common#503
codeready-toolchain/host-operator#1226
codeready-toolchain/toolchain-e2e#1239
codeready-toolchain/registration-service#565
codeready-toolchain/toolchain-cicd#165
kubesaw/ksctl#137
wa#311
https://github.com/codeready-toolchain/sandboxctl/pull/59
https://github.com/codeready-toolchain/sandbox-sre/pull/2815
https://github.com/codeready-toolchain/mcp-server-devsandbox/pull/49
Issue ticket number and link
SANDBOX-1465
Summary by CodeRabbit
Chores
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.