SANDBOX-1465: update kube & openshift dependencies to 4.20 #1226
Conversation
|
Warning Rate limit exceeded@rsoaresd has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 12 minutes and 30 seconds before requesting another review. ⌛ 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. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
WalkthroughThis PR upgrades the Go toolchain and numerous dependencies, bumps kubebuilder controller-gen annotations to v0.18.0 across CRDs, clears the .govulncheck ignored list, and shifts controllers/tests to use RequeueAfter (duration) instead of the Requeue (boolean) flag, including a signature change in space cleanup helper. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
config/crd/bases/toolchain.dev.openshift.com_bannedusers.yaml (1)
6-6: LGTM!Same controller-gen version annotation update as other CRD files.
config/crd/bases/toolchain.dev.openshift.com_usertiers.yaml (1)
6-6: LGTM!Same controller-gen version annotation update as other CRD files.
config/crd/bases/toolchain.dev.openshift.com_usersignups.yaml (1)
6-6: LGTM!Same controller-gen version annotation update as other CRD files.
config/crd/bases/toolchain.dev.openshift.com_spaces.yaml (1)
6-6: LGTM!Same controller-gen version annotation update as other CRD files.
config/crd/bases/toolchain.dev.openshift.com_spacerequests.yaml (1)
6-6: LGTM!Same controller-gen version annotation update as other CRD files.
config/crd/bases/toolchain.dev.openshift.com_socialevents.yaml (1)
6-6: LGTM!Same controller-gen version annotation update as other CRD files.
📜 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 (22)
.govulncheck.yaml(1 hunks)README.adoc(1 hunks)config/crd/bases/toolchain.dev.openshift.com_bannedusers.yaml(1 hunks)config/crd/bases/toolchain.dev.openshift.com_masteruserrecords.yaml(1 hunks)config/crd/bases/toolchain.dev.openshift.com_notifications.yaml(1 hunks)config/crd/bases/toolchain.dev.openshift.com_nstemplatetiers.yaml(1 hunks)config/crd/bases/toolchain.dev.openshift.com_proxyplugins.yaml(1 hunks)config/crd/bases/toolchain.dev.openshift.com_socialevents.yaml(1 hunks)config/crd/bases/toolchain.dev.openshift.com_spacebindingrequests.yaml(1 hunks)config/crd/bases/toolchain.dev.openshift.com_spacebindings.yaml(1 hunks)config/crd/bases/toolchain.dev.openshift.com_spaceprovisionerconfigs.yaml(1 hunks)config/crd/bases/toolchain.dev.openshift.com_spacerequests.yaml(1 hunks)config/crd/bases/toolchain.dev.openshift.com_spaces.yaml(1 hunks)config/crd/bases/toolchain.dev.openshift.com_tiertemplaterevisions.yaml(1 hunks)config/crd/bases/toolchain.dev.openshift.com_tiertemplates.yaml(1 hunks)config/crd/bases/toolchain.dev.openshift.com_toolchainclusters.yaml(1 hunks)config/crd/bases/toolchain.dev.openshift.com_toolchainconfigs.yaml(1 hunks)config/crd/bases/toolchain.dev.openshift.com_toolchainstatuses.yaml(1 hunks)config/crd/bases/toolchain.dev.openshift.com_usersignups.yaml(1 hunks)config/crd/bases/toolchain.dev.openshift.com_usertiers.yaml(1 hunks)go.mod(5 hunks)openshift-ci/Dockerfile.tools(1 hunks)
⏰ 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: Build & push operator bundles & dashboard image for e2e tests
- GitHub Check: Test with Coverage
🔇 Additional comments (17)
README.adoc (1)
13-13: LGTM! Go version requirement updated.The Go version requirement has been updated from 1.23.x to 1.24.x, aligning with the broader dependency upgrade in this PR. Go 1.24.11 includes security fixes and is a stable release.
config/crd/bases/toolchain.dev.openshift.com_notifications.yaml (1)
6-6: LGTM! Controller-gen version annotation updated consistently.The controller-gen version annotation has been correctly updated to v0.18.0 across all 18 CRD base files, as confirmed in the project's go.mod dependency and go.sum checksums. This is a metadata-only change with no impact on CRD structure or behavior, consistent with the PR's objective to upgrade controller-tools.
config/crd/bases/toolchain.dev.openshift.com_toolchainstatuses.yaml (1)
6-6: LGTM!The controller-gen version annotation update from v0.17.3 to v0.18.0 is consistent with the PR objectives. This is expected when regenerating CRDs with updated tooling.
config/crd/bases/toolchain.dev.openshift.com_spacebindings.yaml (1)
6-6: LGTM!Controller-gen version annotation updated consistently with other CRD bases in this PR.
config/crd/bases/toolchain.dev.openshift.com_toolchainconfigs.yaml (1)
6-6: LGTM!Controller-gen version annotation updated consistently.
config/crd/bases/toolchain.dev.openshift.com_toolchainclusters.yaml (1)
6-6: LGTM!Controller-gen version annotation updated consistently.
config/crd/bases/toolchain.dev.openshift.com_spacebindingrequests.yaml (1)
6-6: LGTM!Controller-gen version annotation updated consistently.
config/crd/bases/toolchain.dev.openshift.com_nstemplatetiers.yaml (1)
6-6: LGTM!Controller-gen version annotation updated consistently.
config/crd/bases/toolchain.dev.openshift.com_masteruserrecords.yaml (1)
6-6: LGTM!Controller-gen version annotation updated consistently.
openshift-ci/Dockerfile.tools (1)
11-12: Go 1.24.11 SHA256 checksum verified.The provided checksum matches the official Go 1.24.11 linux-amd64 release.
config/crd/bases/toolchain.dev.openshift.com_tiertemplates.yaml (1)
6-6: LGTM! Controller-gen version annotation updated.The controller-gen version annotation has been correctly updated to v0.18.0, consistent with the controller-tools upgrade described in the PR objectives.
config/crd/bases/toolchain.dev.openshift.com_spaceprovisionerconfigs.yaml (1)
6-6: LGTM! Controller-gen version annotation updated.config/crd/bases/toolchain.dev.openshift.com_tiertemplaterevisions.yaml (1)
6-6: LGTM! Controller-gen version annotation updated.config/crd/bases/toolchain.dev.openshift.com_proxyplugins.yaml (1)
6-6: LGTM! Controller-gen version annotation updated.go.mod (2)
11-14: Address breaking changes in controller-runtime v0.21.0 upgrade.The upgrade to controller-runtime v0.21.0 introduces two significant breaking changes:
- Client-side ratelimiter is now disabled by default; preserve previous behavior by setting QPS 20 and Burst 30 on rest.Config
- NewUnmanaged and NewTypedUnmanaged controllers no longer require a manager
The remaining dependency upgrades (Kubernetes, controller-tools, OpenShift packages, Prometheus) require validation:
- Run tests with the new versions to confirm compatibility
- Review any API changes in dependent repositories (toolchain-common, api)
- Verify OpenShift release-4.20 compatibility
155-157: Flag replace directives as requiring explicit documentation.The Go version upgrade to 1.24.0 with toolchain go1.24.11 is verified and valid. However, the replace directives at lines 159-162 pointing to personal forks (rsoaresd/api and rsoaresd/toolchain-common) require explicit confirmation:
- Are these replacements temporary for cross-repo coordination or permanent?
- Please confirm these are documented in the PR description or related issue.
- Ensure CI/CD pipelines can access these forked repositories and commit hashes before merge.
.govulncheck.yaml (1)
1-1: Clearing the ignored vulnerabilities list is intentional and correct.The previously ignored vulnerabilities (GO-2025-4013, GO-2025-4012, GO-2025-4011, GO-2025-4010, GO-2025-4009, GO-2025-4008, GO-2025-4007, GO-2025-4006) were all in Go standard library packages and had fixes in Go 1.24.8+. With the Kubernetes and OpenShift 4.20 dependency upgrade, these vulnerabilities are no longer present, and their silence-until dates (2025-12-03) have naturally expired. The empty ignore list is the correct state.
go.mod
Outdated
| toolchain go1.23.12 | ||
| toolchain go1.24.11 | ||
|
|
||
| replace ( |
rsoaresd
changed the title
| res, err := r.Reconcile(context.TODO(), req) | ||
| // then | ||
| require.NoError(t, err) | ||
| require.False(t, res.Requeue, "requeue should not be set") |
There was a problem hiding this comment.
I'm curios why we need to remove it?
There was a problem hiding this comment.
result.Requeue is deprecated
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
controllers/masteruserrecord/masteruserrecord_controller_test.go (1)
1437-1437: Consider updating for consistency.For consistency with the updated test pattern, consider changing this to check
RequeueAfterspecifically:- assert.Equal(t, reconcile.Result{Requeue: false}, res) + assert.Empty(t, res.RequeueAfter)This would align with the controller-runtime v0.21.0 pattern used throughout the rest of the test file.
controllers/space/space_controller_test.go (1)
118-118: LGTM! Test assertion pattern updated consistently.The migration from
assert.False(t, res.Requeue)toassert.Empty(t, res.RequeueAfter)correctly aligns with controller-runtime's reconcile semantics where a zero/emptyRequeueAfterduration indicates no requeue. All 22 updated assertions appear in appropriate "no requeue expected" contexts (error cases, completion states, pending states).However, note that the file retains mixed patterns—some tests still check the
Requeueboolean field (e.g., lines 79, 157, 176, 1157, 1362, 1599, 1718, 1777). While this may be intentional if different scenarios warrant different checks, consider standardizing on theRequeueAfterpattern throughout for consistency and maintainability.Also applies to: 206-206, 231-231, 313-313, 340-340, 367-367, 388-388, 421-421, 454-454, 564-564, 832-832, 874-874, 966-966, 1320-1320, 1451-1451, 1493-1493, 1662-1662, 1822-1822, 1840-1840, 1877-1877, 1940-1940, 1973-1973
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
controllers/deactivation/deactivation_controller.go(1 hunks)controllers/deactivation/deactivation_controller_test.go(0 hunks)controllers/masteruserrecord/masteruserrecord_controller.go(1 hunks)controllers/masteruserrecord/masteruserrecord_controller_test.go(3 hunks)controllers/notification/notification_controller_test.go(6 hunks)controllers/socialevent/socialevent_controller_test.go(1 hunks)controllers/space/space_controller_test.go(22 hunks)controllers/spacecleanup/space_cleanup_controller_test.go(10 hunks)controllers/spaceprovisionerconfig/spaceprovisionerconfig_controller_test.go(3 hunks)controllers/toolchainconfig/toolchainconfig_controller.go(1 hunks)controllers/usersignup/usersignup_controller.go(1 hunks)controllers/usersignup/usersignup_controller_test.go(7 hunks)controllers/usersignupcleanup/usersignup_cleanup_controller.go(1 hunks)
💤 Files with no reviewable changes (1)
- controllers/deactivation/deactivation_controller_test.go
✅ Files skipped from review due to trivial changes (5)
- controllers/deactivation/deactivation_controller.go
- controllers/usersignup/usersignup_controller.go
- controllers/toolchainconfig/toolchainconfig_controller.go
- controllers/masteruserrecord/masteruserrecord_controller.go
- controllers/usersignupcleanup/usersignup_cleanup_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). (2)
- GitHub Check: GolangCI Lint
- GitHub Check: Build & push operator bundles & dashboard image for e2e tests
🔇 Additional comments (11)
controllers/usersignup/usersignup_controller_test.go (1)
973-973: LGTM! Test assertions correctly updated for controller-runtime v0.21.0.The assertions have been properly updated from checking the boolean
Requeuefield to checking theRequeueAfterduration field, which aligns with the controller-runtime upgrade from v0.20.4 to v0.21.0. Usingassert.Empty(t, res.RequeueAfter)correctly verifies that no requeue is requested (zero duration = no requeue). The changes are consistent across all test cases and maintain the original test intent.Also applies to: 1036-1036, 4536-4536, 4557-4557, 4582-4582, 4602-4602, 4636-4636
controllers/masteruserrecord/masteruserrecord_controller_test.go (1)
74-74: LGTM! Test assertions updated to useRequeueAftersemantics.The changes correctly update test assertions from checking the boolean
Requeuefield to checkingRequeueAfterduration. Usingassert.Empty(t, result.RequeueAfter)verifies that no requeue is scheduled (duration = 0), which aligns with controller-runtime v0.21.0 best practices and improves consistency with other tests in this file.Also applies to: 109-109, 146-146
controllers/notification/notification_controller_test.go (5)
83-83: LGTM: Correct migration to RequeueAfter semantics.The change from checking
result.Requeueto verifyingresult.RequeueAfteris empty correctly asserts that no requeue is scheduled after successful deletion.
112-112: LGTM: Correct assertion for error case.The empty
RequeueAftercheck is appropriate when an error is returned, as the reconciler should not schedule a requeue in error scenarios.
156-156: LGTM: Correct requeue assertions for successful delivery.The assertions correctly verify that
RequeueAfteris greater than zero, ensuring the controller schedules a requeue to delete the notification after the configured timeout.Also applies to: 203-203, 264-264
305-305: LGTM: Correct assertion for delivery failure.When notification delivery fails with an error, verifying that
RequeueAfteris empty is correct, as the error handling should trigger retry logic rather than scheduled requeue.
83-83: Update assertions to check RequeueAfter instead of Requeue for controller-runtime v0.21.0 compatibility.Result.Requeue was deprecated in controller-runtime v0.21.0. The change to assert on
RequeueAfteraligns with the recommended migration path away from the deprecated field.Also applies to: 112-112, 156-156, 203-203, 264-264, 305-305
controllers/spaceprovisionerconfig/spaceprovisionerconfig_controller_test.go (1)
374-374: Checking onlyRequeueAfteris correct for controller-runtime v0.21.0.The
Requeuefield was deprecated in controller-runtime v0.21.0 (#3107). TheRequeueAfterfield semantically implies thatRequeueis true, and there is no need to setRequeueto true at the same time asRequeueAfter. By checking thatRequeueAfteris empty, the test correctly verifies that no requeue occurs—neither immediate nor delayed. The test change from checkingRequeue == falseto checkingRequeueAfter.Empty()aligns with the deprecation of theRequeuefield.controllers/socialevent/socialevent_controller_test.go (1)
77-77: LGTM! Assertion updated to check RequeueAfter instead of Requeue.The change from checking the boolean
Requeuefield to checking theRequeueAfterduration aligns with controller-runtime v0.21.0's deprecation ofResult.Requeue. SinceRequeueAftersupersedesRequeue, this assertion correctly verifies that no requeue is requested after successful reconciliation.controllers/spacecleanup/space_cleanup_controller_test.go (2)
52-53: Verify consistency with the Requeue field usage.This test case still checks
res.Requeuedirectly (line 52) while all other test cases were updated to checkres.RequeueAfter. If the controller-runtime v0.21.0 API has moved away from theRequeuefield, this assertion might need updating to align with the new pattern. Consider whether this should be changed to only checkRequeueAftersemantics (e.g.,assert.NotEmpty(t, res.RequeueAfter)andassert.LessOrEqual(t, res.RequeueAfter, time.Second)).
37-37: Review the assertion pattern against the tested reconcile result semantics.The change from checking
Requeue == falseto checkingRequeueAfterbeing empty modifies which requeue condition is validated. RequeueAfter tells the Controller to requeue after a duration and implies Requeue is true; there is no need to set Requeue to true at the same time. Comprehensive requeue state validation typically checks both fields, but if the tested code returns emptyResult{}structs for all no-requeue cases, this change is semantically valid. Confirm that the reconcile methods being tested consistently return empty Results when no requeue is needed.Also applies to: 69-69, 84-84, 104-104, 124-124, 148-148, 166-166, 186-186, 205-205, 223-223
make/run-cicd-script.mk
Outdated
| @@ -1,4 +1,4 @@ | |||
| OWNER_AND_BRANCH_LOCATION=codeready-toolchain/toolchain-cicd/master | |||
| OWNER_AND_BRANCH_LOCATION=rsoaresd/toolchain-cicd/update_4_20_cicd | |||
alexeykazakov
left a comment
alexeykazakov
left a comment
There was a problem hiding this comment.
There is a few places left where we can start using "assert.Empty(t, res.RequeueAfter)" in the tests. Otherwise looks good.
| res, err := r.Reconcile(context.TODO(), req) | ||
| // then | ||
| require.NoError(t, err) | ||
| require.False(t, res.Requeue, "requeue should not be set") |
There was a problem hiding this comment.
I guess we can also switch it to "assert.Empty(t, res.RequeueAfter)" as in other places.
There was a problem hiding this comment.
Since the below line has require.Equal(t, time.Duration(0), res.RequeueAfter, "requeueAfter should not be set"), i did not add it. But I will replace require.Equal(t, time.Duration(0), res.RequeueAfter, "requeueAfter should not be set") to assert.Empty(t, res.RequeueAfter, "requeueAfter should not be set")
|
|
||
| // then | ||
| require.NoError(t, err) | ||
| require.False(t, res.Requeue) |
There was a problem hiding this comment.
I had to delete this check because if we replace to assert.Empty(t, res.RequeueAfter, "requeueAfter should not be set"), it will fail with:
--- FAIL: TestReconcile/controller_should_deactivate_user/usersignup_should_be_marked_as_deactivating_-_deactivate30_(30_days)/reconciliation_should_be_requeued_when_notification_not_yet_sent/usersignup_requeued_after_deactivating_notification_created_for_user (0.00s)
deactivation_controller_test.go:429:
Error Trace: /home/runner/work/host-operator/host-operator/controllers/deactivation/deactivation_controller_test.go:429
Error: Should be empty, but was 71h59m59.98258003s
Test: TestReconcile/controller_should_deactivate_user/usersignup_should_be_marked_as_deactivating_-
Since res.Requeue is deprecated, we can only check that the requeue is not immediate by checking the RequeueAfter, in the way we are already checking in the line below (that should be ~about 3 days, in this particular case)
There was a problem hiding this comment.
it's because it's being set (we expect the requeue) - this is also verified in the following lines
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
make/run-cicd-script.mk (1)
1-1: Revert this temporary change before merging.This points CI/CD scripts to your feature branch for cross-repo testing. Before merging to master, revert to
codeready-toolchain/toolchain-cicd/master. Also note that the comment on line 11 still references "master" and may need updating for consistency.go.mod (1)
159-162: Remove temporary replace directives before merge.These replace directives point to your forks for cross-repo testing and must be removed before merging to master. A previous review flagged the
rsoaresd/apifork as inaccessible.Re-verify that both forks are accessible and contain the referenced commits:
#!/bin/bash # Verify fork repositories and commits are accessible echo "Checking rsoaresd/api fork and commit..." curl -s https://api.github.com/repos/rsoaresd/api/commits/99c33cce84d0 | jq -r 'if .sha then "✓ Found commit: \(.sha)\n Message: \(.commit.message // "N/A")" else "✗ Commit not found or repo not accessible" end' echo -e "\nChecking rsoaresd/toolchain-common fork and commit..." curl -s https://api.github.com/repos/rsoaresd/toolchain-common/commits/1b70d2afca0e | jq -r 'if .sha then "✓ Found commit: \(.sha)\n Message: \(.commit.message // "N/A")" else "✗ Commit not found or repo not accessible" end' echo -e "\nVerifying the related PRs are merged before removing these replace directives:" echo "- codeready-toolchain/api#495" echo "- codeready-toolchain/toolchain-common#503" gh pr view 495 --repo codeready-toolchain/api --json state,mergedAt | jq -r '"api#495 state: \(.state), merged: \(.mergedAt // "not merged")"' gh pr view 503 --repo codeready-toolchain/toolchain-common --json state,mergedAt | jq -r '"toolchain-common#503 state: \(.state), merged: \(.mergedAt // "not merged")"'
🧹 Nitpick comments (2)
controllers/space/space_controller_test.go (2)
24-24: Prefer built-in types over cast library for duration comparison.The
castlibrary is unnecessary for comparingRequeueAfterdurations. SinceRequeueAfteris already atime.Duration, you can compare directly withtime.Duration(0)or just check> 0.Apply this pattern throughout the file:
-assert.Greater(t, res.RequeueAfter, cast.ToDuration("0s")) +assert.Greater(t, res.RequeueAfter, time.Duration(0))Or even simpler:
-assert.Greater(t, res.RequeueAfter, cast.ToDuration("0s")) +assert.True(t, res.RequeueAfter > 0)This eliminates the need for the
castimport and makes the intent clearer.
158-158: Consider completing the migration from deprecated Requeue to RequeueAfter.Several assertions in this file still use the deprecated
Requeueboolean field pattern, such as:
assert.Equal(t, reconcile.Result{Requeue: false}, res)assert.Equal(t, reconcile.Result{Requeue: true, RequeueAfter: 1 * time.Second}, res)For consistency with the changes made elsewhere in this file and to fully align with controller-runtime v0.21.0, consider updating these to use the RequeueAfter-based pattern:
// Instead of: assert.Equal(t, reconcile.Result{Requeue: false}, res) // Use: assert.Empty(t, res.RequeueAfter) // Instead of: assert.Equal(t, reconcile.Result{Requeue: true, RequeueAfter: 1 * time.Second}, res) // Use: assert.Equal(t, 1*time.Second, res.RequeueAfter)Also applies to: 177-177, 488-488, 520-520, 600-600, 632-632, 667-667, 759-759, 790-790, 932-935, 994-997, 1024-1024, 1073-1076, 1112-1112, 1533-1536
📜 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 (6)
controllers/deactivation/deactivation_controller_test.go(13 hunks)controllers/space/space_controller_test.go(30 hunks)controllers/spacecleanup/space_cleanup_controller_test.go(10 hunks)controllers/usersignupcleanup/usersignup_cleanup_controller_test.go(2 hunks)go.mod(5 hunks)make/run-cicd-script.mk(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- controllers/spacecleanup/space_cleanup_controller_test.go
🔇 Additional comments (7)
controllers/deactivation/deactivation_controller_test.go (2)
28-28: LGTM! Necessary import for RequeueAfter assertions.The
assertpackage import is required for the newassert.Empty()calls used throughout the test file to verify thatRequeueAfteris not set, replacing the deprecatedRequeueboolean checks.
228-228: LGTM! Correct migration from deprecated Requeue to RequeueAfter semantics.All assertions correctly verify that
RequeueAfteris empty (zero duration) in cases where no requeue is expected. This properly replaces the deprecatedresult.Requeueboolean field and aligns with the controller-runtime v0.21.0 upgrade.Also applies to: 241-241, 285-285, 333-333, 374-374, 466-466, 495-495, 514-514, 603-603, 623-623, 722-722, 754-754
controllers/usersignupcleanup/usersignup_cleanup_controller_test.go (1)
211-211: LGTM! Correct migration from deprecated Requeue to RequeueAfter.The assertions correctly verify that no requeue is requested by checking
RequeueAfteris empty, which aligns with the deprecation of the booleanRequeuefield in controller-runtime v0.21.0.Also applies to: 254-254
controllers/space/space_controller_test.go (2)
119-119: LGTM! Correct migration to RequeueAfter-based assertions.These assertions correctly verify that no requeue is requested by checking
RequeueAfteris empty, which properly aligns with the deprecation of the booleanRequeuefield in controller-runtime v0.21.0.Also applies to: 207-207, 232-232, 314-314, 341-341, 368-368, 389-389, 422-422, 455-455, 565-565, 833-833, 875-875, 967-967, 1319-1319, 1450-1450, 1492-1492, 1661-1661, 1821-1821, 1839-1839, 1876-1876, 1939-1939, 1972-1972
80-80: LGTM! Correct requeue verification pattern.These assertions correctly verify that a requeue is requested by checking
RequeueAfter > 0. The migration from the booleanRequeuefield to the duration-basedRequeueAfteris appropriate for controller-runtime v0.21.0.Note: See the earlier comment about simplifying the cast.ToDuration usage.
Also applies to: 1361-1361, 1402-1402, 1598-1598, 1717-1717, 1776-1776, 1891-1891
go.mod (2)
11-14: OpenShift 4.20 dependencies look correct.The updates to OpenShift API and library-go from the release-4.20 branches align with the PR objectives. Using commit hashes is appropriate for tracking specific versions from release branches.
155-157: Go 1.24 is backward compatible—no verification of breaking changes needed.Go 1.24 maintains the Go 1 promise of compatibility, with almost all Go programs continuing to compile and run as before. The release arrived in February 2025 and Go 1.24.11 was released on 2025-12-02 with only minor security and bug fixes. Verify that your CI/CD pipelines have Go 1.24 support available if needed for the build environment, but no compatibility testing is required for the codebase itself.
|
Looking at the codebase one more time - these are still many places where the Result.Requeue is being used - space_controller.go, space_cleanup_controller.go, notification_controller.go, usersignup_cleanup_controller.go; and also in some unit test files. Let's delete it from these places as well. |
MatousJobanek
dismissed
their stale review
Let's drop the usage of the Result.Requeue completely
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
controllers/nstemplatetier/nstemplatetier_controller_test.go (1)
112-112: Consider using emptyreconcile.Result{}for consistency.Both assertions use
reconcile.Result{RequeueAfter: 0}to indicate no requeue is needed. While functionally correct, this is inconsistent with lines 55 and 73, which use the more idiomatic emptyreconcile.Result{}for the same purpose. Additionally, the comment "// no reconcile" is imprecise—reconciliation did complete; it's the requeue that's not needed.🔎 Consider these consistency improvements:
For line 112:
- require.Equal(t, reconcile.Result{RequeueAfter: 0}, res) // no reconcile + require.Equal(t, reconcile.Result{}, res) // no requeueFor line 161:
- require.Equal(t, reconcile.Result{RequeueAfter: 0}, res) // no reconcile + require.Equal(t, reconcile.Result{}, res) // no requeueAlso applies to: 161-161
controllers/space/space_controller_test.go (1)
158-158: Consider simplifying assertion pattern for consistency.The explicit
assert.Equal(t, reconcile.Result{RequeueAfter: 0}, res)pattern is more verbose thanassert.Empty(t, res.RequeueAfter), which is used elsewhere in this file and achieves the same result for the common case where onlyRequeueAftermatters.For positive requeue checks, consider using
assert.Positive(t, res.RequeueAfter)instead ofassert.Greater(t, res.RequeueAfter, cast.ToDuration("0s"))for better readability and consistency withnotification_controller_test.go.🔎 Example refactor for simpler assertions:
-assert.Equal(t, reconcile.Result{RequeueAfter: 0}, res) +assert.Empty(t, res.RequeueAfter)-assert.Greater(t, res.RequeueAfter, cast.ToDuration("0s")) +assert.Positive(t, res.RequeueAfter)Also applies to: 177-177, 488-488, 520-520, 600-600, 632-632, 667-667, 759-759, 790-790, 1022-1022, 1109-1109
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
controllers/masteruserrecord/masteruserrecord_controller_test.go(4 hunks)controllers/notification/notification_controller.go(0 hunks)controllers/notification/notification_controller_test.go(6 hunks)controllers/nstemplatetier/nstemplatetier_controller_test.go(2 hunks)controllers/space/space_controller.go(0 hunks)controllers/space/space_controller_test.go(41 hunks)controllers/spacecleanup/space_cleanup_controller.go(1 hunks)controllers/usersignupcleanup/usersignup_cleanup_controller.go(1 hunks)
💤 Files with no reviewable changes (2)
- controllers/space/space_controller.go
- controllers/notification/notification_controller.go
🚧 Files skipped from review as they are similar to previous changes (1)
- controllers/masteruserrecord/masteruserrecord_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: go.mod replacements
- GitHub Check: GolangCI Lint
- GitHub Check: Build & push operator bundles & dashboard image for e2e tests
🔇 Additional comments (7)
controllers/usersignupcleanup/usersignup_cleanup_controller.go (2)
59-59: Documentation updated correctly for RequeueAfter semantics.The comment now accurately reflects the requeue condition using
RequeueAfter > 0instead of the deprecatedRequeueboolean.
110-112: Correctly removed redundant Requeue field.Since
RequeueAfteris already set to a positive duration, the explicitRequeue: truefield was redundant and has been appropriately removed per the deprecation.Also applies to: 179-181
controllers/notification/notification_controller_test.go (1)
83-83: Test assertions correctly migrated to RequeueAfter semantics.The assertions now properly validate reconciliation results using
RequeueAfterduration checks:
assert.Empty(t, result.RequeueAfter)for no requeueassert.Positive(t, result.RequeueAfter)for positive requeueThis aligns with the past review suggestion and the deprecation of
result.Requeue.Also applies to: 112-112, 156-156, 203-203, 264-264, 305-305
controllers/space/space_controller_test.go (1)
24-24: Test assertions successfully migrated to RequeueAfter semantics.The import of
castpackage and all assertion updates correctly implement the migration from the deprecatedresult.Requeueboolean toresult.RequeueAfterduration-based semantics. The changes maintain test coverage while adopting the new API pattern.Also applies to: 80-80, 119-119, 207-207, 232-232, 314-314, 341-341, 368-368, 389-389, 422-422, 455-455, 565-565, 833-833, 875-875, 966-966, 1316-1316, 1358-1358, 1399-1399, 1447-1447, 1489-1489, 1594-1594, 1657-1657, 1713-1713, 1772-1772, 1817-1817, 1835-1835, 1872-1872, 1887-1887, 1935-1935, 1968-1968
controllers/spacecleanup/space_cleanup_controller.go (3)
73-78: LGTM: Correctly migrated to RequeueAfter pattern.The Reconcile method correctly uses the returned
requeueAfterduration inctrl.Result{RequeueAfter: requeueAfter}, which properly replaces the deprecatedresult.Requeueboolean flag. WhenrequeueAfter > 0, the controller will requeue after that duration; whenrequeueAfter == 0with no error, no requeue occurs.
80-114: LGTM: All return statements correctly updated.The method signature change from
(bool, time.Duration, error)to(time.Duration, error)is clean and all return sites are correctly updated:
- Error cases (lines 85, 103) return
(0, err)— controller-runtime handles exponential backoff requeue- No-action cases (lines 90, 97, 107) return
(0, nil)— no requeue needed; the watch on SpaceBinding ensures reconciliation when bindings change- Delayed-action case (line 113) returns
(requeueAfter, nil)— correctly schedules requeue after the remaining threshold time
73-114: All files have been correctly updated to use the new pattern.Verification confirms that all files mentioned in MatousJobanek's comment have been successfully updated:
space_controller.go,notification_controller.go, andusersignup_cleanup_controller.goall useRequeueAfterinstead of the deprecated pattern. No instances of the oldresult.Requeuepattern remain in the codebase.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
controllers/space/space_controller_test.go (1)
153-157: Fix incorrect result variable assertion.Line 153 discards the reconcile result with
_, but line 157 asserts onresfrom line 114 (the outer test scope). This means the test is checking the wrong reconcile result.🔎 Proposed fix
// when -_, err = ctrl.Reconcile(context.TODO(), requestFor(s)) +res, err = ctrl.Reconcile(context.TODO(), requestFor(s)) // then require.NoError(t, err) assert.Equal(t, reconcile.Result{RequeueAfter: 0}, res) // no requeue and status with new provisioned namespaces was updated
🧹 Nitpick comments (1)
controllers/space/space_controller_test.go (1)
24-24: Consider using native Go duration literals instead of cast.ToDuration.While
cast.ToDuration("0s")works, Go's nativetime.Duration(0)or0 * time.Secondis more idiomatic and doesn't require an external dependency for this simple use case.🔎 Proposed refactor
Throughout the file, replace
cast.ToDuration("0s")withtime.Duration(0):-assert.Greater(t, res.RequeueAfter, cast.ToDuration("0s")) +assert.Greater(t, res.RequeueAfter, time.Duration(0))If you make this change, you can also remove the cast import at line 24.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
controllers/masteruserrecord/masteruserrecord_controller_test.gocontrollers/space/space_controller_test.gocontrollers/usersignup/usersignup_controller_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- controllers/usersignup/usersignup_controller_test.go
- controllers/masteruserrecord/masteruserrecord_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). (2)
- GitHub Check: Build & push operator bundles & dashboard image for e2e tests
- GitHub Check: GolangCI Lint
🔇 Additional comments (1)
controllers/space/space_controller_test.go (1)
79-79: LGTM! Assertion migration from deprecated Requeue to RequeueAfter.The test assertions have been correctly migrated from checking the deprecated boolean
res.Requeuefield to checking theres.RequeueAfterduration field. The patterns used are appropriate:
assert.Greater(t, res.RequeueAfter, cast.ToDuration("0s"))for expecting an explicit requeueassert.Empty(t, res.RequeueAfter)for expecting no requeueassert.Equal(t, reconcile.Result{RequeueAfter: 0}, res)for comprehensive result validationThis aligns with the PR objective to remove usage of the deprecated
result.Requeuefield.Also applies to: 118-118, 157-157, 176-176, 206-206, 230-230, 313-313, 339-339, 364-364, 384-384, 416-416, 448-448, 480-480, 511-511, 556-556, 591-591, 623-623, 658-658, 749-749, 779-779, 821-821, 863-863, 954-954, 1010-1010, 1097-1097, 1304-1304, 1346-1346, 1387-1387, 1435-1435, 1477-1477, 1582-1582, 1645-1645, 1701-1701, 1760-1760, 1805-1805, 1823-1823, 1860-1860, 1875-1875, 1923-1923, 1956-1956
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexeykazakov, MatousJobanek, rajivnathan, 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @go.mod:
- Around line 24-31: Update the k8s.io/apiextensions-apiserver module version in
go.mod from v0.33.2 to v0.33.4 to match the other k8s.io modules; after changing
the version string for k8s.io/apiextensions-apiserver, run go mod tidy (or go
get k8s.io/apiextensions-apiserver@v0.33.4) to refresh the go.sum and ensure
dependency graph consistency.
- Line 32: You upgraded the kustomize dependency to
sigs.k8s.io/kustomize/kustomize/v5 v5.6.0; scan repo manifests and
kustomization.yaml usages for deprecated fields (patchesStrategicMerge,
patchesJson6902, commonLabels) and migrate them to the unified patches and
labels fields (or run `kustomize edit fix` to automate migration), and also
audit any embedder or downstream code that imports kyaml/cmd/config/api
(embedding or direct uses) for compatibility with the internal package bumps to
v0.19.0 so you can update imports or adapter code as needed.
- Around line 5-6: The go.mod contains unverifiable pseudo-versions for
github.com/codeready-toolchain/api (commit 54d23a1b4f3c) and
github.com/codeready-toolchain/toolchain-common (commit 94ad63d02d50); verify
those commits exist in their repos and update the pseudo-versions to valid
commits or replace them with the correct tagged versions (or run `go get
module@latest`/`go get module@<commit>` to resolve), and for the openshift/api
entry referenced in the comment, either update the pseudo-version to match the
current release-4.20 HEAD (e.g., replace 1cb53e34ca33 with the actual HEAD
commit) or revise the comment to state you intentionally pinned an older commit;
leave openshift/library-go as-is if it already points to the correct HEAD.
- Line 124: Update the indirect dependency golang.org/x/crypto from v0.37.0 to
v0.45.0 (or later) in go.mod to address the SSH vulnerabilities; run `go get
golang.org/x/crypto@v0.45.0` (or the desired newer version) and then `go mod
tidy` to refresh go.sum and ensure all transitive references are updated; verify
builds/tests to confirm compatibility after the upgrade.
📜 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 (1)
go.mod
⏰ 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 (4)
go.mod (4)
118-122: No compatibility concerns with OpenTelemetry v1.33.0 and controller-runtime v0.21.0.The OpenTelemetry packages at v1.33.0 (otel, otel/metric, otel/trace) and otelgrpc v0.58.0 are compatible with controller-runtime v0.21.0, which already uses these same versions as dependencies.
44-44: No action required—versions are already compatible.The k8s.io/kube-openapi version (v0.0.0-20250318190949-c8a335a9a2ff) is the exact version required by controller-tools v0.18.0 itself, as confirmed in controller-tools' go.mod at the v0.18.0 release tag. The dependency is correctly aligned.
Likely an incorrect or invalid review comment.
16-16: No action required. The prometheus/client_golang v1.22.0 upgrade is safe for this codebase.The actual breaking changes in v1.22.0 are: (1) zstd compression support is now opt-in instead of default, and (2) the minimum Go version requirement is raised to Go 1.22. Neither affects this codebase: there is no zstd usage, and the project already requires Go 1.24.0. The metric registration, histogram, counter, and label handling APIs have no breaking changes; the existing usage patterns in
pkg/metrics/metrics.goremain compatible.
155-155: Verify Go 1.24.0 compatibility with all listed dependencies.The go directive has been bumped to 1.24.0 and toolchain to go1.24.11. Ensure all direct and transitive dependencies (especially controller-runtime v0.21.0 and k8s.io modules v0.33.x) are compatible with Go 1.24 by checking for:
- Cgo-related code: Go 1.24 now strictly rejects methods on cgo-generated types (including via aliases). Review any cgo interactions or C bindings in dependencies.
- Crypto and X.509 changes: Stricter RSA and X.509 validation, and changes to key generation behavior—verify that cryptographic operations and certificate handling remain compatible.
- Runtime changes: New Swiss tables implementation for maps and runtime mutex; while usually transparent, verify performance/behavior if dependencies rely on specific map or synchronization characteristics.
- Platform support: Minimum macOS version is now 11; ensure all build/deployment environments meet platform requirements.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 (1)
go.mod
⏰ 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: Build & push operator bundles & dashboard image for e2e tests
- GitHub Check: Test with Coverage
🔇 Additional comments (3)
go.mod (3)
24-32: Kubernetes module versions are consistently aligned.All primary k8s.io modules (api, client-go, apimachinery, apiextensions-apiserver) are at v0.33.4, and the controller-runtime (v0.21.0) and controller-tools (v0.18.0) updates align with the PR objectives. The comments pinning OpenShift modules to the release-4.20 branch commits are helpful for maintainability.
95-95: New indirect dependencies are justified.New indirect dependencies
kylelemons/godebug(line 95) andsigs.k8s.io/randfill(line 150) appear to be pulled in by the k8s.io ecosystem and kustomize updates respectively. These are legitimate and expected.Also applies to: 150-150
118-122: OpenTelemetry and kustomize dependency versions are compatible with controller-runtime v0.21.0.The module graph resolves cleanly without conflicts. Controller-runtime v0.21.0 directly requires the same OpenTelemetry versions (otel v1.33.0, otel/metric v1.33.0, otel/trace v1.33.0) in this go.mod, and kustomize modules (api, cmd/config, kyaml) are consistently pinned to v0.19.0 across the dependency tree. No duplicate or conflicting versions detected.
|
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your changes status has failed because you have indirect coverage changes. Learn more about Unexpected Coverage Changes and reasons for indirect coverage changes. Additional details and impacted files@@ Coverage Diff @@
## master #1226 +/- ##
===========================================
- Coverage 84.23% 70.15% -14.08%
===========================================
Files 84 84
Lines 6571 6565 -6
===========================================
- Hits 5535 4606 -929
- Misses 819 1747 +928
+ Partials 217 212 -5
🚀 New features to boost your workflow:
|
4 participants
Description
Update dependencies
NOTE THAT
result.Requeueis deprecatedRelated PRs
codeready-toolchain/api#495
codeready-toolchain/toolchain-common#503
codeready-toolchain/member-operator#718
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
Bug Fixes
Behavior
Tests
✏️ Tip: You can customize this high-level summary in your review settings.