Allow direct deploy for applications without a project

5 files changed, 60 insertions, 69 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java
index ccfcc0e4b0d..43dd2b6e226 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java
@@ -278,7 +278,7 @@ public class ApplicationController {
             Version version;
             if (options.deployCurrentVersion)
                 version = application.currentVersion(controller, zone);
-            else if (application.deploymentJobs().isSelfTriggering()) // legacy mode: let the client decide
+            else if (canDeployDirectlyTo(zone, options))
                 version = options.vespaVersion.map(Version::new).orElse(controller.systemVersion());
             else if ( ! application.deploying().isPresent() && ! zone.environment().isManuallyDeployed())
                 return unexpectedDeployment(applicationId, zone, applicationPackage);
@@ -286,10 +286,8 @@ public class ApplicationController {
                 version = application.currentDeployVersion(controller, zone);
 
             // Ensure that the deploying change is tested
-            // FIXME: For now only for non-self-triggering applications - VESPA-8418
-            if ( ! application.deploymentJobs().isSelfTriggering() && 
-                 ! zone.environment().isManuallyDeployed() && 
-                 ! application.deploymentJobs().isDeployableTo(zone.environment(), application.deploying()))
+            if (! canDeployDirectlyTo(zone, options) &&
+                ! application.deploymentJobs().isDeployableTo(zone.environment(), application.deploying()))
                 throw new IllegalArgumentException("Rejecting deployment of " + application + " to " + zone +
                                                    " as pending " + application.deploying().get() +
                                                    " is untested");
@@ -305,7 +303,7 @@ public class ApplicationController {
                     application = application.withProjectId(options.screwdriverBuildJob.get().screwdriverId.value());
                 if (application.deploying().isPresent() && application.deploying().get() instanceof Change.ApplicationChange)
                     application = application.withDeploying(Optional.of(Change.ApplicationChange.of(revision)));
-                if ( ! triggeredWith(revision, application, jobType) && !zone.environment().isManuallyDeployed() && jobType != null) {
+                if ( ! triggeredWith(revision, application, jobType) && !canDeployDirectlyTo(zone, options) && jobType != null) {
                     // Triggering information is used to store which changes were made or attempted
                     // - For self-triggered applications we don't have any trigger information, so we add it here.
                     // - For all applications, we don't have complete control over which revision is actually built,
@@ -590,6 +588,11 @@ public class ApplicationController {
         return curator.lock(application, Duration.ofMinutes(10));
     }
 
+    /** Returns whether a direct deployment to given zone is allowed */
+    private static boolean canDeployDirectlyTo(Zone zone, DeployOptions options) {
+        return !options.screwdriverBuildJob.isPresent() || zone.environment().isManuallyDeployed();
+    }
+
     private static final class ApplicationRotation {
         
         private final ImmutableSet<String> cnames;
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTrigger.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTrigger.java
index d2af5fc50cb..8826ee5bf67 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTrigger.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTrigger.java
@@ -285,6 +285,11 @@ public class DeploymentTrigger {
             return application;
         }
 
+        // Ignore applications that are not associated with a project
+        if (!application.deploymentJobs().projectId().isPresent()) {
+            return application;
+        }
+
         log.info(String.format("Triggering %s for %s, %s: %s", jobType, application,
                                application.deploying().map(d -> "deploying " + d).orElse("restarted deployment"),
                                cause));
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ConfigServerClientMock.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ConfigServerClientMock.java
index 6018c99206e..13525969358 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ConfigServerClientMock.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ConfigServerClientMock.java
@@ -51,8 +51,17 @@ public class ConfigServerClientMock extends AbstractComponent implements ConfigS
     /** The exception to throw on the next prepare run, or null to continue normally */
     private RuntimeException prepareException = null;
     
-    /** The version given in the previous prepare call, or null if no call has been made */
-    public Optional<Version> lastPrepareVersion = null;
+    private Optional<Version> lastPrepareVersion = Optional.empty();
+
+    /** The version given in the previous prepare call, or empty if no call has been made */
+    public Optional<Version> lastPrepareVersion() {
+        return lastPrepareVersion;
+    }
+
+    /** Return map of applications that may have been activated */
+    public Map<ApplicationId, Boolean> activated() {
+        return Collections.unmodifiableMap(applicationActivated);
+    }
     
     @Override
     public PreparedApplication prepare(DeploymentId deployment, DeployOptions deployOptions, Set<String> rotationCnames, Set<Rotation> rotations, byte[] content) {
@@ -201,4 +210,5 @@ public class ConfigServerClientMock extends AbstractComponent implements ConfigS
                 ? endpoints.get(endpoint)
                 : result;
     }
+
 }
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java
index 91fca0d37d1..61cc84a07ba 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java
@@ -17,16 +17,10 @@ import com.yahoo.vespa.curator.Lock;
 import com.yahoo.vespa.hosted.controller.api.Tenant;
 import com.yahoo.vespa.hosted.controller.api.application.v4.model.DeployOptions;
 import com.yahoo.vespa.hosted.controller.api.application.v4.model.EndpointStatus;
-import com.yahoo.vespa.hosted.controller.api.application.v4.model.GitRevision;
-import com.yahoo.vespa.hosted.controller.api.application.v4.model.ScrewdriverBuildJob;
 import com.yahoo.vespa.hosted.controller.api.identifiers.AthensDomain;
 import com.yahoo.vespa.hosted.controller.api.identifiers.DeploymentId;
-import com.yahoo.vespa.hosted.controller.api.identifiers.GitBranch;
-import com.yahoo.vespa.hosted.controller.api.identifiers.GitCommit;
-import com.yahoo.vespa.hosted.controller.api.identifiers.GitRepository;
 import com.yahoo.vespa.hosted.controller.api.identifiers.Property;
 import com.yahoo.vespa.hosted.controller.api.identifiers.PropertyId;
-import com.yahoo.vespa.hosted.controller.api.identifiers.ScrewdriverId;
 import com.yahoo.vespa.hosted.controller.api.identifiers.TenantId;
 import com.yahoo.vespa.hosted.controller.api.identifiers.UserGroup;
 import com.yahoo.vespa.hosted.controller.api.integration.BuildService.BuildJob;
@@ -217,7 +211,7 @@ public class ControllerTest {
 
         app1 = applications.require(app1.id());
         assertEquals("First deployment gets system version", systemVersion, app1.deployedVersion().get());
-        assertEquals(systemVersion, tester.configServer().lastPrepareVersion.get());
+        assertEquals(systemVersion, tester.configServer().lastPrepareVersion().get());
 
         // Unexpected deployment
         tester.deploy(productionUsWest1, app1, applicationPackage);
@@ -240,7 +234,7 @@ public class ControllerTest {
 
         app1 = applications.require(app1.id());
         assertEquals("Application change preserves version", systemVersion, app1.deployedVersion().get());
-        assertEquals(systemVersion, tester.configServer().lastPrepareVersion.get());
+        assertEquals(systemVersion, tester.configServer().lastPrepareVersion().get());
 
         // A deployment to the new region gets the same version
         applicationPackage = new ApplicationPackageBuilder()
@@ -251,7 +245,7 @@ public class ControllerTest {
         tester.deployAndNotify(app1, applicationPackage, true, productionUsEast3);
         app1 = applications.require(app1.id());
         assertEquals("Application change preserves version", systemVersion, app1.deployedVersion().get());
-        assertEquals(systemVersion, tester.configServer().lastPrepareVersion.get());
+        assertEquals(systemVersion, tester.configServer().lastPrepareVersion().get());
 
         // Version upgrade changes system version
         Change.VersionChange change = new Change.VersionChange(newSystemVersion);
@@ -263,7 +257,7 @@ public class ControllerTest {
 
         app1 = applications.require(app1.id());
         assertEquals("Version upgrade changes version", newSystemVersion, app1.deployedVersion().get());
-        assertEquals(newSystemVersion, tester.configServer().lastPrepareVersion.get());
+        assertEquals(newSystemVersion, tester.configServer().lastPrepareVersion().get());
     }
 
     /** Adds a new version, higher than the current system version, makes it the system version and returns it */
@@ -533,44 +527,6 @@ public class ControllerTest {
     }
 
     @Test
-    public void testLegacyDeployments() {
-        // Setup system
-        DeploymentTester tester = new DeploymentTester();
-        ApplicationController applications = tester.controller().applications();
-        ApplicationPackage applicationPackage = new ApplicationPackageBuilder()
-                .environment(Environment.prod)
-                .region("us-east-3")
-                .build();
-        Version systemVersion = tester.controller().versionStatus().systemVersion().get().versionNumber();
-
-        Application app1 = tester.createApplication("application1", "tenant1", 1, 1L);
-        applications.store(app1.with(app1.deploymentJobs().asSelfTriggering(true)), applications.lock(app1.id()));
-
-        // Scenario: App already on 6.0, Upgrade to 6.1 (systemversion)
-        Zone prodZone = new Zone(Environment.prod, RegionName.from("us-east-3"));
-        Zone stagingZone = new Zone(Environment.staging, RegionName.from("us-east-3"));
-        Version existingVersion = Version.fromString("6.0");
-
-        // Add deployment on existing version
-        legacyDeploy(tester.controller(), app1, applicationPackage, prodZone, Optional.of(existingVersion), false);
-
-        // Add dev/perf deployment on old version to verify that this does not affect Initialize staging step. VESPA-8469
-        Version devVersion = Version.fromString("5.0");
-        legacyDeploy(tester.controller(), app1, applicationPackage, new Zone(Environment.dev, RegionName.from("us-east-1")), Optional.of(devVersion), false);
-        legacyDeploy(tester.controller(), app1, applicationPackage, new Zone(Environment.perf, RegionName.from("us-east-3")), Optional.of(devVersion), false);
-
-        // Initialize staging on existing version
-        legacyDeploy(tester.controller(), app1, applicationPackage, stagingZone, Optional.of(systemVersion), true);
-        app1 = applications.require(app1.id());
-        assertEquals(existingVersion, app1.currentDeployVersion(tester.controller(), stagingZone));
-
-        // Upgrade to the new version in staging
-        legacyDeploy(tester.controller(), app1, applicationPackage, stagingZone, Optional.of(systemVersion), false);
-        app1 = applications.require(app1.id());
-        assertEquals(systemVersion, app1.currentDeployVersion(tester.controller(), stagingZone));
-    }
-
-    @Test
     public void testDeployUntestedChangeFails() {
         ControllerTester tester = new ControllerTester();
         ApplicationController applications = tester.controller().applications();TenantId tenant = tester.createTenant("tenant1", "domain1", 11L);
@@ -587,16 +543,6 @@ public class ControllerTest {
         }
     }
 
-    private void legacyDeploy(Controller controller, Application application, ApplicationPackage applicationPackage, Zone zone, Optional<Version> version, boolean deployCurrentVersion) {
-        ScrewdriverId app1ScrewdriverId = new ScrewdriverId(String.valueOf(application.deploymentJobs().projectId().get()));
-        GitRevision app1RevisionId = new GitRevision(new GitRepository("repo"), new GitBranch("master"), new GitCommit("commit1"));
-        controller.applications().deployApplication(application.id(),
-                zone,
-                applicationPackage,
-                new DeployOptions(Optional.of(new ScrewdriverBuildJob(app1ScrewdriverId, app1RevisionId)), version, false, deployCurrentVersion));
-
-    }
-
     @Test
     public void testCleanupOfStaleDeploymentData() throws IOException {
         DeploymentTester tester = new DeploymentTester();
@@ -679,4 +625,31 @@ public class ControllerTest {
         assertEquals("fake-global-rotation-tenant1.app1", record.get().value());
     }
 
+    @Test
+    public void testDeployWithoutProjectId() {
+        DeploymentTester tester = new DeploymentTester();
+        tester.controllerTester().zoneRegistry().setSystem(SystemName.cd);
+        ApplicationPackage applicationPackage = new ApplicationPackageBuilder()
+                .environment(Environment.prod)
+                .region("cd-us-central-1")
+                .build();
+
+        // Create application
+        Application app = tester.createApplication("app1", "tenant1", 1, 2L);
+
+        // Direct deploy is allowed when project ID is missing
+        Zone zone = new Zone(Environment.prod, RegionName.from("cd-us-central-1"));
+        // Same options as used in our integration tests
+        DeployOptions options = new DeployOptions(Optional.empty(), Optional.empty(), false,
+                                                  false);
+        tester.controller().applications().deployApplication(app.id(), zone, applicationPackage, options);
+
+        assertTrue("Application deployed and activated",
+                   tester.controllerTester().configServer().activated().getOrDefault(app.id(), false));
+
+        assertTrue("No job status added",
+                   tester.applications().require(app.id()).deploymentJobs().jobStatus().isEmpty());
+
+    }
+
 }
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/UpgraderTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/UpgraderTest.java
index ccdfc8d042e..6709098ba06 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/UpgraderTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/UpgraderTest.java
@@ -57,7 +57,7 @@ public class UpgraderTest {
 
         assertEquals("New system version: Should upgrade Canaries", 2, tester.buildSystem().jobs().size());
         tester.completeUpgrade(canary0, version, "canary");
-        assertEquals(version, tester.configServer().lastPrepareVersion.get());
+        assertEquals(version, tester.configServer().lastPrepareVersion().get());
 
         tester.updateVersionStatus(version);
         tester.upgrader().maintain();
@@ -107,7 +107,7 @@ public class UpgraderTest {
 
         assertEquals("New system version: Should upgrade Canaries", 2, tester.buildSystem().jobs().size());
         tester.completeUpgrade(canary0, version, "canary");
-        assertEquals(version, tester.configServer().lastPrepareVersion.get());
+        assertEquals(version, tester.configServer().lastPrepareVersion().get());
 
         tester.updateVersionStatus(version);
         tester.upgrader().maintain();
@@ -187,7 +187,7 @@ public class UpgraderTest {
 
         assertEquals("New system version: Should upgrade Canaries", 2, tester.buildSystem().jobs().size());
         tester.completeUpgrade(canary0, version, "canary");
-        assertEquals(version, tester.configServer().lastPrepareVersion.get());
+        assertEquals(version, tester.configServer().lastPrepareVersion().get());
 
         tester.updateVersionStatus(version);
         tester.upgrader().maintain();