fix: do not use Optional in Condition, fix generics

metacosm · metacosm · commit 3610ebd34712 · 2022-08-26T16:31:32.000+02:00
diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/Condition.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/Condition.java
@@ -1,11 +1,21 @@
 package io.javaoperatorsdk.operator.processing.dependent.workflow;
 
-import java.util.Optional;
-
 import io.fabric8.kubernetes.api.model.HasMetadata;
 import io.javaoperatorsdk.operator.api.reconciler.Context;
 
 public interface Condition<R, P extends HasMetadata> {
 
-  boolean isMet(P primary, Optional<R> secondary, Context<P> context);
+  /**
+   * Checks whether a condition holds true for a given
+   * {@link io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource} based on the
+   * observed cluster state.
+   *
+   * @param primary the primary resource being considered
+   * @param secondary the secondary resource associated with the specified primary resource or
+   *        {@code null} if no such secondary resource exists (for example because it's been
+   *        deleted)
+   * @param context the current reconciliation {@link Context}
+   * @return {@code true} if the condition holds, {@code false} otherwise
+   */
+  boolean isMet(P primary, R secondary, Context<P> context);
 }
diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DependentResourceNode.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DependentResourceNode.java
@@ -11,9 +11,9 @@
 public class DependentResourceNode<R, P extends HasMetadata> {
 
   private final DependentResource<R, P> dependentResource;
-  private Condition reconcilePrecondition;
-  private Condition deletePostcondition;
-  private Condition readyPostcondition;
+  private Condition<R, P> reconcilePrecondition;
+  private Condition<R, P> deletePostcondition;
+  private Condition<R, P> readyPostcondition;
   private final List<DependentResourceNode> dependsOn = new LinkedList<>();
   private final List<DependentResourceNode> parents = new LinkedList<>();
 
@@ -22,12 +22,12 @@ public DependentResourceNode(DependentResource<R, P> dependentResource) {
   }
 
   public DependentResourceNode(DependentResource<R, P> dependentResource,
-      Condition reconcilePrecondition) {
+      Condition<R, P> reconcilePrecondition) {
     this(dependentResource, reconcilePrecondition, null);
   }
 
   public DependentResourceNode(DependentResource<R, P> dependentResource,
-      Condition reconcilePrecondition, Condition deletePostcondition) {
+      Condition<R, P> reconcilePrecondition, Condition<R, P> deletePostcondition) {
     this.dependentResource = dependentResource;
     this.reconcilePrecondition = reconcilePrecondition;
     this.deletePostcondition = deletePostcondition;
@@ -37,11 +37,11 @@ public DependentResource<R, P> getDependentResource() {
     return dependentResource;
   }
 
-  public Optional<Condition> getReconcilePrecondition() {
+  public Optional<Condition<R, P>> getReconcilePrecondition() {
     return Optional.ofNullable(reconcilePrecondition);
   }
 
-  public Optional<Condition> getDeletePostcondition() {
+  public Optional<Condition<R, P>> getDeletePostcondition() {
     return Optional.ofNullable(deletePostcondition);
   }
 
@@ -63,12 +63,12 @@ public String toString() {
   }
 
   public DependentResourceNode<R, P> setReconcilePrecondition(
-      Condition reconcilePrecondition) {
+      Condition<R, P> reconcilePrecondition) {
     this.reconcilePrecondition = reconcilePrecondition;
     return this;
   }
 
-  public DependentResourceNode<R, P> setDeletePostcondition(Condition cleanupCondition) {
+  public DependentResourceNode<R, P> setDeletePostcondition(Condition<R, P> cleanupCondition) {
     this.deletePostcondition = cleanupCondition;
     return this;
   }
@@ -77,7 +77,7 @@ public Optional<Condition<R, P>> getReadyPostcondition() {
     return Optional.ofNullable(readyPostcondition);
   }
 
-  public DependentResourceNode<R, P> setReadyPostcondition(Condition readyPostcondition) {
+  public DependentResourceNode<R, P> setReadyPostcondition(Condition<R, P> readyPostcondition) {
     this.readyPostcondition = readyPostcondition;
     return this;
   }
diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java
@@ -3,7 +3,6 @@
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Map;
-import java.util.Optional;
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.Future;
@@ -72,7 +71,7 @@ public synchronized WorkflowReconcileResult reconcile() {
     return createReconcileResult();
   }
 
-  private synchronized void handleReconcile(DependentResourceNode<?, P> dependentResourceNode) {
+  private synchronized <R> void handleReconcile(DependentResourceNode<R, P> dependentResourceNode) {
     log.debug("Submitting for reconcile: {}", dependentResourceNode);
 
     if (alreadyVisited(dependentResourceNode)
@@ -86,7 +85,8 @@ private synchronized void handleReconcile(DependentResourceNode<?, P> dependentR
 
     boolean reconcileConditionMet = dependentResourceNode.getReconcilePrecondition()
         .map(rc -> rc.isMet(primary,
-            dependentResourceNode.getDependentResource().getSecondaryResource(primary), context))
+            dependentResourceNode.getDependentResource().getSecondaryResource(primary).orElse(null),
+            context))
         .orElse(true);
 
     if (!reconcileConditionMet) {
@@ -144,9 +144,9 @@ private synchronized void setAlreadyReconciledButNotReady(
     notReady.add(dependentResourceNode);
   }
 
-  private class NodeReconcileExecutor implements Runnable {
+  private class NodeReconcileExecutor<R> implements Runnable {
 
-    private final DependentResourceNode dependentResourceNode;
+    private final DependentResourceNode<R, P> dependentResourceNode;
 
     private NodeReconcileExecutor(DependentResourceNode dependentResourceNode) {
       this.dependentResourceNode = dependentResourceNode;
@@ -166,9 +166,10 @@ public void run() {
         ReconcileResult reconcileResult = dependentResource.reconcile(primary, context);
         reconcileResults.put(dependentResource, reconcileResult);
         reconciled.add(dependentResourceNode);
-        boolean ready = ((Optional<Condition>) dependentResourceNode.getReadyPostcondition())
+        boolean ready = dependentResourceNode.getReadyPostcondition()
             .map(rc -> rc.isMet(primary,
-                dependentResourceNode.getDependentResource().getSecondaryResource(primary),
+                dependentResourceNode.getDependentResource().getSecondaryResource(primary)
+                    .orElse(null),
                 context))
             .orElse(true);
 
@@ -187,11 +188,11 @@ public void run() {
     }
   }
 
-  private class NodeDeleteExecutor implements Runnable {
+  private class NodeDeleteExecutor<R> implements Runnable {
 
-    private final DependentResourceNode<?, P> dependentResourceNode;
+    private final DependentResourceNode<R, P> dependentResourceNode;
 
-    private NodeDeleteExecutor(DependentResourceNode<?, P> dependentResourceNode) {
+    private NodeDeleteExecutor(DependentResourceNode<R, P> dependentResourceNode) {
       this.dependentResourceNode = dependentResourceNode;
     }
 
@@ -209,7 +210,8 @@ public void run() {
         alreadyVisited.add(dependentResourceNode);
         boolean deletePostConditionMet =
             deletePostCondition.map(c -> c.isMet(primary,
-                dependentResourceNode.getDependentResource().getSecondaryResource(primary),
+                dependentResourceNode.getDependentResource().getSecondaryResource(primary)
+                    .orElse(null),
                 context)).orElse(true);
         if (deletePostConditionMet) {
           handleDependentDeleted(dependentResourceNode);
diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/ControllerConfigurationOverriderTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/ControllerConfigurationOverriderTest.java
@@ -342,8 +342,7 @@ public UpdateControl<ConfigMap> reconcile(ConfigMap resource, Context<ConfigMap>
   private static class TestCondition implements Condition<ConfigMap, ConfigMap> {
 
     @Override
-    public boolean isMet(ConfigMap primary, Optional<ConfigMap> secondary,
-        Context<ConfigMap> context) {
+    public boolean isMet(ConfigMap primary, ConfigMap secondary, Context<ConfigMap> context) {
       return true;
     }
   }
diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/workflowallfeature/ConfigMapDeletePostCondition.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/workflowallfeature/ConfigMapDeletePostCondition.java
@@ -1,7 +1,5 @@
 package io.javaoperatorsdk.operator.sample.workflowallfeature;
 
-import java.util.Optional;
-
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -16,9 +14,9 @@ public class ConfigMapDeletePostCondition
 
   @Override
   public boolean isMet(
-      WorkflowAllFeatureCustomResource primary, Optional<ConfigMap> secondary,
+      WorkflowAllFeatureCustomResource primary, ConfigMap secondary,
       Context<WorkflowAllFeatureCustomResource> context) {
-    var configMapDeleted = secondary.isEmpty();
+    var configMapDeleted = secondary == null;
     log.debug("Config Map Deleted: {}", configMapDeleted);
     return configMapDeleted;
   }
diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/workflowallfeature/ConfigMapReconcileCondition.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/workflowallfeature/ConfigMapReconcileCondition.java
@@ -1,7 +1,5 @@
 package io.javaoperatorsdk.operator.sample.workflowallfeature;
 
-import java.util.Optional;
-
 import io.fabric8.kubernetes.api.model.ConfigMap;
 import io.javaoperatorsdk.operator.api.reconciler.Context;
 import io.javaoperatorsdk.operator.processing.dependent.workflow.Condition;
@@ -10,7 +8,7 @@ public class ConfigMapReconcileCondition
     implements Condition<ConfigMap, WorkflowAllFeatureCustomResource> {
 
   @Override
-  public boolean isMet(WorkflowAllFeatureCustomResource primary, Optional<ConfigMap> secondary,
+  public boolean isMet(WorkflowAllFeatureCustomResource primary, ConfigMap secondary,
       Context<WorkflowAllFeatureCustomResource> context) {
     return primary.getSpec().isCreateConfigMap();
   }
diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/workflowallfeature/DeploymentReadyCondition.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/workflowallfeature/DeploymentReadyCondition.java
@@ -1,17 +1,17 @@
 package io.javaoperatorsdk.operator.sample.workflowallfeature;
 
-import java.util.Optional;
-
 import io.fabric8.kubernetes.api.model.apps.Deployment;
 import io.javaoperatorsdk.operator.api.reconciler.Context;
 import io.javaoperatorsdk.operator.processing.dependent.workflow.Condition;
 
 public class DeploymentReadyCondition
     implements Condition<Deployment, WorkflowAllFeatureCustomResource> {
   @Override
-  public boolean isMet(WorkflowAllFeatureCustomResource primary, Optional<Deployment> secondary,
+  public boolean isMet(WorkflowAllFeatureCustomResource primary, Deployment deployment,
       Context<WorkflowAllFeatureCustomResource> context) {
-    var deployment = secondary.orElseThrow();
+    if (deployment == null) {
+      return false;
+    }
     var readyReplicas = deployment.getStatus().getReadyReplicas();
 
     return readyReplicas != null && deployment.getSpec().getReplicas().equals(readyReplicas);
diff --git a/sample-operators/webpage/src/main/java/io/javaoperatorsdk/operator/sample/ExposedIngressCondition.java b/sample-operators/webpage/src/main/java/io/javaoperatorsdk/operator/sample/ExposedIngressCondition.java
@@ -1,14 +1,12 @@
 package io.javaoperatorsdk.operator.sample;
 
-import java.util.Optional;
-
 import io.fabric8.kubernetes.api.model.networking.v1.Ingress;
 import io.javaoperatorsdk.operator.api.reconciler.Context;
 import io.javaoperatorsdk.operator.processing.dependent.workflow.Condition;
 
 public class ExposedIngressCondition implements Condition<Ingress, WebPage> {
   @Override
-  public boolean isMet(WebPage primary, Optional<Ingress> secondary, Context<WebPage> context) {
+  public boolean isMet(WebPage primary, Ingress secondary, Context<WebPage> context) {
     return primary.getSpec().getExposed();
   }
 }

Original file line number	Diff line number	Diff line change
`@@ -11,9 +11,9 @@`
`11`	`11`	`public class DependentResourceNode<R, P extends HasMetadata> {`
`12`	`12`
`13`	`13`	`private final DependentResource<R, P> dependentResource;`
`14`		`- private Condition reconcilePrecondition;`
`15`		`- private Condition deletePostcondition;`
`16`		`- private Condition readyPostcondition;`
	`14`	`+ private Condition<R, P> reconcilePrecondition;`
	`15`	`+ private Condition<R, P> deletePostcondition;`
	`16`	`+ private Condition<R, P> readyPostcondition;`
`17`	`17`	`private final List<DependentResourceNode> dependsOn = new LinkedList<>();`
`18`	`18`	`private final List<DependentResourceNode> parents = new LinkedList<>();`
`19`	`19`
`@@ -22,12 +22,12 @@ public DependentResourceNode(DependentResource<R, P> dependentResource) {`
`22`	`22`	`}`
`23`	`23`
`24`	`24`	`public DependentResourceNode(DependentResource<R, P> dependentResource,`
`25`		`- Condition reconcilePrecondition) {`
	`25`	`+ Condition<R, P> reconcilePrecondition) {`
`26`	`26`	`this(dependentResource, reconcilePrecondition, null);`
`27`	`27`	`}`
`28`	`28`
`29`	`29`	`public DependentResourceNode(DependentResource<R, P> dependentResource,`
`30`		`- Condition reconcilePrecondition, Condition deletePostcondition) {`
	`30`	`+ Condition<R, P> reconcilePrecondition, Condition<R, P> deletePostcondition) {`
`31`	`31`	`this.dependentResource = dependentResource;`
`32`	`32`	`this.reconcilePrecondition = reconcilePrecondition;`
`33`	`33`	`this.deletePostcondition = deletePostcondition;`
`@@ -37,11 +37,11 @@ public DependentResource<R, P> getDependentResource() {`
`37`	`37`	`return dependentResource;`
`38`	`38`	`}`
`39`	`39`
`40`		`- public Optional<Condition> getReconcilePrecondition() {`
	`40`	`+ public Optional<Condition<R, P>> getReconcilePrecondition() {`
`41`	`41`	`return Optional.ofNullable(reconcilePrecondition);`
`42`	`42`	`}`
`43`	`43`
`44`		`- public Optional<Condition> getDeletePostcondition() {`
	`44`	`+ public Optional<Condition<R, P>> getDeletePostcondition() {`
`45`	`45`	`return Optional.ofNullable(deletePostcondition);`
`46`	`46`	`}`
`47`	`47`
`@@ -63,12 +63,12 @@ public String toString() {`
`63`	`63`	`}`
`64`	`64`
`65`	`65`	`public DependentResourceNode<R, P> setReconcilePrecondition(`
`66`		`- Condition reconcilePrecondition) {`
	`66`	`+ Condition<R, P> reconcilePrecondition) {`
`67`	`67`	`this.reconcilePrecondition = reconcilePrecondition;`
`68`	`68`	`return this;`
`69`	`69`	`}`
`70`	`70`
`71`		`- public DependentResourceNode<R, P> setDeletePostcondition(Condition cleanupCondition) {`
	`71`	`+ public DependentResourceNode<R, P> setDeletePostcondition(Condition<R, P> cleanupCondition) {`
`72`	`72`	`this.deletePostcondition = cleanupCondition;`
`73`	`73`	`return this;`
`74`	`74`	`}`
`@@ -77,7 +77,7 @@ public Optional<Condition<R, P>> getReadyPostcondition() {`
`77`	`77`	`return Optional.ofNullable(readyPostcondition);`
`78`	`78`	`}`
`79`	`79`
`80`		`- public DependentResourceNode<R, P> setReadyPostcondition(Condition readyPostcondition) {`
	`80`	`+ public DependentResourceNode<R, P> setReadyPostcondition(Condition<R, P> readyPostcondition) {`
`81`	`81`	`this.readyPostcondition = readyPostcondition;`
`82`	`82`	`return this;`
`83`	`83`	`}`
Original file line number	Diff line number	Diff line change
`@@ -342,8 +342,7 @@ public UpdateControl<ConfigMap> reconcile(ConfigMap resource, Context<ConfigMap>`
`342`	`342`	`private static class TestCondition implements Condition<ConfigMap, ConfigMap> {`
`343`	`343`
`344`	`344`	`@Override`
`345`		`- public boolean isMet(ConfigMap primary, Optional<ConfigMap> secondary,`
`346`		`- Context<ConfigMap> context) {`
	`345`	`+ public boolean isMet(ConfigMap primary, ConfigMap secondary, Context<ConfigMap> context) {`
`347`	`346`	`return true;`
`348`	`347`	`}`
`349`	`348`	`}`
Original file line number	Diff line number	Diff line change
`@@ -1,14 +1,12 @@`
`1`	`1`	`package io.javaoperatorsdk.operator.sample;`
`2`	`2`
`3`		`-import java.util.Optional;`
`4`		`-`
`5`	`3`	`import io.fabric8.kubernetes.api.model.networking.v1.Ingress;`
`6`	`4`	`import io.javaoperatorsdk.operator.api.reconciler.Context;`
`7`	`5`	`import io.javaoperatorsdk.operator.processing.dependent.workflow.Condition;`
`8`	`6`
`9`	`7`	`public class ExposedIngressCondition implements Condition<Ingress, WebPage> {`
`10`	`8`	`@Override`
`11`		`- public boolean isMet(WebPage primary, Optional<Ingress> secondary, Context<WebPage> context) {`
	`9`	`+ public boolean isMet(WebPage primary, Ingress secondary, Context<WebPage> context) {`
`12`	`10`	`return primary.getSpec().getExposed();`
`13`	`11`	`}`
`14`	`12`	`}`