✨ Generate and use ApplyConfiguration types for Server-Side Apply#2515
✨ Generate and use ApplyConfiguration types for Server-Side Apply#2515joelanford wants to merge 2 commits intooperator-framework:mainfrom
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
| GroupVersion = schema.GroupVersion{Group: "olm.operatorframework.io", Version: "v1"} | ||
|
|
||
| // SchemeGroupVersion is an alias for GroupVersion, required by the | ||
| // generated apply configuration code. | ||
| SchemeGroupVersion = GroupVersion |
There was a problem hiding this comment.
Does GroupVersion change at all after initialization, same with SchemeGroupVersion. Are these treated as constants?
There was a problem hiding this comment.
Assumed to be constant. If something changes it, I'd consider it a bug.
There was a problem hiding this comment.
Shame golang won't let you declare them as a constant.
There was a problem hiding this comment.
Pull request overview
This PR generates typed ApplyConfiguration structs for all OLM API types (ClusterCatalog, ClusterExtension, and ClusterExtensionRevision) and migrates the Boxcutter component from the deprecated client.Patch(ctx, rev, client.Apply, ...) pattern to the typed client.Apply(ctx, rev, ...) method for Server-Side Apply.
Changes:
- Generate ApplyConfiguration types using controller-gen's
applyconfigurationgenerator - Add
+genclientand+genclient:nonNamespacedmarkers to cluster-scoped API types - Migrate
ClusterExtensionRevisionGeneratorinterface and implementations to return AC types - Update
PhaseSortand helper functions to operate on AC types natively - Replace
client.Patchwithclient.Applythroughout Boxcutter and BoxcutterStorageMigrator - Set owner references via
metav1ac.OwnerReference()builder instead ofcontrollerutil.SetControllerReference
Reviewed changes
Copilot reviewed 38 out of 38 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| api/v1/groupversion_info.go | Add AC generation markers and SchemeGroupVersion alias |
| api/v1/clustercatalog_types.go | Add +genclient markers for AC generation |
| api/v1/clusterextension_types.go | Add +genclient markers for AC generation |
| api/v1/clusterextensionrevision_types.go | Add +genclient markers for AC generation |
| api/v1/ac/*.go | Generated ApplyConfiguration types for all API types |
| internal/operator-controller/applier/phase.go | Update PhaseSort to use AC types with pointer returns |
| internal/operator-controller/applier/phase_test.go | Update test expectations to use AC types |
| internal/operator-controller/applier/boxcutter.go | Migrate to typed Apply, update generator interface |
| internal/operator-controller/applier/boxcutter_test.go | Update mocks and expectations for AC types |
| go.mod | Move structured-merge-diff from indirect to direct dependency |
| Makefile | Add AC generation step to generate target |
Comments suppressed due to low confidence (3)
internal/operator-controller/applier/phase.go:137
- Potential nil pointer dereference: The function
compareClusterExtensionRevisionObjectsaccessesa.Objectandb.Objectwithout checking if they are nil. If an ApplyConfiguration object is created without callingWithObject(), this will cause a panic. Consider adding nil checks at the beginning of the function to handle this edge case gracefully.
internal/operator-controller/applier/phase.go:149 - Potential nil pointer dereference: The
PhaseSortfunction accessesobj.Object.GroupVersionKind()on line 147 without checking ifobj.Objectis nil. This could panic if an ApplyConfiguration object is created without setting the Object field. Consider adding validation to skip or handle objects with nil Object fields.
internal/operator-controller/applier/boxcutter.go:670 - Potential nil pointer dereference: The function
revisionManagementPermsdereferencesrev.GetName()on line 662 without checking if it returns nil. WhileGetName()ensures the ObjectMeta exists, it can still return a nil pointer if the Name was never set. Consider adding a nil check or ensuring the name is always set before calling this function.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2515 +/- ##
==========================================
- Coverage 73.21% 65.16% -8.06%
==========================================
Files 102 130 +28
Lines 8505 9085 +580
==========================================
- Hits 6227 5920 -307
- Misses 1802 2697 +895
+ Partials 476 468 -8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I'm noting that there's maybe some redundant path components?
|
joelanford
force-pushed
the
generate-applyconfigurations
branch
from
a04fd03 to
998a7f8
Compare
Yeah good call. I moved |
Use controller-gen's applyconfiguration generator to produce typed apply configuration structs for Server-Side Apply. This adds generated types for ClusterCatalog, ClusterExtension, ClusterExtensionRevision, and all their sub-types in applyconfigurations/. Changes: - Add applyconfiguration generator to the Makefile generate target - Add +kubebuilder:ac:generate=true and +kubebuilder:ac:output:package markers to the API package - Add SchemeGroupVersion alias required by the generated code Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Migrate Boxcutter from deprecated client.Patch(ctx, rev, client.Apply) to typed client.Apply(ctx, rev) using the generated ClusterExtensionRevisionApplyConfiguration types. - ClusterExtensionRevisionGenerator interface now returns AC types - GenerateRevision/GenerateRevisionFromHelmRelease build AC objects directly via builder pattern instead of API struct literals - Rename createOrUpdate to apply; uses client.Apply instead of deprecated client.Patch with client.Apply option - Owner references set via metav1ac.OwnerReference() builder instead of controllerutil.SetControllerReference - BoxcutterStorageMigrator.Migrate uses client.Apply; interface replaces Create with Apply and adds FieldOwner field - PhaseSort and helpers operate on AC types natively - All tests updated to use AC builder pattern and mock Apply Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
joelanford
force-pushed
the
generate-applyconfigurations
branch
from
998a7f8 to
6ff4d55
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 39 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
internal/operator-controller/applier/phase.go:137
- Potential nil pointer dereference: The function accesses
a.Object.GroupVersionKind()andb.Object.GetNamespace()without checking ifa.Objectorb.Objectis nil. While the ApplyConfiguration type uses pointer fields, the Object field might be nil in certain scenarios, which would cause a panic.
Consider adding nil checks:
if a.Object == nil || b.Object == nil {
// Handle nil objects appropriately
}internal/operator-controller/applier/phase.go:149
- Potential nil pointer dereference: Line 147 accesses
obj.Object.GroupVersionKind()without checking ifobj.Objectis nil. This could cause a panic if the Object field is not initialized in the ApplyConfiguration.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| User: user, | ||
| Name: rev.Name, | ||
| Name: *rev.GetName(), |
There was a problem hiding this comment.
Potential nil pointer dereference: Line 662 dereferences rev.GetName() without checking if the returned pointer is nil. If the name hasn't been set in the ApplyConfiguration, this will cause a panic.
Consider checking for nil:
name := rev.GetName()
if name == nil {
// Handle nil name appropriately
}| // Convert to pointers for WithObjects | ||
| objPtrs := make([]*ocv1ac.ClusterExtensionRevisionObjectApplyConfiguration, len(objs)) | ||
| for i := range objs { | ||
| objPtrs[i] = &objs[i] | ||
| } |
There was a problem hiding this comment.
Aliasing issue with pointer conversion: The code creates pointers to slice elements (&objs[i]) which point to the slice's backing array. After the loop completes, these pointers become invalid if the slice is reallocated. While WithObjects dereferences these immediately (line 64 in the generated code), this pattern is fragile and could lead to bugs if the generated code changes or if the slice is modified between pointer creation and dereferencing.
A safer approach would be to pass values directly if WithObjects accepts pointers but dereferences them, or to store the objects differently. However, since WithObjects immediately dereferences the pointers, this is likely safe in practice but represents a code smell.
| generate: $(CONTROLLER_GEN) #EXHELP Generate code containing DeepCopy, DeepCopyInto, DeepCopyObject, and ApplyConfiguration type implementations. | ||
| # Need to delete the files for them to be generated properly | ||
| @find api cmd hack internal -name "zz_generated.deepcopy.go" -not -path "*/vendor/*" -delete && rm -rf applyconfigurations | ||
| $(CONTROLLER_GEN) --load-build-tags=$(GO_BUILD_TAGS) applyconfiguration:headerFile="hack/boilerplate.go.txt" paths="./api/..." |
There was a problem hiding this comment.
Hi @joelanford should all APIs that we have use applyconfigurations now?
OR just the specific one?
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. 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. |
5 participants
Description
Generate typed apply configuration structs for all OLM API types and migrate Boxcutter from the deprecated
client.Patch(ctx, rev, client.Apply, ...)to typedclient.Apply(ctx, rev, ...).The project previously lacked generated apply configuration types, which forced Boxcutter to construct full API objects and use the deprecated
client.Patchwithclient.Applyoption for Server-Side Apply. This PR generates those types and migrates all ClusterExtensionRevision construction and application to use them natively.Key changes:
applyconfigurationgenerator+genclientand+genclient:nonNamespacedmarkers to cluster-scoped API types so generated constructors correctly omit the namespace parameterClusterExtensionRevisionGeneratorinterface returns AC types built via the fluent builder patterncreateOrUpdatetoapply; usesclient.Applydirectlymetav1ac.OwnerReference()builder instead ofcontrollerutil.SetControllerReferenceBoxcutterStorageMigratorusesclient.Applyinstead ofclient.CreatePhaseSortand helpers operate on AC types nativelyReviewer Checklist