Only approve change requests pending approval. Replace noop change client with mock.

3 files changed, 77 insertions, 10 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ChangeRequestMaintainer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ChangeRequestMaintainer.java
index 71291e5d766..ca9ebe132fd 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ChangeRequestMaintainer.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ChangeRequestMaintainer.java
@@ -3,9 +3,11 @@ package com.yahoo.vespa.hosted.controller.maintenance;
 
 import com.yahoo.config.provision.SystemName;
 import com.yahoo.vespa.hosted.controller.Controller;
+import com.yahoo.vespa.hosted.controller.api.integration.vcmr.ChangeRequest;
 import com.yahoo.vespa.hosted.controller.api.integration.vcmr.ChangeRequestClient;
 
 import java.time.Duration;
+import java.util.List;
 import java.util.function.Predicate;
 import java.util.logging.Logger;
 import java.util.stream.Collectors;
@@ -17,10 +19,12 @@ public class ChangeRequestMaintainer extends ControllerMaintainer {
 
     private final Logger logger = Logger.getLogger(ChangeRequestMaintainer.class.getName());
     private final ChangeRequestClient changeRequestClient;
+    private final SystemName system;
 
     public ChangeRequestMaintainer(Controller controller, Duration interval) {
         super(controller, interval, null, SystemName.allOf(Predicate.not(SystemName::isPublic)));
         this.changeRequestClient = controller.serviceRegistry().changeRequestClient();
+        this.system = controller.system();
     }
 
 
@@ -29,18 +33,23 @@ public class ChangeRequestMaintainer extends ControllerMaintainer {
         var changeRequests = changeRequestClient.getUpcomingChangeRequests();
 
         if (!changeRequests.isEmpty()) {
-            logger.fine(() -> "Found the following upcoming change requests:");
-            changeRequests.forEach(changeRequest -> logger.fine(changeRequest::toString));
+            logger.info(() -> "Found the following upcoming change requests:");
+            changeRequests.forEach(changeRequest -> logger.info(changeRequest::toString));
         }
 
+        if (system.equals(SystemName.main))
+            approveChanges(changeRequests);
+
+        // TODO: Store in curator?
+        return true;
+    }
+
+    private void approveChanges(List<ChangeRequest> changeRequests) {
         var unapprovedRequests = changeRequests
                 .stream()
-                .filter(changeRequest -> !changeRequest.isApproved())
+                .filter(changeRequest -> changeRequest.getApproval() == ChangeRequest.Approval.REQUESTED)
                 .collect(Collectors.toList());
 
         changeRequestClient.approveChangeRequests(unapprovedRequests);
-
-        // TODO: Store in curator?
-        return true;
     }
 }
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ServiceRegistryMock.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ServiceRegistryMock.java
index e96eed2b6d9..326928b9463 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ServiceRegistryMock.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/integration/ServiceRegistryMock.java
@@ -27,7 +27,6 @@ import com.yahoo.vespa.hosted.controller.api.integration.resource.CostReportCons
 import com.yahoo.vespa.hosted.controller.api.integration.routing.GlobalRoutingService;
 import com.yahoo.vespa.hosted.controller.api.integration.routing.MemoryGlobalRoutingService;
 import com.yahoo.vespa.hosted.controller.api.integration.secrets.NoopTenantSecretService;
-import com.yahoo.vespa.hosted.controller.api.integration.secrets.TenantSecretService;
 import com.yahoo.vespa.hosted.controller.api.integration.stubs.DummyOwnershipIssues;
 import com.yahoo.vespa.hosted.controller.api.integration.stubs.DummySystemMonitor;
 import com.yahoo.vespa.hosted.controller.api.integration.stubs.LoggingDeploymentIssues;
@@ -36,7 +35,7 @@ import com.yahoo.vespa.hosted.controller.api.integration.stubs.MockMeteringClien
 import com.yahoo.vespa.hosted.controller.api.integration.stubs.MockRunDataStore;
 import com.yahoo.vespa.hosted.controller.api.integration.stubs.MockTesterCloud;
 import com.yahoo.vespa.hosted.controller.api.integration.vcmr.ChangeRequestClient;
