fix: error message handling issues (#162)

Signed-off-by: Attila Mészáros <[email protected]>

Author: csviri

Diff for: src/main/java/io/javaoperatorsdk/operator/glue/reconciler/ValidationAndErrorHandler.java

@@ -22,6 +22,8 @@
 @Singleton
 public class ValidationAndErrorHandler {
 
+  public static final int MAX_MESSAGE_SIZE = 150;
+
   private static final Logger log = LoggerFactory.getLogger(ValidationAndErrorHandler.class);
 
   public static final String NON_UNIQUE_NAMES_FOUND_PREFIX = "Non unique names found: ";
@@ -37,7 +39,11 @@ public class ValidationAndErrorHandler {
           .setErrorMessage(NON_UNIQUE_NAMES_FOUND_PREFIX + String.join(",", ex.getDuplicates()));
       return ErrorStatusUpdateControl.updateStatus(resource).withNoRetry();
     } else {
-      resource.getStatus().setErrorMessage("Error during reconciliation");
+      var message = e.getMessage();
+      if (message.length() > MAX_MESSAGE_SIZE) {
+        message = message.substring(0, MAX_MESSAGE_SIZE) + "...";
+      }
+      resource.getStatus().setErrorMessage("Error: " + message);
       return ErrorStatusUpdateControl.updateStatus(resource);
     }
   }

Diff for: src/main/java/io/javaoperatorsdk/operator/glue/reconciler/glue/GlueReconciler.java

@@ -93,17 +93,7 @@ public UpdateControl<Glue> reconcile(Glue primary,
     informerRegister.deRegisterInformerOnResourceFlowChange(context, primary);
     result.throwAggregateExceptionIfErrorsPresent();
     patchRelatedResourcesStatus(context, primary);
-    return UpdateControl.noUpdate();
-  }
-
-  private boolean deletedGlueIfParentMarkedForDeletion(Context<Glue> context, Glue primary) {
-    var parent = getParentRelatedResource(primary, context);
-    if (parent.map(HasMetadata::isMarkedForDeletion).orElse(false)) {
-      context.getClient().resource(primary).delete();
-      return true;
-    } else {
-      return false;
-    }
+    return removeErrorMessageFromGlueStatusIfPresent(primary);
   }
 
   @Override
@@ -128,6 +118,34 @@ public DeleteControl cleanup(Glue primary, Context<Glue> context) {
     }
   }
 
+  @Override
+  public ErrorStatusUpdateControl<Glue> updateErrorStatus(Glue resource, Context<Glue> context,
+      Exception e) {
+    if (resource.getStatus() == null) {
+      resource.setStatus(new GlueStatus());
+    }
+    return validationAndErrorHandler.updateStatusErrorMessage(e, resource);
+  }
+
+  private boolean deletedGlueIfParentMarkedForDeletion(Context<Glue> context, Glue primary) {
+    var parent = getParentRelatedResource(primary, context);
+    if (parent.map(HasMetadata::isMarkedForDeletion).orElse(false)) {
+      context.getClient().resource(primary).delete();
+      return true;
+    } else {
+      return false;
+    }
+  }
+
+  private UpdateControl<Glue> removeErrorMessageFromGlueStatusIfPresent(Glue primary) {
+    if (primary.getStatus() != null && primary.getStatus().getErrorMessage() != null) {
+      primary.getStatus().setErrorMessage(null);
+      return UpdateControl.patchStatus(primary);
+    } else {
+      return UpdateControl.noUpdate();
+    }
+  }
+
   private void registerRelatedResourceInformers(Context<Glue> context,
       Glue glue) {
     glue.getSpec().getRelatedResources().forEach(r -> {
@@ -234,7 +252,6 @@ private GenericDependentResource createDependentResource(DependentResourceSpec s
     }
   }
 
-  // todo add workflow result?
   private void patchRelatedResourcesStatus(Context<Glue> context,
       Glue primary) {
 
@@ -345,15 +362,6 @@ private String parentFinalizer(String glueName) {
     return PARENT_GLUE_FINALIZER_PREFIX + glueName;
   }
 
-  @Override
-  public ErrorStatusUpdateControl<Glue> updateErrorStatus(Glue resource, Context<Glue> context,
-      Exception e) {
-    if (resource.getStatus() == null) {
-      resource.setStatus(new GlueStatus());
-    }
-    return validationAndErrorHandler.updateStatusErrorMessage(e, resource);
-  }
-
   public static boolean isGlueOfAGlueOperator(Glue glue) {
     var labelValue =
         glue.getMetadata().getLabels().get(GlueOperatorReconciler.FOR_GLUE_OPERATOR_LABEL_KEY);

Diff for: src/test/java/io/javaoperatorsdk/operator/glue/GlueTest.java

@@ -380,11 +380,37 @@ void simpleBulk() {
     });
 
     delete(glue);
-
     await().untilAsserted(
         () -> assertThat(getRelatedList(ConfigMap.class, glue.getMetadata().getName()).isEmpty()));
   }
 
+  @Test
+  void invalidGlueMessageHandling() {
+    var glueName = "invalid-glue";
+    var glue = createGlue("/glue/Invalid.yaml");
+
+    await().pollDelay(INITIAL_RECONCILE_WAIT_TIMEOUT).untilAsserted(() -> {
+      var g = get(Glue.class, glueName);
+      assertThat(g.getStatus()).isNotNull();
+      assertThat(g.getStatus().getErrorMessage()).isNotNull();
+    });
+
+    // fix error
+    glue.getSpec().getChildResources().get(1).setName("configMap2");
+    glue.getMetadata().setResourceVersion(null);
+    update(glue);
+
+    await().pollDelay(INITIAL_RECONCILE_WAIT_TIMEOUT).untilAsserted(() -> {
+      var g = get(Glue.class, glueName);
+      assertThat(g.getStatus().getErrorMessage()).isNull();
+    });
+
+    delete(glue);
+    await().pollDelay(INITIAL_RECONCILE_WAIT_TIMEOUT).untilAsserted(() -> {
+      var g = get(Glue.class, glueName);
+      assertThat(g).isNull();
+    });
+  }
 
 
   private List<Glue> testWorkflowList(int num) {

Diff for: src/test/resources/glue/Invalid.yaml

@@ -0,0 +1,23 @@
+# Invalid GLUE, presents resources with non-unique name
+apiVersion: io.javaoperatorsdk.operator.glue/v1beta1
+kind: Glue
+metadata:
+  name: invalid-glue
+spec:
+  childResources:
+    - name: configMap
+      resource:
+        apiVersion: v1
+        kind: ConfigMap
+        metadata:
+          name: simple-glue-configmap1
+        data:
+          key: "value1"
+    - name: configMap  # invalid: duplicate name
+      resource:
+        apiVersion: v1
+        kind: ConfigMap
+        metadata:
+          name: simple-glue-configmap2
+        data:
+          key: "value2"