Remove broken step completion in DeploymentSpec and handle it in orchestration instead

3 files changed, 30 insertions, 88 deletions

diff --git a/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentSpec.java b/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentSpec.java
index 8c05b47e8e8..170547430cb 100644
--- a/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentSpec.java
+++ b/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentSpec.java
@@ -63,7 +63,7 @@ public class DeploymentSpec {
                           Optional<AthenzDomain> athenzDomain,
                           Optional<AthenzService> athenzService,
                           String xmlForm) {
-        this.steps = List.copyOf(completeSteps(steps));
+        this.steps = List.copyOf(steps);
         this.majorVersion = majorVersion;
         this.athenzDomain = athenzDomain;
         this.athenzService = athenzService;
@@ -72,50 +72,6 @@ public class DeploymentSpec {
         validateUpgradePoliciesOfIncreasingConservativeness(steps);
     }
 
-    /** Adds missing required steps and reorders steps to a permissible order */
-    private static List<DeploymentSpec.Step> completeSteps(List<DeploymentSpec.Step> inputSteps) {
-        List<Step> steps = new ArrayList<>(inputSteps);
-
-        // Add staging if required and missing
-        if (steps.stream().anyMatch(step -> step.concerns(Environment.prod)) &&
-            steps.stream().noneMatch(step -> step.concerns(Environment.staging))) {
-            steps.add(new DeploymentSpec.DeclaredZone(Environment.staging));
-        }
-
-        // Add test if required and missing
-        if (steps.stream().anyMatch(step -> step.concerns(Environment.staging)) &&
-            steps.stream().noneMatch(step -> step.concerns(Environment.test))) {
-            steps.add(new DeploymentSpec.DeclaredZone(Environment.test));
-        }
-
-        // Enforce order test, staging, prod
-        DeploymentSpec.DeclaredZone testStep = remove(Environment.test, steps);
-        if (testStep != null)
-            steps.add(0, testStep);
-        DeploymentSpec.DeclaredZone stagingStep = remove(Environment.staging, steps);
-        if (stagingStep != null)
-            steps.add(1, stagingStep);
-
-        return steps;
-    }
-
-    /**
-     * Removes the first occurrence of a deployment step to the given environment and returns it.
-     *
-     * @return the removed step, or null if it is not present
-     */
-    private static DeploymentSpec.DeclaredZone remove(Environment environment, List<DeploymentSpec.Step> steps) {
-        for (int i = 0; i < steps.size(); i++) {
-            if ( ! (steps.get(i) instanceof DeploymentSpec.DeclaredZone)) continue;
-            DeploymentSpec.DeclaredZone zoneStep = (DeploymentSpec.DeclaredZone)steps.get(i);
-            if (zoneStep.environment() == environment) {
-                steps.remove(i);
-                return zoneStep;
-            }
-        }
-        return null;
-    }
-
     /** Throw an IllegalArgumentException if the total delay exceeds 24 hours */
     private void validateTotalDelay(List<Step> steps) {
         long totalDelaySeconds = steps.stream().mapToLong(step -> (step.delay().getSeconds())).sum();
diff --git a/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecTest.java b/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecTest.java
index e4f52f573b0..ac605add892 100644
--- a/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecTest.java
+++ b/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecTest.java
@@ -77,9 +77,8 @@ public class DeploymentSpecTest {
         );
 
         DeploymentSpec spec = DeploymentSpec.fromXml(r);
-        assertEquals(2, spec.steps().size());
+        assertEquals(1, spec.steps().size());
         assertEquals(1, spec.requireInstance("default").steps().size());
-        assertTrue(spec.steps().get(0).concerns(Environment.test));
         assertTrue(spec.requireInstance("default").steps().get(0).concerns(Environment.staging));
         assertFalse(spec.requireInstance("default").deploysTo(Environment.test, Optional.empty()));
         assertTrue(spec.requireInstance("default").deploysTo(Environment.staging, Optional.empty()));
@@ -101,13 +100,9 @@ public class DeploymentSpecTest {
         );
 
         DeploymentSpec spec = DeploymentSpec.fromXml(r);
-        assertEquals(3, spec.steps().size());
+        assertEquals(1, spec.steps().size());
         assertEquals(2, spec.requireInstance("default").steps().size());
 
-        assertTrue(spec.steps().get(0).concerns(Environment.test));
-
-        assertTrue(spec.steps().get(1).concerns(Environment.staging));
-
         assertTrue(spec.requireInstance("default").steps().get(0).concerns(Environment.prod, Optional.of(RegionName.from("us-east1"))));
         assertFalse(((DeploymentSpec.DeclaredZone)spec.requireInstance("default").steps().get(0)).active());
 
@@ -500,6 +495,7 @@ public class DeploymentSpecTest {
     public void testNestedParallelAndSteps() {
         StringReader r = new StringReader(
                 "<deployment athenz-domain='domain'>" +
+                "   <staging />" +
                 "   <instance id='instance' athenz-service='in-service'>" +
                 "      <prod>" +
                 "         <parallel>" +
@@ -529,13 +525,12 @@ public class DeploymentSpecTest {
 
         DeploymentSpec spec = DeploymentSpec.fromXml(r);
         List<DeploymentSpec.Step> steps = spec.steps();
-        assertEquals(3, steps.size());
-        assertEquals("test", steps.get(0).toString());
-        assertEquals("staging", steps.get(1).toString());
-        assertEquals("instance 'instance'", steps.get(2).toString());
-        assertEquals(Duration.ofHours(4), steps.get(2).delay());
+        assertEquals(2, steps.size());
+        assertEquals("staging", steps.get(0).toString());
+        assertEquals("instance 'instance'", steps.get(1).toString());
+        assertEquals(Duration.ofHours(4), steps.get(1).delay());
 
-        List<DeploymentSpec.Step> instanceSteps = steps.get(2).steps();
+        List<DeploymentSpec.Step> instanceSteps = steps.get(1).steps();
         assertEquals(2, instanceSteps.size());
         assertEquals("4 parallel steps", instanceSteps.get(0).toString());
         assertEquals("prod.us-north-7", instanceSteps.get(1).toString());
@@ -590,12 +585,10 @@ public class DeploymentSpecTest {
 
         DeploymentSpec spec = DeploymentSpec.fromXml(r);
         List<DeploymentSpec.Step> steps = spec.steps();
-        assertEquals(3, steps.size());
-        assertEquals("test", steps.get(0).toString());
-        assertEquals("staging", steps.get(1).toString());
-        assertEquals("2 parallel steps", steps.get(2).toString());
+        assertEquals(1, steps.size());
+        assertEquals("2 parallel steps", steps.get(0).toString());
 
-        List<DeploymentSpec.Step> parallelSteps = steps.get(2).steps();
+        List<DeploymentSpec.Step> parallelSteps = steps.get(0).steps();
         assertEquals("instance 'instance0'", parallelSteps.get(0).toString());
         assertEquals("instance 'instance1'", parallelSteps.get(1).toString());
     }
@@ -620,12 +613,10 @@ public class DeploymentSpecTest {
 
         DeploymentSpec spec = DeploymentSpec.fromXml(r);
         List<DeploymentSpec.Step> steps = spec.steps();
-        assertEquals(5, steps.size());
-        assertEquals("test", steps.get(0).toString());
-        assertEquals("staging", steps.get(1).toString());
-        assertEquals("instance 'instance0'", steps.get(2).toString());
-        assertEquals("delay PT12H", steps.get(3).toString());
-        assertEquals("instance 'instance1'", steps.get(4).toString());
+        assertEquals(3, steps.size());
+        assertEquals("instance 'instance0'", steps.get(0).toString());
+        assertEquals("delay PT12H", steps.get(1).toString());
+        assertEquals("instance 'instance1'", steps.get(2).toString());
     }
 
     @Test
diff --git a/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecWithoutInstanceTest.java b/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecWithoutInstanceTest.java
index 00f0a1bfa41..50999759b77 100644
--- a/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecWithoutInstanceTest.java
+++ b/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecWithoutInstanceTest.java
@@ -71,9 +71,8 @@ public class DeploymentSpecWithoutInstanceTest {
         );
 
         DeploymentSpec spec = DeploymentSpec.fromXml(r);
-        assertEquals(2, spec.steps().size());
+        assertEquals(1, spec.steps().size());
         assertEquals(1, spec.requireInstance("default").steps().size());
-        assertTrue(spec.steps().get(0).concerns(Environment.test));
         assertTrue(spec.requireInstance("default").steps().get(0).concerns(Environment.staging));
         assertFalse(spec.requireInstance("default").deploysTo(Environment.test, Optional.empty()));
         assertTrue(spec.requireInstance("default").deploysTo(Environment.staging, Optional.empty()));
@@ -93,13 +92,9 @@ public class DeploymentSpecWithoutInstanceTest {
         );
 
         DeploymentSpec spec = DeploymentSpec.fromXml(r);
-        assertEquals(3, spec.steps().size());
+        assertEquals(1, spec.steps().size());
         assertEquals(2, spec.requireInstance("default").steps().size());
 
-        assertTrue(spec.steps().get(0).concerns(Environment.test));
-
-        assertTrue(spec.steps().get(1).concerns(Environment.staging));
-
         assertTrue(spec.requireInstance("default").steps().get(0).concerns(Environment.prod, Optional.of(RegionName.from("us-east1"))));
         assertFalse(((DeploymentSpec.DeclaredZone)spec.requireInstance("default").steps().get(0)).active());
 
@@ -352,6 +347,7 @@ public class DeploymentSpecWithoutInstanceTest {
     public void testNestedParallelAndSteps() {
         StringReader r = new StringReader(
                 "<deployment athenz-domain='domain' athenz-service='service'>" +
+                "   <staging />" +
                 "   <prod>" +
                 "      <parallel>" +
                 "         <region active='true'>us-west-1</region>" +
@@ -379,18 +375,17 @@ public class DeploymentSpecWithoutInstanceTest {
 
         DeploymentSpec spec = DeploymentSpec.fromXml(r);
         List<DeploymentSpec.Step> steps = spec.steps();
-        assertEquals(3, steps.size());
-        assertEquals("test", steps.get(0).toString());
-        assertEquals("staging", steps.get(1).toString());
-        assertEquals("instance 'default'", steps.get(2).toString());
-        assertEquals(Duration.ofHours(4), steps.get(2).delay());
-
-        List<DeploymentSpec.Step> instanceSteps = steps.get(2).steps();
-        assertEquals(2, instanceSteps.size());
-        assertEquals("4 parallel steps", instanceSteps.get(0).toString());
-        assertEquals("prod.us-north-7", instanceSteps.get(1).toString());
-
-        List<DeploymentSpec.Step> parallelSteps = instanceSteps.get(0).steps();
+        assertEquals(1, steps.size());
+        assertEquals("instance 'default'", steps.get(0).toString());
+        assertEquals(Duration.ofHours(4), steps.get(0).delay());
+
+        List<DeploymentSpec.Step> instanceSteps = steps.get(0).steps();
+        assertEquals(3, instanceSteps.size());
+        assertEquals("staging", instanceSteps.get(0).toString());
+        assertEquals("4 parallel steps", instanceSteps.get(1).toString());
+        assertEquals("prod.us-north-7", instanceSteps.get(2).toString());
+
+        List<DeploymentSpec.Step> parallelSteps = instanceSteps.get(1).steps();
         assertEquals(4, parallelSteps.size());
         assertEquals("prod.us-west-1", parallelSteps.get(0).toString());
         assertEquals("4 steps", parallelSteps.get(1).toString());