-import com.yahoo.vespa.hosted.controller.api.integration.vcmr.NoopChangeRequestClient;
+import com.yahoo.vespa.hosted.controller.api.integration.vcmr.MockChangeRequestClient;
 
 /**
  * A mock implementation of a {@link ServiceRegistry} for testing purposes.
@@ -72,7 +71,7 @@ public class ServiceRegistryMock extends AbstractComponent implements ServiceReg
     private final ContainerRegistryMock containerRegistry = new ContainerRegistryMock();
     private final NoopTenantSecretService tenantSecretService = new NoopTenantSecretService();
     private final ArchiveService archiveService = new MockArchiveService();
-    private final ChangeRequestClient changeRequestClient = new NoopChangeRequestClient();
+    private final MockChangeRequestClient changeRequestClient = new MockChangeRequestClient();
 
     public ServiceRegistryMock(SystemName system) {
         this.zoneRegistryMock = new ZoneRegistryMock(system);
@@ -225,7 +224,7 @@ public class ServiceRegistryMock extends AbstractComponent implements ServiceReg
     }
 
     @Override
-    public ChangeRequestClient changeRequestClient() {
+    public MockChangeRequestClient changeRequestClient() {
         return changeRequestClient;
     }
 
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/ChangeRequestMaintainerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/ChangeRequestMaintainerTest.java
new file mode 100644
index 00000000000..b4df16348d2
--- /dev/null
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/ChangeRequestMaintainerTest.java
@@ -0,0 +1,59 @@
+package com.yahoo.vespa.hosted.controller.maintenance;
+
+import com.yahoo.vespa.hosted.controller.ControllerTester;
+import com.yahoo.vespa.hosted.controller.api.integration.vcmr.ChangeRequest;
+import com.yahoo.vespa.hosted.controller.api.integration.vcmr.ChangeRequestSource;
+import com.yahoo.vespa.hosted.controller.api.integration.vcmr.MockChangeRequestClient;
+import org.junit.Test;
+
+import java.time.Duration;
+import java.time.ZonedDateTime;
+import java.util.List;
+
+import static org.junit.Assert.*;
+
+/**
+ * @author olaa
+ */
+public class ChangeRequestMaintainerTest {
+
+    private final ControllerTester tester = new ControllerTester();
+    private final MockChangeRequestClient changeRequestClient = tester.serviceRegistry().changeRequestClient();
+    private final ChangeRequestMaintainer changeRequestMaintainer = new ChangeRequestMaintainer(tester.controller(), Duration.ofMinutes(1));
+
+    @Test
+    public void only_approve_requests_pending_approval() {
+
+        var upcomingChangeRequests = List.of(
+                newChangeRequest("id1", ChangeRequest.Approval.APPROVED),
+                newChangeRequest("id2", ChangeRequest.Approval.REQUESTED)
+        );
+
+        changeRequestClient.setUpcomingChangeRequests(upcomingChangeRequests);
+        changeRequestMaintainer.maintain();
+
+        var approvedChangeRequests = changeRequestClient.getApprovedChangeRequests();
+
+        assertEquals(1, approvedChangeRequests.size());
+        assertEquals("id2", approvedChangeRequests.get(0).getId());
+    }
+
+    private ChangeRequest newChangeRequest(String id, ChangeRequest.Approval approval) {
+        return new ChangeRequest.Builder()
+                .id(id)
+                .approval(approval)
+                .impact(ChangeRequest.Impact.VERY_HIGH)
+                .impactedSwitches(List.of())
+                .impactedHosts(List.of())
+                .changeRequestSource(new ChangeRequestSource.Builder()
+                        .plannedStartTime(ZonedDateTime.now())
+                        .plannedEndTime(ZonedDateTime.now())
+                        .id("some-id")
+                        .url("some-url")
+                        .system("some-system")
+                        .status(ChangeRequestSource.Status.CLOSED)
+                        .build())
+                .build();
+
+    }
+}
\ No newline at end of file