CCO-837: Replace deprecated golang/mock with go.uber.org/mock - #1069
CCO-837: Replace deprecated golang/mock with go.uber.org/mock#1069ericahinkleRH wants to merge 1 commit into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@ericahinkleRH: This pull request references CCO-837 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
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:
WalkthroughThe module migrates from ChangesGoMock migration
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ericahinkleRH The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1069 +/- ##
=======================================
Coverage 47.21% 47.21%
=======================================
Files 97 97
Lines 12650 12650
=======================================
Hits 5973 5973
Misses 6017 6017
Partials 660 660
🚀 New features to boost your workflow:
|
|
/retest-required |
866edd0 to
4363f6f
Compare
|
/test e2e-gcp-manual-oidc |
1 similar comment
|
/test e2e-gcp-manual-oidc |
0c8c918 to
3a243dc
Compare
- Migrate from deprecated github.com/golang/mock to go.uber.org/mock - Update go.mod dependencies - Update toolchain to Go 1.26.5 for security fixes - Update google.golang.org/grpc to v1.82.1 for security vulnerabilities - Regenerate mocks using mockgen v0.5.0 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
3a243dc to
8202791
Compare
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/azure/actuator_test.go`:
- Line 34: Remove all explicit gomock.Controller.Finish calls from the tests
using gomock.NewController(t), including pkg/azure/actuator_test.go lines
556-557 and 682-702, pkg/cmd/provisioning/gcp/create_service_accounts_test.go
lines 190-191, and pkg/operator/secretannotator/gcp/reconciler_test.go lines
218-219; rely on the controller’s t.Cleanup registration and leave the test
setup and assertions unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8db0c8f5-dd89-4ef0-9df2-7c6f1f688a76
⛔ Files ignored due to path filters (11)
go.sumis excluded by!**/*.sumvendor/github.com/golang/mock/CONTRIBUTORSis excluded by!**/vendor/**,!vendor/**vendor/go.uber.org/mock/AUTHORSis excluded by!**/vendor/**,!vendor/**vendor/go.uber.org/mock/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/go.uber.org/mock/gomock/call.gois excluded by!**/vendor/**,!vendor/**vendor/go.uber.org/mock/gomock/callset.gois excluded by!**/vendor/**,!vendor/**vendor/go.uber.org/mock/gomock/controller.gois excluded by!**/vendor/**,!vendor/**vendor/go.uber.org/mock/gomock/doc.gois excluded by!**/vendor/**,!vendor/**vendor/go.uber.org/mock/gomock/matchers.gois excluded by!**/vendor/**,!vendor/**vendor/go.uber.org/mock/gomock/string.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (28)
go.modpkg/aws/actuator/actuator_test.gopkg/aws/mock/client_generated.gopkg/azure/actuator_test.gopkg/azure/mock/client_generated.gopkg/cmd/provisioning/aws/create-iam-roles_test.gopkg/cmd/provisioning/aws/create_identity_provider_test.gopkg/cmd/provisioning/azure/create_managed_identities_test.gopkg/cmd/provisioning/azure/create_oidc_issuer_test.gopkg/cmd/provisioning/gcp/create_service_accounts_test.gopkg/cmd/provisioning/gcp/create_workload_identity_pool_test.gopkg/cmd/provisioning/gcp/create_workload_identity_provider_test.gopkg/cmd/provisioning/ibmcloud/create_service_id_test.gopkg/cmd/provisioning/ibmcloud/delete_service_id_test.gopkg/cmd/provisioning/ibmcloud/refresh-keys_test.gopkg/gcp/mock/client_generated.gopkg/ibmcloud/mock/client_generated.gopkg/operator/cleanup/cleanup_controller_test.gopkg/operator/credentialsrequest/credentialsrequest_controller_azure_test.gopkg/operator/credentialsrequest/credentialsrequest_controller_gcp_test.gopkg/operator/credentialsrequest/credentialsrequest_controller_test.gopkg/operator/credentialsrequest/credentialsrequest_controller_vsphere_test.gopkg/operator/credentialsrequest/status_test.gopkg/operator/podidentity/podidentitywebhook_controller_test.gopkg/operator/secretannotator/aws/reconciler_test.gopkg/operator/secretannotator/gcp/reconciler_test.gopkg/operator/secretannotator/vsphere/reconciler_test.gopkg/operator/utils/gcp/utils_test.go
🚧 Files skipped from review as they are similar to previous changes (14)
- pkg/ibmcloud/mock/client_generated.go
- pkg/operator/podidentity/podidentitywebhook_controller_test.go
- pkg/operator/credentialsrequest/credentialsrequest_controller_azure_test.go
- pkg/cmd/provisioning/azure/create_oidc_issuer_test.go
- pkg/operator/credentialsrequest/credentialsrequest_controller_vsphere_test.go
- pkg/operator/secretannotator/aws/reconciler_test.go
- pkg/cmd/provisioning/aws/create_identity_provider_test.go
- pkg/cmd/provisioning/ibmcloud/refresh-keys_test.go
- pkg/cmd/provisioning/aws/create-iam-roles_test.go
- pkg/operator/utils/gcp/utils_test.go
- pkg/cmd/provisioning/ibmcloud/delete_service_id_test.go
- pkg/cmd/provisioning/gcp/create_workload_identity_pool_test.go
- pkg/cmd/provisioning/ibmcloud/create_service_id_test.go
- pkg/gcp/mock/client_generated.go
| log "github.com/sirupsen/logrus" | ||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| "go.uber.org/mock/gomock" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in pkg/azure/actuator_test.go pkg/cmd/provisioning/gcp/create_service_accounts_test.go pkg/operator/secretannotator/gcp/reconciler_test.go; do
echo "===== $f ====="
nl -ba "$f" | sed -n '1,260p'
echo
doneRepository: openshift/cloud-credential-operator
Length of output: 252
🌐 Web query:
go.uber.org/mock gomock NewController testing.T auto cleanup Finish idempotent documentation
💡 Result:
In GoMock (including the version at go.uber.org/mock), the gomock.NewController(t) function automatically handles cleanup for you when provided with a *testing.T, starting from Go 1.14 [1][2][3]. Key details regarding this functionality: Automatic Cleanup: When you pass a *testing.T (or any type implementing the Cleanup method) to NewController, the library registers a cleanup function using t.Cleanup [1][2]. This function triggers ctrl.Finish automatically at the end of the test, removing the need to manually call defer ctrl.Finish [1][2][3]. Idempotency and Multiple Calls: The Finish method itself is not inherently idempotent [1][2]. Internally, it uses a flag (ctrl.finished) to track state [1][2]. If Finish is called more than once, it will trigger a fatal error (via T.Fatalf) to notify you of the misuse, unless it is being handled by the automatic cleanup mechanism which manages the state safely [1][2]. Best Practice: Because of the automatic cleanup provided by Go 1.14+, calling ctrl.Finish manually is now considered unnecessary and is often marked as deprecated [4]. You should rely on the automatic invocation handled by the controller via t.Cleanup to ensure test expectations are verified correctly without manual intervention [3].
Citations:
- 1: https://github.com/golang/mock/blob/674a1c4a/gomock/controller.go
- 2: https://github.com/golang/mock/blob/v1.6.0/gomock/controller.go
- 3: https://deepwiki.com/golang/mock/2.1-controller
- 4: Deprecate the function ctrl.Finish() uber-go/mock#50
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
files = [
"pkg/azure/actuator_test.go",
"pkg/cmd/provisioning/gcp/create_service_accounts_test.go",
"pkg/operator/secretannotator/gcp/reconciler_test.go",
]
for p in files:
text = Path(p).read_text()
for needle in ["NewController(", ".Finish()", "defer ctrl.Finish()", "gomock.NewController("]:
if needle in text:
print(f"== {p} : {needle} ==")
for i, line in enumerate(text.splitlines(), 1):
if needle in line:
print(f"{i}: {line}")
print()
PYRepository: openshift/cloud-credential-operator
Length of output: 1265
🌐 Web query:
go.uber.org/mock gomock NewController finish cleanup t.Cleanup documentation
💡 Result:
In Gomock, specifically when using go.uber.org/mock/gomock, you generally do not need to explicitly call ctrl.Finish() if you are using Go 1.14 or later and passing *testing.T (or any testing.TB implementation that supports Cleanup) to gomock.NewController(t) [1][2][3]. When you provide a *testing.T to NewController, the library checks if the provided reporter implements a Cleanup method [1][4]. If it does, gomock registers its internal finish method with t.Cleanup() [1]. This ensures that Finish()—which validates that all expected mock calls occurred—is automatically executed when the test completes, regardless of whether the test passes or fails (or panics) [1][2]. If you are using an older version of Go (pre-1.14) or a test reporter that does not support Cleanup, you must continue to manually call defer ctrl.Finish() to ensure your mock expectations are verified [2][5]. Key points to remember: 1. Automatic Cleanup: Only occurs if t passed to NewController supports Cleanup (added in Go 1.14) [1][2]. 2. Idempotency: ctrl.Finish() is designed to be idempotent; calling it multiple times will not cause issues, as the controller tracks whether it has already finished [1][4]. 3. Best Practice: While automatic cleanup reduces boilerplate, deferring Finish() remains a safe and compatible pattern across all versions of Go and Gomock [2][6].
Citations:
- 1: https://github.com/golang/mock/blob/674a1c4a/gomock/controller.go
- 2: https://deepwiki.com/golang/mock/2.1-controller
- 3: https://github.com/golang/mock
- 4: https://github.com/golang/mock/blob/v1.6.0/gomock/controller.go
- 5: https://github.com/golang/mock/blob/master/README.md
- 6: Automatic call to Controller.Finish breaks "stub" style tests that worked on prior version golang/mock#584
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== go.mod gomock dependency =="
grep -nE 'go\.uber\.org/mock|golang\.org/x' go.mod || true
echo
echo "== Go env module cache =="
go env GOMODCACHE GOPATH GOROOT | sed 's/^/ /'
echo
modcache="$(go env GOMODCACHE)"
echo "== Cached go.uber.org/mock files =="
fd -a 'controller.go' "$modcache" 2>/dev/null | rg 'go\.uber\.org/mock/.*/gomock/controller\.go$' || trueRepository: openshift/cloud-credential-operator
Length of output: 822
🌐 Web query:
site:github.com/go.uber.org/mock gomock/controller.go Finish idempotent t.Cleanup source
💡 Result:
In go.uber.org/mock/gomock, Controller.Finish() is not idempotent and should only be called once. If you pass *testing.T to NewController, it registers a t.Cleanup that calls Finish() automatically after the test and subtests complete, so you usually should not call ctrl.Finish() yourself. (pkg.go.dev)
If you want, I can also point you to the exact controller.go logic in the repo.
Citations:
🌐 Web query:
"go.uber.org/mock/gomock" Finish idempotent Cleanup source controller.go
💡 Result:
The query appears to conflate two distinct concepts in Go development: the gomock library (now maintained as go.uber.org/mock) and Kubernetes controller patterns for idempotent cleanup. 1. gomock and Controller.Finish: In the gomock library (go.uber.org/mock), the Controller.Finish method is used to verify that all expected method calls occurred [1][2]. Historically, it was not idempotent and required explicit calling [2][3]. However, since Go 1.14, if you pass a *testing.T to gomock.NewController(t), the library automatically registers a cleanup function using t.Cleanup to call Finish for you [2][3]. This removes the need for manual invocation in standard tests [3]. 2. Kubernetes Controller Idempotent Cleanup: In the context of Kubernetes controllers (e.g., source-controller.go, often built with controller-runtime), "idempotent cleanup" typically refers to the implementation of Finalizers [4][5]. When a resource is marked for deletion (DeletionTimestamp is non-zero), the controller must perform cleanup tasks (like removing external cloud resources) in an idempotent manner so that the logic can safely run multiple times without side effects [4]. Once the cleanup is confirmed successful, the finalizer is removed from the resource to allow Kubernetes to complete the deletion [4][5]. The phrase "Finish idempotent Cleanup source controller.go" likely combines a search for gomock's Finish mechanism with the common requirement to write idempotent cleanup logic within a Kubernetes source-controller.
Citations:
- 1: https://go.pact.im/doc/pkg/go.uber.org/mock/gomock.html
- 2: https://github.com/golang/mock/blob/v1.6.0/gomock/controller.go
- 3: Use t.Cleanup (added in go1.14) to call
Finishautomatically golang/mock#407 - 4: https://book.kubebuilder.io/reference/using-finalizers
- 5: https://github.com/borghei/Claude-Skills/blob/main/engineering/kubernetes-operator/references/controller-runtime-patterns.md
🌐 Web query:
go.uber.org/mock README Finish defer ctrl.Finish safe double call t.Cleanup
💡 Result:
When using go.uber.org/mock (or the original golang/mock), calling ctrl.Finish() is used to verify that all expectations set on a mock controller have been met [1][2]. For Go 1.14 and later, when you pass a *testing.T (or any testing.TB) to gomock.NewController(t), the library automatically registers a t.Cleanup() function to call ctrl.Finish() [1][3][4]. Because Finish() is designed to be safe to call multiple times, this automatic cleanup eliminates the need for an explicit defer ctrl.Finish() in your test code [1][3][4]. If you call ctrl.Finish() manually and it is also called by t.Cleanup, the implementation handles the double call safely [1][3][4]. While some earlier development efforts in uber-go/mock attempted to mark ctrl.Finish() as deprecated to encourage this automatic behavior, community feedback noted that explicit calls are still necessary for certain specific test frameworks, so the function remains available for use [5][6].
Citations:
- 1: https://github.com/golang/mock/blob/main/README.md
- 2: https://context7.com/uber-go/mock/llms.txt?tokens=10000
- 3: Use t.Cleanup (added in go1.14) to call
Finishautomatically golang/mock#407 - 4: https://deepwiki.com/golang/mock/2.1-controller
- 5: Add new linter gomockcontrollerfinish golangci/golangci-lint#4202
- 6: Deprecate the function ctrl.Finish() uber-go/mock#50
🌐 Web query:
go.uber.org/mock v0.6.0 controller.go Finish called multiple times safe idempotent
💡 Result:
The Controller.Finish method in go.uber.org/mock v0.6.0 is not idempotent [1]. If it is called more than once, the library will trigger a failure [2][1]. Specifically, the implementation in controller.go checks the internal finished state; if Finish has already been called, it uses the testing reporter to call t.Fatalf, which aborts the test with the error message: "Controller.Finish was called more than once. It has to be called exactly once." [2] To avoid this, you should rely on the automatic cleanup provided by the library [1]. When you pass a *testing.T (or a compatible TestReporter) into gomock.NewController, the controller registers a cleanup function that automatically invokes Finish when the test and its subtests complete [3][1]. Consequently, explicit calls to Finish are unnecessary in modern Go tests [3][1].
Citations:
- 1: https://go.pact.im/doc/pkg/go.uber.org/mock/gomock.html
- 2: https://go.pact.im/doc/src/go.uber.org/mock/gomock/controller.go.html
- 3: https://context7.com/uber-go/mock/llms.txt
🌐 Web query:
site:github.com/uber-go/mock controller.go "finish" "Cleanup" "finished"
💡 Result:
The phrase "Cleanup finished" does not appear to be a standard or recognizable log message associated with controller.go in prominent Go libraries like gomock or Kubernetes controller-runtime. In the context of the uber-go/mock (gomock) library, there is a prominent function named Controller.Finish defined in controller.go [1]. This function is used in testing to verify that all expected mock calls have been satisfied [2][3]. Regarding "Cleanup": - Modern Go (since 1.14) provides t.Cleanup in the testing package, which allows tests to register cleanup functions that run automatically when the test finishes [2]. - In gomock, the Controller.Finish method was marked as deprecated because it is often unnecessary to call manually when t.Cleanup is used to automatically handle cleanup, although it remains available for specific test framework requirements [2][3]. If you are seeing a specific log message "Cleanup finished" in a file named controller.go, it is likely custom logging code within your specific project or a library not standard to common Go infrastructure, rather than a built-in message from standard Go mocking or controller libraries. You should check the source code for that string to identify which logger or function is outputting it.
Citations:
- 1: https://github.com/uber-go/mock/tree/main/gomock
- 2: Deprecate the function ctrl.Finish() uber-go/mock#50
- 3: deprecation of
Controller.Finishbreaks certain test frameworks uber-go/mock#81
Remove the explicit Finish() calls gomock.NewController(t) already registers cleanup via t.Cleanup, so these extra finalizers can trip Controller.Finish was called more than once in these tests.
pkg/azure/actuator_test.go#L556-L557,pkg/azure/actuator_test.go#L682-L702pkg/cmd/provisioning/gcp/create_service_accounts_test.go#L190-L191pkg/operator/secretannotator/gcp/reconciler_test.go#L218-L219
📍 Affects 3 files
pkg/azure/actuator_test.go#L34-L34(this comment)pkg/cmd/provisioning/gcp/create_service_accounts_test.go#L12-L12pkg/operator/secretannotator/gcp/reconciler_test.go#L25-L25
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/azure/actuator_test.go` at line 34, Remove all explicit
gomock.Controller.Finish calls from the tests using gomock.NewController(t),
including pkg/azure/actuator_test.go lines 556-557 and 682-702,
pkg/cmd/provisioning/gcp/create_service_accounts_test.go lines 190-191, and
pkg/operator/secretannotator/gcp/reconciler_test.go lines 218-219; rely on the
controller’s t.Cleanup registration and leave the test setup and assertions
unchanged.
Source: MCP tools
|
/test security |
1 similar comment
|
/test security |
|
/test e2e-azure-manual-oidc |
|
/test security |
|
@ericahinkleRH: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Summary
Replace deprecated
golang/mockwithgo.uber.org/mockas the maintained fork.The golang/mock repository is no longer maintained and recommends migrating to go.uber.org/mock.
Changes
[]anyinstead of[]*gomock.Call)Testing
Jira
https://redhat.atlassian.net/browse/CCO-837
Notes
This PR is independent and does NOT depend on CCO-834 (K8s 1.36 upgrade).
Summary by CodeRabbit
Chores
Tests