NO-ISSUE: Update fake clientsets to NewClientset#1340
NO-ISSUE: Update fake clientsets to NewClientset#1340openshift-merge-bot[bot] merged 1 commit intoopenshift:mainfrom
Conversation
WalkthroughReplaces fake client constructors in three tests and updates security test assertions to normalize and clear ManagedFields before comparing API actions and SecurityContextConstraints; adds a helper to strip ManagedFields from recorded Update actions. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/cvo/cvo_test.go (1)
506-515:⚠️ Potential issue | 🟠 MajorComplete the fake clientset migration in this file.
The PR updates some test cases in
pkg/cvo/cvo_test.goto use the new fake clientset constructors, but the file still contains 32 deprecated constructor calls that need to be replaced. This incomplete migration will prevent the lint failure from being resolved. Replace all remaining instances offake.NewSimpleClientset(...)andkfake.NewSimpleClientset(...)throughout the file with the corresponding new constructors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cvo/cvo_test.go` around lines 506 - 515, Search pkg/cvo/cvo_test.go for all uses of fake.NewSimpleClientset(...) and kfake.NewSimpleClientset(...) and replace them with the new constructors used elsewhere in the file (e.g., fake.NewClientset(...) and kfake.NewClientset(...)) so every test uses the updated fake clientset API; update each test case that currently calls fake.NewSimpleClientset or kfake.NewSimpleClientset to call the corresponding NewClientset variant and keep the same object arguments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pkg/cvo/cvo_test.go`:
- Around line 506-515: Search pkg/cvo/cvo_test.go for all uses of
fake.NewSimpleClientset(...) and kfake.NewSimpleClientset(...) and replace them
with the new constructors used elsewhere in the file (e.g.,
fake.NewClientset(...) and kfake.NewClientset(...)) so every test uses the
updated fake clientset API; update each test case that currently calls
fake.NewSimpleClientset or kfake.NewSimpleClientset to call the corresponding
NewClientset variant and keep the same object arguments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 729a9d84-ba95-410c-b822-9bebdf36370f
📒 Files selected for processing (3)
lib/resourceapply/security_test.gopkg/cvo/cvo_test.gopkg/featuregates/featurechangestopper_test.go
| for _, tc := range tests { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| newClient := securityfake.NewSimpleClientset() | ||
| newClient := securityfake.NewClientset() |
There was a problem hiding this comment.
Trying to understand this change, the Godocs say:
Deprecated: NewClientset replaces this with support for field management, which significantly improves server side apply testing. NewClientset is only available when apply configurations are generated (e.g. via
--with-applyconfig).
so... we don't need to worry about that "...only available...", because we're not generating the client library? And... we don't care about field-management support, but aren't hurt by it either? Am I understanding the pivot correctly?
There was a problem hiding this comment.
Looks like we do need to update some fixtures, or clear managedFields before comparing with fixtures, or some such:
github.com/openshift/cluster-version-operator/lib/resourceapply: TestApplySecurityContextConstraintsv1/create_nonexistent,_expect_modified expand_less 0s
{Failed === RUN TestApplySecurityContextConstraintsv1/create_nonexistent,_expect_modified
security_test.go:187: SCC differs from expected:
&v1.SecurityContextConstraints{
TypeMeta: {Kind: "SecurityContextConstraints", APIVersion: "security.openshift.io/v1"},
ObjectMeta: v1.ObjectMeta{
... // 12 identical fields
OwnerReferences: nil,
Finalizers: nil,
- ManagedFields: nil,
+ ManagedFields: []v1.ManagedFieldsEntry{
+ {
+ Manager: "unknown",
+ Operation: "Update",
+ APIVersion: "security.openshift.io/v1",
+ Time: s"2026-03-09 17:57:27.879975246 +0000 UTC",
+ FieldsType: "FieldsV1",
+ FieldsV1: s`{"f:allowedCapabilities":{},"f:fsGroup":{"f:type":{}},"f:metadat`...,
+ },
+ },
},
Priority: nil,
AllowPrivilegedContainer: false,
... // 23 identical fields
}
There was a problem hiding this comment.
Pushed a change to clear the managed fields before comparing. IMO it's not the job of this test suite to be comparing what happens in the managed fields
JoelSpeed
force-pushed
the
update-golangci-lint
branch
from
b3c8484 to
d5f7201
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/cvo/cvo_test.go`:
- Line 506: Multiple test files still call the deprecated NewSimpleClientset;
replace every occurrence of NewSimpleClientset(...) with fake.NewClientset(...)
(preserving the same arguments) across the tests (e.g., the remaining uses in
cvo_test and upgradeable_test) so all fake client construction uses the new
NewClientset symbol; ensure imports remain correct and run tests/linter to
verify no deprecated usages remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 97aa1a2f-3bc6-4a23-9582-1c602dd03893
📒 Files selected for processing (3)
lib/resourceapply/security_test.gopkg/cvo/cvo_test.gopkg/featuregates/featurechangestopper_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/featuregates/featurechangestopper_test.go
| namespace: "test", | ||
| name: "default", | ||
| client: fake.NewSimpleClientset( | ||
| client: fake.NewClientset( |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all remaining usages of NewSimpleClientset in the codebase
rg -n "NewSimpleClientset" --type goRepository: openshift/cluster-version-operator
Length of output: 3464
Incomplete migration: Dozens of NewSimpleClientset instances remain.
This change correctly updates fake.NewSimpleClientset to fake.NewClientset at lines 506, 563, and 4233, but the migration is far more incomplete than initially apparent. A comprehensive search reveals 33 additional instances of the deprecated NewSimpleClientset function still in use across the test files:
pkg/cvo/cvo_test.go: 32 instances at lines 436, 2039, 2324, 2340, 2384, 2427, 2471, 2535, 2603, 2664, 2705, 2842, 2857, 2900, 2953, 3014, 3078, 3144, 3223, 3257, 3293, 3341, 3385, 3429, 3473, 3517, 3562, 3603, 3653, 3698, 3747, 3786pkg/cvo/upgradeable_test.go:143: 1 instance
These remaining usages will continue to trigger golangci-lint deprecation warnings and should all be updated to NewClientset() for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/cvo/cvo_test.go` at line 506, Multiple test files still call the
deprecated NewSimpleClientset; replace every occurrence of
NewSimpleClientset(...) with fake.NewClientset(...) (preserving the same
arguments) across the tests (e.g., the remaining uses in cvo_test and
upgradeable_test) so all fake client construction uses the new NewClientset
symbol; ensure imports remain correct and run tests/linter to verify no
deprecated usages remain.
|
@JoelSpeed: 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JoelSpeed, wking 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 |
|
@wking: Overrode contexts on behalf of wking: ci/prow/e2e-agnostic-ovn, ci/prow/e2e-agnostic-ovn-techpreview-serial-1of3 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 kubernetes-sigs/prow repository. |
openshift-ci
bot
added
the
approved
wking
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@JoelSpeed: This pull request explicitly references no jira issue. 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. |
|
/verified by the e2e CI we're adjusting still passing |
openshift-ci-robot
added
the
verified
|
@wking: This PR has been marked as verified by 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. |
|
Don't wait for the expensive retries; none of the changes that have merged into the main branch since the previous tests will have broken this fix. /override ci/prow/e2e-agnostic-operator |
|
@wking: Overrode contexts on behalf of wking: ci/prow/e2e-agnostic-operator, ci/prow/e2e-agnostic-ovn, ci/prow/e2e-agnostic-ovn-techpreview-serial-1of3, ci/prow/e2e-agnostic-ovn-techpreview-serial-2of3, ci/prow/e2e-agnostic-ovn-techpreview-serial-3of3, ci/prow/e2e-agnostic-ovn-upgrade-into-change, ci/prow/e2e-agnostic-ovn-upgrade-out-of-change, ci/prow/e2e-aws-ovn-techpreview, ci/prow/e2e-hypershift, ci/prow/e2e-hypershift-conformance 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 kubernetes-sigs/prow repository. |
3 participants
Running the latest golangci-lint against CVO produces only a handful of errors about a deprecated
NewSimpleClientsetfunction, this fixes this so that we can update golangci-lint to latest (v2.11.2)CC @DavidHurta
Summary by CodeRabbit