🌱 Fix SA1019: server-side apply required, needs generated apply configurations by camilamacedo86 · Pull Request #2511 · operator-framework/operator-controller

camilamacedo86 · 2026-02-16T17:39:02Z

No description provided.

netlify · 2026-02-16T17:39:07Z

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	`30e7965`
🔍 Latest deploy log	https://app.netlify.com/projects/olmv1/deploys/69957190e813800008d2e9b1
😎 Deploy Preview	https://deploy-preview-2511--olmv1.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

Copilot

Pull request overview

This PR addresses the SA1019 deprecation warning by replacing the deprecated client.Apply variable with a custom serverSideApplyPatch implementation. The change maintains server-side apply semantics with field ownership management while avoiding the need to generate ApplyConfiguration types.

Changes:

Implemented custom serverSideApplyPatch type that satisfies the client.Patch interface
Replaced deprecated client.Apply usage with serverSideApplyPatch{}
Removed extensive deprecation comment and nolint directive
Added two new imports: k8s.io/apimachinery/pkg/types and k8s.io/apimachinery/pkg/util/json

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

codecov · 2026-02-16T17:58:25Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.40%. Comparing base (23dd207) to head (30e7965).
⚠️ Report is 21 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #2511 +/- ##
==========================================
+ Coverage 73.21% 73.40% +0.18% 
==========================================
Files 102 102 Lines 8504 8564 +60 ==========================================
+ Hits 6226 6286 +60 
Misses 1799 1799 Partials 479 479

