feat: move name is directly to dependent resource (#2250)

* feat: move name is directly to dependent resource - use this name when throwing aggregate exception Signed-off-by: Attila Mészáros <csviri@gmail.com> * refactor to use a dedicated interface for setting the name Signed-off-by: Attila Mészáros <csviri@gmail.com> * refactor: add default implementation for name() (#2255) Signed-off-by: Chris Laprun <claprun@redhat.com> Signed-off-by: Attila Mészáros <csviri@gmail.com> --------- Signed-off-by: Attila Mészáros <csviri@gmail.com> Signed-off-by: Chris Laprun <claprun@redhat.com> Co-authored-by: Chris Laprun <metacosm@users.noreply.github.com>
operator-framework · Mar 7, 2024 · 8c34b59 · 8c34b59
1 parent bd5297c
commit 8c34b59
Show file tree

Hide file tree

Showing 19 changed files with 116 additions and 98 deletions.
diff --git a/...src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/DependentResource.java b/...src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/DependentResource.java
@@ -68,4 +68,9 @@ static String defaultNameFor(Class<? extends DependentResource> dependentResourc
   default boolean isDeletable() {
     return this instanceof Deleter;
   }
+
+  default String name() {
+    return defaultNameFor(getClass());
+  }
+
 }
diff --git a/...k-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/NameSetter.java b/...k-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/NameSetter.java
@@ -0,0 +1,7 @@
+package io.javaoperatorsdk.operator.api.reconciler.dependent;
+
+public interface NameSetter {
+
+  void setName(String name);
+
+}
diff --git a/...main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java b/...main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java
@@ -11,13 +11,14 @@
 import io.javaoperatorsdk.operator.api.reconciler.ResourceDiscriminator;
 import io.javaoperatorsdk.operator.api.reconciler.dependent.Deleter;
 import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource;
+import io.javaoperatorsdk.operator.api.reconciler.dependent.NameSetter;
 import io.javaoperatorsdk.operator.api.reconciler.dependent.ReconcileResult;
 import io.javaoperatorsdk.operator.processing.dependent.Matcher.Result;
 import io.javaoperatorsdk.operator.processing.event.ResourceID;
 
 @Ignore
 public abstract class AbstractDependentResource<R, P extends HasMetadata>
-    implements DependentResource<R, P> {
+    implements DependentResource<R, P>, NameSetter {
   private static final Logger log = LoggerFactory.getLogger(AbstractDependentResource.class);
 
   private final boolean creatable = this instanceof Creator;
@@ -29,14 +30,21 @@ public abstract class AbstractDependentResource<R, P extends HasMetadata>
   private ResourceDiscriminator<R, P> resourceDiscriminator;
   private final DependentResourceReconciler<R, P> dependentResourceReconciler;
 
+  protected String name;
+
   @SuppressWarnings({"unchecked"})
   protected AbstractDependentResource() {
+    this(null);
+  }
+
+  protected AbstractDependentResource(String name) {
     creator = creatable ? (Creator<R, P>) this : null;
     updater = updatable ? (Updater<R, P>) this : null;
 
     dependentResourceReconciler = this instanceof BulkDependentResource
         ? new BulkDependentResourceReconciler<>((BulkDependentResource<R, P>) this)
         : new SingleDependentResourceReconciler<>(this);
+    this.name = name == null ? DependentResource.defaultNameFor(this.getClass()) : name;
   }
 
   /**
@@ -176,4 +184,13 @@ protected boolean isUpdatable() {
   public boolean isDeletable() {
     return deletable;
   }
+
+  @Override
+  public String name() {
+    return name;
+  }
+
+  public void setName(String name) {
+    this.name = name;
+  }
 }
diff --git a/...operatorsdk/operator/processing/dependent/AbstractEventSourceHolderDependentResource.java b/...operatorsdk/operator/processing/dependent/AbstractEventSourceHolderDependentResource.java
@@ -31,6 +31,11 @@ public abstract class AbstractEventSourceHolderDependentResource<R, P extends Ha
   protected String eventSourceNameToUse;
 
   protected AbstractEventSourceHolderDependentResource(Class<R> resourceType) {
+    this(resourceType, null);
+  }
+
+  protected AbstractEventSourceHolderDependentResource(Class<R> resourceType, String name) {
+    super(name);
     this.resourceType = resourceType;
   }
 

diff --git a/...javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java b/...javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java
@@ -43,7 +43,12 @@ public abstract class KubernetesDependentResource<R extends HasMetadata, P exten
 
   @SuppressWarnings("unchecked")
   public KubernetesDependentResource(Class<R> resourceType) {
-    super(resourceType);
+    this(resourceType, null);
+  }
+
+  @SuppressWarnings("unchecked")
+  public KubernetesDependentResource(Class<R> resourceType, String name) {
+    super(resourceType, name);
 
     usingCustomResourceUpdateMatcher = this instanceof ResourceUpdaterMatcher;
     updaterMatcher = usingCustomResourceUpdateMatcher

diff --git a/...a/io/javaoperatorsdk/operator/processing/dependent/workflow/AbstractWorkflowExecutor.java b/...a/io/javaoperatorsdk/operator/processing/dependent/workflow/AbstractWorkflowExecutor.java
@@ -18,7 +18,7 @@
 import io.javaoperatorsdk.operator.processing.event.ResourceID;
 
 @SuppressWarnings("rawtypes")
-public abstract class AbstractWorkflowExecutor<P extends HasMetadata> {
+abstract class AbstractWorkflowExecutor<P extends HasMetadata> {
 
   protected final Workflow<P> workflow;
   protected final P primary;
@@ -133,17 +133,16 @@ protected <R> void registerOrDeregisterEventSourceBasedOnActivation(
       boolean activationConditionMet,
       DependentResourceNode<R, P> dependentResourceNode) {
     if (dependentResourceNode.getActivationCondition().isPresent()) {
+      final var dr = dependentResourceNode.getDependentResource();
+      final var eventSourceRetriever = context.eventSourceRetriever();
       if (activationConditionMet) {
         var eventSource =
-            dependentResourceNode.getDependentResource().eventSource(context.eventSourceRetriever()
-                .eventSourceContextForDynamicRegistration());
+            dr.eventSource(eventSourceRetriever.eventSourceContextForDynamicRegistration());
         var es = eventSource.orElseThrow();
-        context.eventSourceRetriever()
-            .dynamicallyRegisterEventSource(dependentResourceNode.getName(), es);
+        eventSourceRetriever.dynamicallyRegisterEventSource(dr.name(), es);
 
       } else {
-        context.eventSourceRetriever()
-            .dynamicallyDeRegisterEventSource(dependentResourceNode.getName());
+        eventSourceRetriever.dynamicallyDeRegisterEventSource(dr.name());
       }
     }
   }

diff --git a/...ava/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultManagedWorkflow.java b/...ava/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultManagedWorkflow.java
@@ -12,8 +12,10 @@
 import io.javaoperatorsdk.operator.api.config.dependent.DependentResourceSpec;
 import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource;
 import io.javaoperatorsdk.operator.api.reconciler.dependent.EventSourceReferencer;
+import io.javaoperatorsdk.operator.api.reconciler.dependent.NameSetter;
 import io.javaoperatorsdk.operator.api.reconciler.dependent.managed.KubernetesClientAware;
 
+import static io.javaoperatorsdk.operator.api.reconciler.Constants.NO_VALUE_SET;
 import static io.javaoperatorsdk.operator.processing.dependent.workflow.Workflow.THROW_EXCEPTION_AUTOMATICALLY_DEFAULT;
 
 @SuppressWarnings("rawtypes")
@@ -77,13 +79,14 @@ public Workflow<P> resolve(KubernetesClient client,
       ControllerConfiguration<P> configuration) {
     final var alreadyResolved = new HashMap<String, DependentResourceNode>(orderedSpecs.size());
     for (DependentResourceSpec spec : orderedSpecs) {
-      final var node = new DependentResourceNode(spec.getName(),
+      final var dependentResource = resolve(spec, client, configuration);
+      final var node = new DependentResourceNode(
           spec.getReconcileCondition(),
           spec.getDeletePostCondition(),
           spec.getReadyCondition(),
           spec.getActivationCondition(),
-          resolve(spec, client, configuration));
-      alreadyResolved.put(node.getName(), node);
+          dependentResource);
+      alreadyResolved.put(dependentResource.name(), node);
       spec.getDependsOn()
           .forEach(depend -> node.addDependsOnRelation(alreadyResolved.get(depend)));
     }
@@ -104,6 +107,11 @@ private <R> DependentResource<R, P> resolve(DependentResourceSpec<R, P> spec,
         configuration.getConfigurationService().dependentResourceFactory()
             .createFrom(spec, configuration);
 
+    final var name = spec.getName();
+    if (name != null && !NO_VALUE_SET.equals(name) && dependentResource instanceof NameSetter) {
+      ((NameSetter) dependentResource).setName(name);
+    }
+
     if (dependentResource instanceof KubernetesClientAware) {
       ((KubernetesClientAware) dependentResource).setKubernetesClient(client);
     }

diff --git a/.../main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultWorkflow.java b/.../main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultWorkflow.java
@@ -20,7 +20,7 @@
  * @param <P> primary resource
  */
 @SuppressWarnings("rawtypes")
-public class DefaultWorkflow<P extends HasMetadata> implements Workflow<P> {
+class DefaultWorkflow<P extends HasMetadata> implements Workflow<P> {
 
   private final Map<String, DependentResourceNode> dependentResourceNodes;
   private final Set<DependentResourceNode> topLevelResources;
@@ -77,9 +77,9 @@ private Map<String, DependentResourceNode> toMap(Set<DependentResourceNode> node
           bottomLevelResource.remove(dependsOn);
         }
       }
-      map.put(node.getName(), node);
+      map.put(node.getDependentResource().name(), node);
     }
-    if (topLevelResources.size() == 0) {
+    if (topLevelResources.isEmpty()) {
       throw new IllegalStateException(
           "No top-level dependent resources found. This might indicate a cyclic Set of DependentResourceNode has been provided.");
     }

diff --git a/...java/io/javaoperatorsdk/operator/processing/dependent/workflow/DependentResourceNode.java b/...java/io/javaoperatorsdk/operator/processing/dependent/workflow/DependentResourceNode.java
@@ -8,29 +8,24 @@
 import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource;
 
 @SuppressWarnings("rawtypes")
-public class DependentResourceNode<R, P extends HasMetadata> {
+class DependentResourceNode<R, P extends HasMetadata> {
 
   private final List<DependentResourceNode> dependsOn = new LinkedList<>();
   private final List<DependentResourceNode> parents = new LinkedList<>();
-  private final String name;
+
   private Condition<R, P> reconcilePrecondition;
   private Condition<R, P> deletePostcondition;
   private Condition<R, P> readyPostcondition;
   private Condition<R, P> activationCondition;
   private final DependentResource<R, P> dependentResource;
 
-  DependentResourceNode(String name, DependentResource<R, P> dependentResource) {
-    this(name, null, null, null, null, dependentResource);
-  }
-
   DependentResourceNode(DependentResource<R, P> dependentResource) {
-    this(getNameFor(dependentResource), null, null, null, null, dependentResource);
+    this(null, null, null, null, dependentResource);
   }
 
-  public DependentResourceNode(String name, Condition<R, P> reconcilePrecondition,
+  public DependentResourceNode(Condition<R, P> reconcilePrecondition,
       Condition<R, P> deletePostcondition, Condition<R, P> readyPostcondition,
       Condition<R, P> activationCondition, DependentResource<R, P> dependentResource) {
-    this.name = name;
     this.reconcilePrecondition = reconcilePrecondition;
     this.deletePostcondition = deletePostcondition;
     this.readyPostcondition = readyPostcondition;
@@ -55,16 +50,10 @@ public List<DependentResourceNode> getParents() {
     return parents;
   }
 
-  public String getName() {
-    return name;
-  }
-
-
   public Optional<Condition<R, P>> getReconcilePrecondition() {
     return Optional.ofNullable(reconcilePrecondition);
   }
 
-
   public Optional<Condition<R, P>> getDeletePostcondition() {
     return Optional.ofNullable(deletePostcondition);
   }
@@ -106,18 +95,12 @@ public boolean equals(Object o) {
       return false;
     }
     DependentResourceNode<?, ?> that = (DependentResourceNode<?, ?>) o;
-    return name.equals(that.name);
+    return this.getDependentResource().name().equals(that.getDependentResource().name());
   }
 
   @Override
   public int hashCode() {
-    return name.hashCode();
-  }
-
-  @SuppressWarnings("rawtypes")
-  static String getNameFor(DependentResource dependentResource) {
-    return DependentResource.defaultNameFor(dependentResource.getClass()) + "#"
-        + dependentResource.hashCode();
+    return this.getDependentResource().name().hashCode();
   }
 
   @Override

diff --git a/.../main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowBuilder.java b/.../main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowBuilder.java
@@ -21,14 +21,9 @@ public class WorkflowBuilder<P extends HasMetadata> {
   private boolean isCleaner = false;
 
   public WorkflowBuilder<P> addDependentResource(DependentResource dependentResource) {
-    return addDependentResource(dependentResource, null);
-  }
-
-  public WorkflowBuilder<P> addDependentResource(DependentResource dependentResource, String name) {
-    currentNode = name == null ? new DependentResourceNode<>(dependentResource)
-        : new DependentResourceNode<>(name, dependentResource);
+    currentNode = new DependentResourceNode<>(dependentResource);
     isCleaner = isCleaner || dependentResource.isDeletable();
-    final var actualName = currentNode.getName();
+    final var actualName = dependentResource.name();
     dependentResourceNodes.put(actualName, currentNode);
     return this;
   }
@@ -70,8 +65,7 @@ public WorkflowBuilder<P> withActivationCondition(Condition activationCondition)
 
   DependentResourceNode getNodeByDependentResource(DependentResource<?, ?> dependentResource) {
     // first check by name
-    final var node =
-        dependentResourceNodes.get(DependentResourceNode.getNameFor(dependentResource));
+    final var node = dependentResourceNodes.get(dependentResource.name());
     if (node != null) {
       return node;
     } else {

diff --git a/...c/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResult.java b/...c/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResult.java
@@ -3,6 +3,7 @@
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Map.Entry;
+import java.util.stream.Collectors;
 
 import io.javaoperatorsdk.operator.AggregatedOperatorException;
 import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource;
@@ -37,22 +38,9 @@ public boolean erroredDependentsExist() {
 
   public void throwAggregateExceptionIfErrorsPresent() {
     if (erroredDependentsExist()) {
-      Map<String, Exception> exceptionMap = new HashMap<>();
-      Map<String, Integer> numberOfClasses = new HashMap<>();
-
-      for (Entry<DependentResource, Exception> entry : erroredDependents.entrySet()) {
-        String name = entry.getKey().getClass().getName();
-        var num = numberOfClasses.getOrDefault(name, 0);
-        if (num > 0) {
-          exceptionMap.put(name + NUMBER_DELIMITER + num, entry.getValue());
-        } else {
-          exceptionMap.put(name, entry.getValue());
-        }
-        numberOfClasses.put(name, num + 1);
-      }
-
-      throw new AggregatedOperatorException("Exception(s) during workflow execution.",
-          exceptionMap);
+       throw new AggregatedOperatorException("Exception(s) during workflow execution.",
+           erroredDependents.entrySet().stream()
+               .collect(Collectors.toMap(e -> e.getKey().name(), Entry::getValue)));
     }
   }
 }
diff --git a/...est/java/io/javaoperatorsdk/operator/api/config/ControllerConfigurationOverriderTest.java b/...est/java/io/javaoperatorsdk/operator/api/config/ControllerConfigurationOverriderTest.java
@@ -26,11 +26,7 @@
 import io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependentResourceConfigBuilder;
 import io.javaoperatorsdk.operator.processing.dependent.workflow.Condition;
 
-import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.assertFalse;
-import static org.junit.jupiter.api.Assertions.assertNotNull;
-import static org.junit.jupiter.api.Assertions.assertNull;
-import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assertions.*;
 
 class ControllerConfigurationOverriderTest {
   private final BaseConfigurationService configurationService = new BaseConfigurationService();
@@ -75,8 +71,7 @@ void overridingNSShouldPreserveUntouchedDependents() {
   private static class NamedDependentReconciler implements Reconciler<ConfigMap> {
 
     @Override
-    public UpdateControl<ConfigMap> reconcile(ConfigMap resource, Context<ConfigMap> context)
-        throws Exception {
+    public UpdateControl<ConfigMap> reconcile(ConfigMap resource, Context<ConfigMap> context) {
       return null;
     }
 

diff --git a/...operatorsdk/operator/api/config/dependent/DependentResourceConfigurationResolverTest.java b/...operatorsdk/operator/api/config/dependent/DependentResourceConfigurationResolverTest.java
@@ -24,10 +24,7 @@
 import io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependentResource;
 
 import static io.javaoperatorsdk.operator.api.config.dependent.DependentResourceConfigurationResolverTest.CustomAnnotationReconciler.DR_NAME;
-import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.assertNotNull;
-import static org.junit.jupiter.api.Assertions.assertNull;
-import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assertions.*;
 
 class DependentResourceConfigurationResolverTest {
 

diff --git a/...est/java/io/javaoperatorsdk/operator/processing/dependent/EmptyTestDependentResource.java b/...est/java/io/javaoperatorsdk/operator/processing/dependent/EmptyTestDependentResource.java
@@ -9,6 +9,8 @@
 public class EmptyTestDependentResource
     implements DependentResource<Deployment, TestCustomResource> {
 
+  private String name;
+
   @Override
   public ReconcileResult<Deployment> reconcile(TestCustomResource primary,
       Context<TestCustomResource> context) {
@@ -19,5 +21,14 @@ public ReconcileResult<Deployment> reconcile(TestCustomResource primary,
   public Class<Deployment> resourceType() {
     return Deployment.class;
   }
+
+  @Override
+  public String name() {
+    return name;
+  }
+
+  public void setName(String name) {
+    this.name = name;
+  }
 }