Skip to content

improve: facelift samples to use ReconcileUtils #3135

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
csviri merged 3 commits into from
Jan 27, 2026
Merged

improve: facelift samples to use ReconcileUtils #3135

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
af2b565
improce: facelift samples to use ReconcileUtils
csviri Jan 26, 2026
553d430
wip
csviri Jan 26, 2026
1ffcd5c
wip
csviri Jan 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 4 additions & 15 deletions ...operators/webpage/src/main/java/io/javaoperatorsdk/operator/sample/WebPageReconciler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,12 +105,7 @@ public UpdateControl<WebPage> reconcile(WebPage webPage, Context<WebPage> contex
"Creating or updating ConfigMap {} in {}",
desiredHtmlConfigMap.getMetadata().getName(),
ns);
context
.getClient()
.configMaps()
.inNamespace(ns)
.resource(desiredHtmlConfigMap)
.serverSideApply();
ReconcileUtils.serverSideApply(context, desiredHtmlConfigMap);
Copy link
Copy Markdown
Collaborator

@metacosm metacosm Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes me think that maybe we should remove ReconcileUtils altogether and put its method on Context instead.

Copy link
Copy Markdown
Collaborator Author

@csviri csviri Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ReconcileUtils now contains all kind of other methods, see also PR #3130
So we would need a Reconcile utils anyways.

For adding those methods into Context I have mixed feelings: it would be easier to discover for sure, on the other hand having basically utility methods there does not feel right to me. Also I'm a little bit affraid that would make too much noise/pollute the Context, so would be harder to understand the details of context for the user.
Also, I assume some more such methods might come in the future.

So with design wise I would formulate it like: have minimal stuff exposed absolutely necessary through the Context, and putting everything else, helper methods into the ReconcileUtils.

Copy link
Copy Markdown
Collaborator Author

@csviri csviri Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But up to discussion.

Copy link
Copy Markdown
Collaborator Author

@csviri csviri Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xstefank @shawkins what do you think?

Copy link
Copy Markdown
Collaborator

@metacosm metacosm Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes sense but from a design-perspective, I'm always suspicious of static functions that all take the same object parameter. It's usually an indication that this function should probably be a method of that object.

Regarding that other PR, it doesn't seem like these new methods are particularly related to reconciliation, which weakens the unity of ReconcileUtils, imo. I mean I get not wanting to create lots of utility classes for discovery purposes but, on the same token, putting too many loosely coupled functions in a single object doesn't help discovery either (and yes, that applies to Context as you correctly pointed out).

Copy link
Copy Markdown
Collaborator

@xstefank xstefank Jan 27, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH, I can see both points but I'm more inclined to put these methods on context because less visual noise for the user and easier discoverability. I rarely scroll down all available methods of an object. I think most people will read the docs first and understand how to call everything. So to me it's between static method vs the object already in params, and the second case wins.

Copy link
Copy Markdown
Collaborator Author

@csviri csviri Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the thing is that the param in that Reconcile utils is not the primary resource but a resource that user explicitly creates. Let's have a separate issues for this, dicsuss it on community meeting.

Copy link
Copy Markdown
Collaborator Author

@csviri csviri Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's continue here, will merge this and the other PR, and we can still refactor before the release:
#3136

}

var existingDeployment = context.getSecondaryResource(Deployment.class).orElse(null);
Expand All @@ -119,13 +114,7 @@ public UpdateControl<WebPage> reconcile(WebPage webPage, Context<WebPage> contex
"Creating or updating Deployment {} in {}",
desiredDeployment.getMetadata().getName(),
ns);
context
.getClient()
.apps()
.deployments()
.inNamespace(ns)
.resource(desiredDeployment)
.serverSideApply();
ReconcileUtils.serverSideApply(context, desiredDeployment);
}

var existingService = context.getSecondaryResource(Service.class).orElse(null);
Expand All @@ -134,14 +123,14 @@ public UpdateControl<WebPage> reconcile(WebPage webPage, Context<WebPage> contex
"Creating or updating Deployment {} in {}",
desiredDeployment.getMetadata().getName(),
ns);
context.getClient().services().inNamespace(ns).resource(desiredService).serverSideApply();
ReconcileUtils.serverSideApply(context, desiredService);
}

var existingIngress = context.getSecondaryResource(Ingress.class);
if (Boolean.TRUE.equals(webPage.getSpec().getExposed())) {
var desiredIngress = makeDesiredIngress(webPage);
if (existingIngress.isEmpty() || !match(desiredIngress, existingIngress.get())) {
context.getClient().resource(desiredIngress).inNamespace(ns).serverSideApply();
ReconcileUtils.serverSideApply(context, desiredIngress);
}
} else existingIngress.ifPresent(ingress -> context.getClient().resource(ingress).delete());

Expand Down