Don't remove other conditions when adding ConfigChangesPendingRestart (#2447)

Author: russgold

operator/src/main/java/oracle/kubernetes/weblogic/domain/model/DomainConditionType.java

@@ -13,13 +13,8 @@ DomainConditionType[] typesToRemove() {
   Available,
   ConfigChangesPendingRestart {
     @Override
-    String getStatusMessage(DomainCondition condition) {
-      return condition.getMessage();
-    }
-
-    @Override
-    String getStatusReason(DomainCondition condition) {
-      return condition.getReason();
+    DomainConditionType[] typesToRemove() {
+      return new DomainConditionType[] {};
     }
   },
   Failed {
@@ -34,19 +29,15 @@ String getStatusReason(DomainCondition condition) {
     }
 
     @Override
-    DomainConditionType[] typesToRemoveAlways() {
-      return new DomainConditionType[] {Progressing};
+    DomainConditionType[] typesToRemove() {
+      return new DomainConditionType[] {Progressing, Available};
     }
   };
 
   DomainConditionType[] typesToRemove() {
     return new DomainConditionType[] {Progressing, Available, Failed};
   }
 
-  DomainConditionType[] typesToRemoveAlways() {
-    return new DomainConditionType[] {};
-  }
-
   String getStatusMessage(DomainCondition condition) {
     return null;
   }

operator/src/main/java/oracle/kubernetes/weblogic/domain/model/DomainStatus.java

@@ -5,6 +5,7 @@
 
 import java.time.OffsetDateTime;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collections;
 import java.util.Comparator;
 import java.util.HashSet;
@@ -116,28 +117,24 @@ public DomainStatus(DomainStatus that) {
    */
   public DomainStatus addCondition(DomainCondition newCondition) {
     if (conditions.contains(newCondition)) {
-      conditions = conditions.stream()
-          .filter(c -> preserve(c, newCondition.getType().typesToRemoveAlways())).collect(Collectors.toList());
       return this;
     }
 
-    conditions = conditions.stream()
-        .filter(c -> preserve(c, newCondition.getType().typesToRemove())).collect(Collectors.toList());
+    conditions = conditions.stream().filter(c -> preserve(c, newCondition.getType())).collect(Collectors.toList());
 
     conditions.add(newCondition);
     reason = newCondition.getStatusReason();
     message = newCondition.getStatusMessage();
     return this;
   }
 
-  private boolean preserve(DomainCondition condition, DomainConditionType[] types) {
-    for (DomainConditionType type : types) {
-      if (condition.getType() == type) {
-        return false;
-      }
+  // Returns true if adding a condition of the new type should not remove the specified condition.
+  private boolean preserve(DomainCondition condition, DomainConditionType newType) {
+    if (newType == condition.getType() && !"True".equalsIgnoreCase(condition.getStatus())) {
+      return false;
+    } else {
+      return !Arrays.asList(newType.typesToRemove()).contains(condition.getType());
     }
-
-    return true;
   }
 
   /**

operator/src/test/java/oracle/kubernetes/weblogic/domain/model/DomainStatusTest.java

@@ -18,6 +18,7 @@
 import static oracle.kubernetes.operator.WebLogicConstants.SHUTDOWN_STATE;
 import static oracle.kubernetes.weblogic.domain.model.DomainConditionMatcher.hasCondition;
 import static oracle.kubernetes.weblogic.domain.model.DomainConditionType.Available;
+import static oracle.kubernetes.weblogic.domain.model.DomainConditionType.ConfigChangesPendingRestart;
 import static oracle.kubernetes.weblogic.domain.model.DomainConditionType.Failed;
 import static oracle.kubernetes.weblogic.domain.model.DomainConditionType.Progressing;
 import static oracle.kubernetes.weblogic.domain.model.DomainStatusTest.ClusterStatusMatcher.clusterStatus;
@@ -64,38 +65,12 @@ public void whenAddedConditionEqualsPresentCondition_ignoreIt() {
   }
 
   @Test
-  public void whenAddedConditionProgressing_addedItWithoutRemovingFailed() {
-    DomainCondition originalCondition1 = new DomainCondition(Failed).withStatus("True");
-    domainStatus.addCondition(originalCondition1);
-    DomainCondition originalCondition2 = new DomainCondition(Progressing).withStatus("True");
-    domainStatus.addCondition(originalCondition2);
+  public void whenAddedConditionIsFailed_retainOldFailedCondition() {
+    domainStatus.addCondition(new DomainCondition(Failed).withStatus("True").withMessage("problem 1"));
+    domainStatus.addCondition(new DomainCondition(Failed).withStatus("True").withMessage("problem 2"));
 
-    assertThat(domainStatus.getConditionWithType(Failed), sameInstance(originalCondition1));
-    assertThat(domainStatus.getConditionWithType(Progressing), sameInstance(originalCondition2));
-  }
-
-  @Test
-  public void whenAddedConditionFailed_removeProgressingCondition() {
-    DomainCondition originalCondition1 = new DomainCondition(Failed).withStatus("True");
-    domainStatus.addCondition(originalCondition1);
-    DomainCondition originalCondition2 = new DomainCondition(Progressing).withStatus("True");
-    domainStatus.addCondition(originalCondition2);
-
-    SystemClockTestSupport.increment();
-    domainStatus.addCondition(new DomainCondition(Failed).withStatus("True"));
-
-    assertThat(domainStatus.getConditionWithType(Failed), sameInstance(originalCondition1));
-    assertThat(domainStatus.getConditionWithType(Progressing), is(nullValue()));
-  }
-
-  @Test
-  public void whenAddedConditionIsFailed_replaceOldFailedCondition() {
-    domainStatus.addCondition(new DomainCondition(Failed).withStatus("False"));
-
-    domainStatus.addCondition(new DomainCondition(Failed).withStatus("True"));
-
-    assertThat(domainStatus, hasCondition(Failed).withStatus("True"));
-    assertThat(domainStatus, not(hasCondition(Failed).withStatus("False")));
+    assertThat(domainStatus, hasCondition(Failed).withMessage("problem 1"));
+    assertThat(domainStatus, hasCondition(Failed).withMessage("problem 2"));
   }
 
   @Test
@@ -178,6 +153,26 @@ public void whenAddedConditionIsProgress_doNotRmoveExistedFailedCondition() {
     assertThat(domainStatus, hasCondition(Progressing).withStatus("True"));
   }
 
+  @Test
+  public void whenAddedConditionIsConfigChangesPending_doNotRemoveExistingFailedCondition() {
+    domainStatus.addCondition(new DomainCondition(Failed).withStatus("True"));
+
+    domainStatus.addCondition(new DomainCondition(ConfigChangesPendingRestart).withStatus("True"));
+
+    assertThat(domainStatus, hasCondition(Failed));
+    assertThat(domainStatus, hasCondition(ConfigChangesPendingRestart));
+  }
+
+  @Test
+  public void whenAddedConditionIsConfigChangesPending_doNotRemoveExistingAvailableCondition() {
+    domainStatus.addCondition(new DomainCondition(Available));
+
+    domainStatus.addCondition(new DomainCondition(ConfigChangesPendingRestart).withStatus("True"));
+
+    assertThat(domainStatus, hasCondition(Available));
+    assertThat(domainStatus, hasCondition(ConfigChangesPendingRestart));
+  }
+
   @Test
   public void beforeConditionAdded_statusFailsPredicate() {
     assertThat(domainStatus.hasConditionWith(c -> c.hasType(Available)), is(false));