fix: incorrect logic by introducing createOrUpdate method#3172
Conversation
Signed-off-by: Chris Laprun <metacosm@gmail.com>
metacosm
force-pushed
the
fix-webpage
branch
from
0175b00 to
0ab4891
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors the low-level WebPageReconciler reconciliation flow by introducing a shared createOrUpdate helper to reduce duplicated “apply-if-different” logic, and fixes an incorrect resource application in the Ingress reconciliation path.
Changes:
- Introduces a generic
createOrUpdatehelper to consolidate create/update logic for secondary resources. - Fixes Ingress reconciliation to server-side-apply the desired Ingress (instead of the Deployment).
- Simplifies desired resource builders to derive namespace/name directly from
WebPageand removesStringUtilsusage.
Comments suppressed due to low confidence (1)
sample-operators/webpage/src/main/java/io/javaoperatorsdk/operator/sample/WebPageReconciler.java:110
- The comment reads "not that this is not necessary" which looks like a typo/grammar issue; it should be "note that this is not necessary" (or similar) to make the intent clear.
// not that this is not necessary, eventually mounted config map would be updated, just this way
// is much faster; this is handy for demo purposes.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private <T extends HasMetadata> T createOrUpdate( | ||
| Context<WebPage> context, T desired, BiFunction<T, T, Boolean> matcher) { | ||
| @SuppressWarnings("unchecked") | ||
| final T previous = (T) context.getSecondaryResource(desired.getClass()).orElse(null); | ||
| if (!matcher.apply(desired, previous)) { | ||
| log.info( | ||
| "Creating or updating {} {} in {}", | ||
| desired.getKind(), | ||
| desired.getMetadata().getName(), | ||
| desired.getMetadata().getNamespace()); | ||
| context.resourceOperations().serverSideApply(desired); | ||
| } | ||
| return previous; |
There was a problem hiding this comment.
CI runs spotless:check with google-java-format; this new createOrUpdate block doesn’t appear to be google-java-format output (long method signature on one line, indentation/alignment inside log.info, extra blank line above). Please run Spotless / apply google-java-format so the file matches the enforced formatter and the PR check passes.
| Context<WebPage> context, T desired, BiFunction<T, T, Boolean> matcher) { | ||
| @SuppressWarnings("unchecked") | ||
| final T previous = (T) context.getSecondaryResource(desired.getClass()).orElse(null); |
There was a problem hiding this comment.
createOrUpdate relies on an unchecked cast from getSecondaryResource(desired.getClass()) to T, requiring @SuppressWarnings("unchecked"). To avoid potential ClassCastException and remove the suppression, consider passing the resource Class<T> explicitly (e.g., createOrUpdate(context, Deployment.class, desiredDeployment, this::match)) or otherwise restructuring so the Optional is typed without a cast.
| if (Boolean.TRUE.equals(webPage.getSpec().getExposed())) { | ||
| var desiredIngress = makeDesiredIngress(webPage); | ||
| if (existingIngress.isEmpty() || !match(desiredIngress, existingIngress.get())) { | ||
| context.resourceOperations().serverSideApply(desiredDeployment); | ||
| context.resourceOperations().serverSideApply(desiredIngress); | ||
| } |
There was a problem hiding this comment.
The exposed-Ingress reconciliation path was changed (applying desiredIngress instead of desiredDeployment), but there’s no automated coverage for spec.exposed=true in the existing E2E test. Consider extending WebPageOperatorAbstractTest to create a WebPage with spec.setExposed(true) and assert that an Ingress is created/updated; this would prevent regressions of this logic.
2 participants
Signed-off-by: Chris Laprun metacosm@gmail.com