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

Show file tree
Hide file tree
Changes from all commits
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
2 changes: 1 addition & 1 deletion .../src/test/java/io/javaoperatorsdk/operator/baseapi/changenamespace/ChangeNamespaceIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ private static void assertReconciled(
assertThat(
reconciler.numberOfResourceReconciliations(
resourceInAdditionalTestNamespace))
.isEqualTo(2));
.isEqualTo(1));
}

private static void assertNotReconciled(
Expand Down
10 changes: 1 addition & 9 deletions ...va/io/javaoperatorsdk/operator/baseapi/changenamespace/ChangeNamespaceTestReconciler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,15 +53,7 @@ public UpdateControl<ChangeNamespaceTestCustomResource> reconcile(
ChangeNamespaceTestCustomResource primary,
Context<ChangeNamespaceTestCustomResource> context) {

var actualConfigMap = context.getSecondaryResource(ConfigMap.class);
if (actualConfigMap.isEmpty()) {
context
.getClient()
.configMaps()
.inNamespace(primary.getMetadata().getNamespace())
.resource(configMap(primary))
.create();
}
ReconcileUtils.serverSideApply(context, configMap(primary));

if (primary.getStatus() == null) {
primary.setStatus(new ChangeNamespaceTestCustomResourceStatus());
Expand Down
28 changes: 1 addition & 27 deletions ...aseapi/generickubernetesresourcehandling/GenericKubernetesResourceHandlingReconciler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@

import java.io.IOException;
import java.io.InputStream;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

Expand All @@ -41,36 +40,11 @@ public UpdateControl<GenericKubernetesResourceHandlingCustomResource> reconcile(
GenericKubernetesResourceHandlingCustomResource primary,
Context<GenericKubernetesResourceHandlingCustomResource> context) {

var secondary = context.getSecondaryResource(GenericKubernetesResource.class);

secondary.ifPresentOrElse(
r -> {
var desired = desiredConfigMap(primary, context);
if (!matches(r, desired)) {
context
.getClient()
.genericKubernetesResources(VERSION, KIND)
.resource(desired)
.update();
}
},
() ->
context
.getClient()
.genericKubernetesResources(VERSION, KIND)
.resource(desiredConfigMap(primary, context))
.create());
ReconcileUtils.serverSideApply(context, desiredConfigMap(primary, context));

return UpdateControl.noUpdate();
}

@SuppressWarnings("unchecked")
private boolean matches(GenericKubernetesResource actual, GenericKubernetesResource desired) {
var actualData = (HashMap<String, String>) actual.getAdditionalProperties().get("data");
var desiredData = (HashMap<String, String>) desired.getAdditionalProperties().get("data");
return actualData.equals(desiredData);
}

GenericKubernetesResource desiredConfigMap(
GenericKubernetesResourceHandlingCustomResource primary,
Context<GenericKubernetesResourceHandlingCustomResource> context) {
Expand Down
28 changes: 3 additions & 25 deletions ...operator/baseapi/multiplesecondaryeventsource/MultipleSecondaryEventSourceReconciler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import io.javaoperatorsdk.operator.api.reconciler.Context;
import io.javaoperatorsdk.operator.api.reconciler.ControllerConfiguration;
import io.javaoperatorsdk.operator.api.reconciler.EventSourceContext;
import io.javaoperatorsdk.operator.api.reconciler.ReconcileUtils;
import io.javaoperatorsdk.operator.api.reconciler.Reconciler;
import io.javaoperatorsdk.operator.api.reconciler.UpdateControl;
import io.javaoperatorsdk.operator.processing.event.ResourceID;
Expand Down Expand Up @@ -53,31 +54,8 @@ public UpdateControl<MultipleSecondaryEventSourceCustomResource> reconcile(
Context<MultipleSecondaryEventSourceCustomResource> context) {
numberOfExecutions.addAndGet(1);

final var client = context.getClient();
if (client
.configMaps()
.inNamespace(resource.getMetadata().getNamespace())
.withName(getName1(resource))
.get()
== null) {
client
.configMaps()
.inNamespace(resource.getMetadata().getNamespace())
.resource(configMap(getName1(resource), resource))
.createOrReplace();
}
if (client
.configMaps()
.inNamespace(resource.getMetadata().getNamespace())
.withName(getName2(resource))
.get()
== null) {
client
.configMaps()
.inNamespace(resource.getMetadata().getNamespace())
.resource(configMap(getName2(resource), resource))
.createOrReplace();
}
ReconcileUtils.serverSideApply(context, configMap(getName1(resource), resource));
ReconcileUtils.serverSideApply(context, configMap(getName2(resource), resource));

if (numberOfExecutions.get() >= 3) {
if (context.getSecondaryResources(ConfigMap.class).size() != 2) {
Expand Down
3 changes: 2 additions & 1 deletion ...est/java/io/javaoperatorsdk/operator/dependent/externalstate/ExternalStateReconciler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import io.javaoperatorsdk.operator.api.reconciler.ControllerConfiguration;
import io.javaoperatorsdk.operator.api.reconciler.DeleteControl;
import io.javaoperatorsdk.operator.api.reconciler.EventSourceContext;
import io.javaoperatorsdk.operator.api.reconciler.ReconcileUtils;
import io.javaoperatorsdk.operator.api.reconciler.Reconciler;
import io.javaoperatorsdk.operator.api.reconciler.UpdateControl;
import io.javaoperatorsdk.operator.processing.event.ResourceID;
Expand Down Expand Up @@ -110,7 +111,7 @@ private void createExternalResource(
// This is critical in this case, since on next reconciliation if it would not be in the cache
// it would be created again.
configMapEventSource.eventFilteringUpdateAndCacheResource(
configMap, toCreate -> context.getClient().configMaps().resource(toCreate).create());
configMap, toCreate -> ReconcileUtils.serverSideApply(context, toCreate));
externalResourceEventSource.handleRecentResourceCreate(primaryID, createdResource);
}

Expand Down
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
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
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
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
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
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
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
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
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