Flag	Coverage Δ
e2e	`45.55% <0.00%> (-0.29%)`	⬇️
experimental-e2e	`53.95% <100.00%> (+0.45%)`	⬆️
unit	`58.09% <86.44%> (+0.20%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

pedjak · 2026-02-17T11:28:09Z

-	// nolint:staticcheck // SA1019: server-side apply required, needs generated apply configurations
-	return bc.Client.Patch(ctx, rev, client.Apply, client.FieldOwner(bc.FieldOwner), client.ForceOwnership)
+	// Use Server-Side Apply to manage field ownership and handle conflicts during OLM upgrades.
+	return bc.Client.Patch(ctx, rev, serverSideApplyPatch{}, client.FieldOwner(bc.FieldOwner), client.ForceOwnership)


IMHO, the right fix here would be to use Client.Apply(..) and generate apply configuration from unstructured, see https://github.com/kubernetes-sigs/controller-runtime/blob/main/pkg/client/applyconfigurations.go#L38

Amazing idea. Done !!!

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

joelanford

This kind of conversion is not safe:

(typed struct) -> (unstructured) -> (apply configuration)

In this type of conversion it is impossible to tell the difference between:

Intentionally use the default value of a type for a field, AND
I don't care about that field

joelanford · 2026-02-17T14:25:06Z

-	return bc.Client.Patch(ctx, rev, client.Apply, client.FieldOwner(bc.FieldOwner), client.ForceOwnership)
+	// Convert the revision to an unstructured object to create an apply configuration.
+	// This allows us to use Server-Side Apply without needing generated apply types.
+	unstructuredRev, err := runtime.DefaultUnstructuredConverter.ToUnstructured(rev)


If there is any field in rev without omitzero and omitempty, those fields will be serialized and applied with our field owner, despite rev not having an opinion on them. To use SSA properly there are basically only two options:
Build unstructured from scratch
Use an apply configuration type.

I think the solution here is to generate the apply configurations for our types (which I think is now possible using controller-gen?)

I think the solution here is to generate the apply configurations for our types (which I think is now possible using controller-gen?)
IMHO, this is a bigger change, perhaps we should merge this first and explore it in another PR?

I'm reading the comments that are removed and it seems like they all still apply though? So at a minimum, I think we should restore the comments that talk about needing to use generated applyconfigurations.
Otherwise, what is the functional change here?
Does the switch from this:
bc.Client.Patch(ctx, rev, client.Apply, client.FieldOwner(bc.FieldOwner), client.ForceOwnership)
to this (with the rev -> unstructured -> applyConfig traversal):
bc.Client.Apply(ctx, applyConfig, client.FieldOwner(bc.FieldOwner), client.ForceOwnership)
have any actual change other than removing our use of a deprecated function?

Joe I checked this one and accordingly to Claude
Your Current Implementation is Already Correct! ✓
Implements the recommended approach for using Server-Side Apply with controller-runtime v0.23+:
Manual construction of unstructured objects with only managed fields
client.ApplyConfigurationFromUnstructured() to create apply configurations
client.Apply() instead of the deprecated client.Patch() with client.Apply type
For custom CRDs, the manual ApplyConfigurationFromUnstructured approach is the recommended pattern because:
It gives you fine-grained control over which fields to manage
Avoids ownership conflicts from zero-value fields
No code generation required
Works perfectly with Server-Side Apply semantics
However, I think we should invest some times, maybe create a tech-debt task for deeper analyse the possibility to use controller-tools with serversideapply markers to generate the manifests. Looking for better maintainability in long term.

pedjak

/lgtm

camilamacedo86 · 2026-02-17T15:20:40Z

I will hold based on the @joelanford feedback #2511 (comment) and solve it in another way

/hold

openshift-ci · 2026-02-18T07:07:38Z

New changes are detected. LGTM label has been removed.

openshift-ci · 2026-02-18T07:07:40Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from joelanford. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…rations

camilamacedo86 · 2026-02-18T08:01:21Z

Hi @joelanford and @pedjak

The comments raised were all addressed.
I added the review done by claude and the info generated from there as well.

So, I think it is good to fly 🚀 now.

camilamacedo86 · 2026-02-18T08:04:58Z

/hold cancel

All addressed

pedjak · 2026-02-18T13:08:40Z

+	// We cannot safely convert the typed ClusterExtensionRevision struct directly to unstructured
+	// because that would serialize all fields, including zero values for fields without omitempty tags.
+	// This would incorrectly claim field ownership for fields we don't manage, violating Server-Side
+	// Apply semantics.


Just checked ClusterExtensionRevisionSpec and we do not have fields without omitempty tag. Hence, it is safe to use the previous approach with DefaultUnstructuredConverter:
func (bc*Boxcutter) createOrUpdate(ctx context.Context, user user.Info, rev*ocv1.ClusterExtensionRevision) error { // Run auth preflight checksiferr:=bc.runPreAuthorizationChecks(ctx, user, rev); err!=nil { returnerr } data, err:=runtime.DefaultUnstructuredConverter.ToUnstructured(rev) iferr!=nil { returnerr } u:=&unstructured.Unstructured{Object: data} gvk, err:=apiutil.GVKForObject(rev, bc.Scheme) iferr!=nil { returnerr } u.GetObjectKind().SetGroupVersionKind(gvk) applyConfig:=client.ApplyConfigurationFromUnstructured(u) // Use Server-Side Apply with field ownership to manage the revision.returnbc.Client.Apply(ctx, applyConfig, client.FieldOwner(bc.FieldOwner), client.ForceOwnership) }
why this works?
rev argument contains desired resource, created by bc.RevisionGenerator.GenerateRevision - that object contains only the fields we are interested in, it is not pulled from the cache.
even if we had a field to zero value, we should own it, actually we should own the whole .spec, because we are creating and reconciling CERs - no other controller should be able to change the spec.

I think is safe we keep as it is
WHY?
If something change there all still working

joelanford · 2026-03-02T14:46:07Z

Addressed by #2515

CopilotAI review requested due to automatic review settings February 16, 2026 17:39

openshift-ciBot requested review from bentito and pedjak February 16, 2026 17:39

Copilotstarted reviewing on behalf of camilamacedo86 February 16, 2026 17:39 View session

camilamacedo86 force-pushed the fix branch from 1de8ff1 to 10c35beCompare February 16, 2026 17:40

CopilotAI reviewed Feb 16, 2026
View reviewed changes

camilamacedo86 requested a review from perdasilva February 16, 2026 18:10

tmshort reviewed Feb 16, 2026
View reviewed changes

Comment threadinternal/operator-controller/applier/boxcutter.go Outdated

pedjak reviewed Feb 17, 2026
View reviewed changes

camilamacedo86 force-pushed the fix branch from 10c35be to 777262cCompare February 17, 2026 12:06

CopilotAI review requested due to automatic review settings February 17, 2026 12:09

camilamacedo86 force-pushed the fix branch from 777262c to a626b17Compare February 17, 2026 12:09

Copilotstarted reviewing on behalf of camilamacedo86 February 17, 2026 12:09 View session

CopilotAI reviewed Feb 17, 2026
View reviewed changes

Comment threadinternal/operator-controller/applier/boxcutter_test.go Outdated

camilamacedo86 force-pushed the fix branch 2 times, most recently from 767a688 to 83002f3Compare February 17, 2026 14:19

CopilotAI review requested due to automatic review settings February 17, 2026 14:19

Copilotstarted reviewing on behalf of camilamacedo86 February 17, 2026 14:20 View session

CopilotAI reviewed Feb 17, 2026
View reviewed changes

Comment threadinternal/operator-controller/applier/boxcutter_test.go
Comment threadinternal/operator-controller/applier/boxcutter.go Outdated

joelanford requested changes Feb 17, 2026
View reviewed changes

openshift-ciBot assigned joelanford Feb 17, 2026

pedjak reviewed Feb 17, 2026
View reviewed changes

openshift-ciBot assigned pedjak Feb 17, 2026

openshift-ciBot added the lgtm Indicates that a PR is ready to be merged. label Feb 17, 2026

camilamacedo86 changed the title ~~🌱 Fix SA1019: server-side apply required, needs generated apply configurations~~WIP 🌱 Fix SA1019: server-side apply required, needs generated apply configurationsFeb 17, 2026

openshift-ciBot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 17, 2026

openshift-ciBot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 17, 2026

openshift-ciBot removed the lgtm Indicates that a PR is ready to be merged. label Feb 18, 2026

camilamacedo86 force-pushed the fix branch from 52c21b3 to 3dc9d83Compare February 18, 2026 07:17

CopilotAI review requested due to automatic review settings February 18, 2026 07:17

Copilotstarted reviewing on behalf of camilamacedo86 February 18, 2026 07:17 View session

CopilotAI reviewed Feb 18, 2026
View reviewed changes

Comment threadinternal/operator-controller/applier/boxcutter.go Outdated

Fix SA1019: server-side apply required, needs generated apply configu…
30e7965
…rations

camilamacedo86 force-pushed the fix branch from 3dc9d83 to 30e7965Compare February 18, 2026 08:00

camilamacedo86 changed the title ~~WIP 🌱 Fix SA1019: server-side apply required, needs generated apply configurations~~🌱 Fix SA1019: server-side apply required, needs generated apply configurationsFeb 18, 2026

openshift-ciBot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 18, 2026

openshift-ciBot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 18, 2026

pedjak reviewed Feb 18, 2026
View reviewed changes

camilamacedo86 requested review from joelanford, pedjak and tmshort February 18, 2026 14:57

perdasilva mentioned this pull request Mar 2, 2026
✨ Generate and use ApplyConfiguration types for Server-Side Apply #2515
Merged
4 tasks

joelanford closed this Mar 2, 2026

Uh oh!

Conversation

camilamacedo86 commented Feb 16, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlifyBot commented Feb 16, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for olmv1 ready!

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

codecovBot commented Feb 16, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

joelanford left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

camilamacedo86Feb 18, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pedjak left a comment

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 commented Feb 17, 2026

Uh oh!

openshift-ciBot commented Feb 18, 2026

Uh oh!

openshift-ciBot commented Feb 18, 2026

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

camilamacedo86 commented Feb 18, 2026

Uh oh!

camilamacedo86 commented Feb 18, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joelanford commented Mar 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

camilamacedo86 commented Feb 16, 2026•
edited
Loading

netlifyBot commented Feb 16, 2026•
edited
Loading

codecovBot commented Feb 16, 2026•
edited
Loading

camilamacedo86Feb 18, 2026•
edited
Loading