Upgrade controller-runtime, address breaking changes, and fix linter issues#905
Conversation
adamkasztenny
force-pushed
the
upgrade-controller-runtime
branch
2 times, most recently
from
2c0fb48 to
837d3a7
Compare
adamkasztenny
force-pushed
the
upgrade-controller-runtime
branch
2 times, most recently
from
fe53e56 to
6e789c1
Compare
This upgrade required several changes to address breaking changes and new linter requirements: - Update go.mod dependencies for controller-runtime v0.21.0 - Fix gocritic deprecatedComment lint issues by adding blank lines before Deprecated: comments - Fix godoclint duplicate package doc comments - Fix staticcheck warnings for deprecated webhook interfaces by adding assertions for new admission.Defaulter and admission.Validator interfaces - Add revive linter exclusion for the errors package (intentionally shadows stdlib) - Apply golangci-lint auto-fixes (slices.Contains, maps.Copy, etc.) - Fix flaky test by sorting GVKs by full key (Group, Version, Kind) Created with Cursor using Claude 4.5 Opus Signed-off-by: Adam Kasztenny <adam.kasztenny@elastic.co>
adamkasztenny
force-pushed
the
upgrade-controller-runtime
branch
from
6e789c1 to
552d14c
Compare
📝 WalkthroughWalkthroughAdds SubResource Apply support (wrapperStatusClient.Apply and mock/test support), updates webhook tests to assert controller-runtime admission interfaces, adds a golangci-lint exclusion for Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Would you like a follow-up PR to remove the nolint comments once callers are fully migrated to the new admission interfaces? 🚥 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. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pkg/gate/gate_test.go (1)
251-262:⚠️ Potential issue | 🔴 Criticalsync.WaitGroup doesn't have a Go method—this won't compile.
The loop on line 257 calls
wg.Go(), butsync.WaitGrouponly providesAdd(),Done(), andWait(). Did you mean to useerrgroup.Group(which would require importinggolang.org/x/sync/errgroup), or would you prefer to stick withsync.WaitGroupand adjust to theAdd/Donepattern?🛠️ Suggested fix (sync.WaitGroup)
for range numGoroutines { - wg.Go(func() { + wg.Add(1) + go func() { + defer wg.Done() g.Register(func() { callCount <- struct{}{} }, "shared-condition") - }) + }() }pkg/reconciler/managed/reconciler_modern_test.go (1)
1946-1985:⚠️ Potential issue | 🟡 MinorTest name says Create-only, but setup uses ManagementActionAll.
ManagementPolicyCreateCreateSuccessfulsetsManagementActionAll, which overlaps the All case and doesn’t exercise Create-only behavior. Did you intend this to validate Create-only? If so, switching toManagementActionCreate(or renaming the case) would make the intent clear. Thanks!Potential alignment with the case name
- MockGet: test.NewMockGetFn(nil, func(obj client.Object) error { - mg := asModernManaged(obj, 42) - mg.SetManagementPolicies(xpv1.ManagementPolicies{xpv1.ManagementActionAll}) + MockGet: test.NewMockGetFn(nil, func(obj client.Object) error { + mg := asModernManaged(obj, 42) + mg.SetManagementPolicies(xpv1.ManagementPolicies{xpv1.ManagementActionCreate}) return nil }), @@ - want := newModernManaged(42) - want.SetManagementPolicies(xpv1.ManagementPolicies{xpv1.ManagementActionAll}) + want := newModernManaged(42) + want.SetManagementPolicies(xpv1.ManagementPolicies{xpv1.ManagementActionCreate})As per coding guidelines:
**/*_test.go: "Check for proper test case naming and reason fields."pkg/test/fake.go (1)
399-431:⚠️ Potential issue | 🟠 MajorWire Apply mocks through MockClient to complete the implementation and avoid nil panics.
The
Applymethod was added toMockSubResourceClient, but the wiring is incomplete.Status()andSubResource()don't wire theApplyfield, andMockClientlacksMockStatusApplyandMockSubResourceApplyfields or defaults. If any code callsStatus().Apply()orSubResource().Apply(), it will panic on a nil function.The suggested wiring (fields, defaults, and method updates) follows the existing pattern for other mock methods—thank you for including that detail! This would keep Apply configurable and safe, just like the other sub-resource operations.
Suggested wiring (fields, defaults, and helpers)
// A MockSubResourceApplyFn is used to mock client.SubResourceClient's apply implementation. type MockSubResourceApplyFn func(ctx context.Context, obj runtime.ApplyConfiguration, opts ...client.SubResourceApplyOption) error + +// NewMockSubResourceApplyFn returns a MockSubResourceApplyFn that returns the supplied error. +func NewMockSubResourceApplyFn(err error, afn ...ApplyFn) MockSubResourceApplyFn { + return func(_ context.Context, obj runtime.ApplyConfiguration, _ ...client.SubResourceApplyOption) error { + for _, fn := range afn { + if fnErr := fn(obj); fnErr != nil { + return fnErr + } + } + return err + } +} @@ type MockClient struct { MockGet MockGetFn @@ MockStatusCreate MockSubResourceCreateFn MockStatusUpdate MockSubResourceUpdateFn MockStatusPatch MockSubResourcePatchFn + MockStatusApply MockSubResourceApplyFn @@ MockSubResourceGet MockSubResourceGetFn MockSubResourceCreate MockSubResourceCreateFn MockSubResourceUpdate MockSubResourceUpdateFn MockSubResourcePatch MockSubResourcePatchFn + MockSubResourceApply MockSubResourceApplyFn @@ func NewMockClient() *MockClient { return &MockClient{ @@ MockStatusUpdate: NewMockSubResourceUpdateFn(nil), MockStatusPatch: NewMockSubResourcePatchFn(nil), + MockStatusApply: NewMockSubResourceApplyFn(nil), @@ + MockSubResourceApply: NewMockSubResourceApplyFn(nil), @@ func (c *MockClient) Status() client.SubResourceWriter { return &MockSubResourceClient{ MockCreate: c.MockStatusCreate, MockUpdate: c.MockStatusUpdate, MockPatch: c.MockStatusPatch, + MockApply: c.MockStatusApply, } } @@ func (c *MockClient) SubResource(_ string) client.SubResourceClient { return &MockSubResourceClient{ MockGet: c.MockSubResourceGet, MockCreate: c.MockSubResourceCreate, MockUpdate: c.MockSubResourceUpdate, MockPatch: c.MockSubResourcePatch, + MockApply: c.MockSubResourceApply, } }
🤖 Fix all issues with AI agents
In `@pkg/resource/unstructured/client.go`:
- Around line 209-217: Change the error returned by wrapperStatusClient.Apply
when obj is a Wrapper to be user‑actionable: update the message in the Apply
method (where it currently checks "if u, ok := obj.(Wrapper); ok") to explain
the operation the user attempted (applying a configuration to an unstructured
object), suggest a concrete next step (e.g., use a typed
runtime.ApplyConfiguration or use client.Patch with ApplyPatchType), and avoid
developer jargon; keep references to Wrapper and runtime.ApplyConfiguration so
users know the alternatives.
🧹 Nitpick comments (2)
pkg/resource/unstructured/client_test.go (1)
83-107: Consider addingreasonfields to the table-driven cases.These cases don’t include a
reasonstring, so failures lose intent. Would you be open to addingreasonand printing it in the error output (like other tests in this repo) to improve clarity? Thanks!Example pattern (apply across other cases if you agree)
- cases := map[string]struct { - c client.Client - args args - want error - }{ + cases := map[string]struct { + reason string + c client.Client + args args + want error + }{ "Unwrapped": { + reason: "Unstructured objects should pass through unchanged", c: &test.MockClient{MockGet: test.NewMockGetFn(nil, func(obj client.Object) error { if obj.(metav1.Object).GetName() != nameUnwrapped { return errWrapped } return nil })}, args: args{obj: NewUnwrapped()}, }, @@ - if diff := cmp.Diff(tc.want, got, test.EquateErrors()); diff != "" { - t.Errorf("\nc.Get(...): -want error, +got error:\n %s", diff) + if diff := cmp.Diff(tc.want, got, test.EquateErrors()); diff != "" { + t.Errorf("\nReason: %s\nc.Get(...): -want error, +got error:\n %s", tc.reason, diff) }As per coding guidelines:
**/*_test.go: "Check for proper test case naming and reason fields."pkg/webhook/mutator_test.go (1)
81-83: Consider usingcmpopts.EquateErrors()for consistency with test guidelines.The coding guidelines specify using
cmp.Diffwithcmpopts.EquateErrors()for error testing in test files. The current code usestest.EquateErrors(), which is a Crossplane-specific function with different semantics—it checks error type and string equality, whereascmpopts.EquateErrors()useserrors.Issemantics for wrapped errors. If the wrapped error behavior is what you need, we'd recommend aligning with the standard pattern.Suggested change
import ( "context" "testing" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/controller-runtime/pkg/webhook" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" - - "github.com/crossplane/crossplane-runtime/v2/pkg/test" ) @@ - if diff := cmp.Diff(tc.err, err, test.EquateErrors()); diff != "" { + if diff := cmp.Diff(tc.err, err, cmpopts.EquateErrors()); diff != "" { t.Errorf("\n%s\nDefault(...): -want, +got\n%s\n", tc.reason, diff) }
adamkasztenny
changed the title
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Adam Kasztenny <adam.kasztenny@elastic.co>
adamkasztenny
force-pushed
the
upgrade-controller-runtime
branch
from
b682fd0 to
1182b3b
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Created with Cursor using Claude 4.5 Opus Signed-off-by: Adam Kasztenny <adam.kasztenny@elastic.co>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
I think the lint/test failures are because the |
Regenerate gomod2nix.toml to reflect the upgraded controller-runtime dependency and fix Nix build failures. Created with Cursor using Claude 4.5 Opus Signed-off-by: Adam Kasztenny <adam.kasztenny@elastic.co>
|
@adamwg Fixed, can you run CI again? |
adamwg
left a comment
adamwg
left a comment
There was a problem hiding this comment.
lgtm, thanks for working through all the linter fixes!
|
Thanks @adamwg! Should we wait for a release to resolve the issues on our end or can we just use the latest commit in our go.mod? |
|
It's not uncommon for folks to update their go.mod to pick up latest from main on crossplane-runtime, if that works for your needs. Otherwise, we should have the v2.2 RC published early next week and the full v2.2 release week of Feb 17 |
3 participants
Description of your changes
The breaking change is described in kubernetes-sigs/controller-runtime#3321.
Turns out this is a bit of a rabbit hole since Go also had to be upgraded, followed by golangci-lint, which caused a lot of linter failures which had to be fixed. Some lint failures were auto-fixed, others needed changes.
AI disclosure: Some commits were created with Cursor using Claude 4.5 Opus.
Fixes #904.
I have:
earthly +reviewableto ensure this PR is ready for review.[ ] Added or updated unit tests.[ ] Linked a PR or a [docs tracking issue] to [document this change].[ ] Addedbackport release-x.ylabels to auto-backport this PR.