fix(managed): handle empty string manifest path when importing delive…

…ry config (#4058)
spinnaker · Feb 2, 2021 · a05abd8 · a05abd8
1 parent b297787
commit a05abd8
Show file tree

Hide file tree

Showing 2 changed files with 43 additions and 13 deletions.
diff --git a/orca-keel/src/main/kotlin/com/netflix/spinnaker/orca/keel/task/ImportDeliveryConfigTask.kt b/orca-keel/src/main/kotlin/com/netflix/spinnaker/orca/keel/task/ImportDeliveryConfigTask.kt
@@ -106,6 +106,10 @@ constructor(
       }
     }
 
+    if (context.manifest.isNullOrBlank()) {
+      context.manifest = "spinnaker.yml"
+    }
+
     // this is just a friend URI-like string to refer to the delivery config location in logs
     return "${context.repoType}://${context.projectKey}/${context.repositorySlug}/<manifestBaseDir>/${context.directory
       ?: ""}/${context.manifest}@${context.ref}"
@@ -215,7 +219,7 @@ constructor(
     var projectKey: String? = null,
     var repositorySlug: String? = null,
     var directory: String? = null, // as in, the directory *under* whatever manifest base path is configured in igor (e.g. ".netflix")
-    var manifest: String? = "spinnaker.yml",
+    var manifest: String? = null,
     var ref: String? = null,
     var attempt: Int = 1,
     val maxRetries: Int? = MAX_RETRIES,

diff --git a/orca-keel/src/test/kotlin/com/netflix/spinnaker/orca/keel/ImportDeliveryConfigTaskTests.kt b/orca-keel/src/test/kotlin/com/netflix/spinnaker/orca/keel/ImportDeliveryConfigTaskTests.kt
@@ -22,6 +22,7 @@ import com.netflix.spinnaker.kork.web.exceptions.InvalidRequestException
 import com.netflix.spinnaker.orca.KeelService
 import com.netflix.spinnaker.orca.api.pipeline.TaskResult
 import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus
+import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus.SUCCEEDED
 import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionType
 import com.netflix.spinnaker.orca.api.pipeline.models.Trigger
 import com.netflix.spinnaker.orca.config.KeelConfiguration
@@ -64,20 +65,19 @@ internal class ImportDeliveryConfigTaskTests : JUnit5Minutests {
   )
 
   data class Fixture(
-    val trigger: Trigger
-  ) {
-    companion object {
-      val objectMapper: ObjectMapper = KeelConfiguration().keelObjectMapper()
-    }
-
-    val manifestLocation = ManifestLocation(
+    val trigger: Trigger,
+    val manifestLocation: ManifestLocation = ManifestLocation(
       repoType = "stash",
       projectKey = "SPKR",
       repositorySlug = "keeldemo",
       directory = ".",
       manifest = "spinnaker.yml",
       ref = "refs/heads/master"
     )
+  ) {
+    companion object {
+      val objectMapper: ObjectMapper = KeelConfiguration().keelObjectMapper()
+    }
 
     val manifest = mapOf(
       "name" to "keeldemo-manifest",
@@ -152,7 +152,7 @@ internal class ImportDeliveryConfigTaskTests : JUnit5Minutests {
       }
       test("successfully retrieves manifest from SCM and publishes to keel") {
         val result = execute(manifestLocation.toMap())
-        expectThat(result.status).isEqualTo(ExecutionStatus.SUCCEEDED)
+        expectThat(result.status).isEqualTo(SUCCEEDED)
         verify(exactly = 1) {
           scmService.getDeliveryConfigManifest(
             manifestLocation.repoType,
@@ -193,7 +193,7 @@ internal class ImportDeliveryConfigTaskTests : JUnit5Minutests {
               it.remove("ref")
             }
           )
-          expectThat(result.status).isEqualTo(ExecutionStatus.SUCCEEDED)
+          expectThat(result.status).isEqualTo(SUCCEEDED)
           verify(exactly = 1) {
             scmService.getDeliveryConfigManifest(
               manifestLocation.repoType,
@@ -225,7 +225,7 @@ internal class ImportDeliveryConfigTaskTests : JUnit5Minutests {
       context("with fully-populated stage context") {
         test("disregards trigger and uses context information to retrieve manifest from SCM") {
           val result = execute(manifestLocation.toMap())
-          expectThat(result.status).isEqualTo(ExecutionStatus.SUCCEEDED)
+          expectThat(result.status).isEqualTo(SUCCEEDED)
           verify(exactly = 1) {
             scmService.getDeliveryConfigManifest(
               manifestLocation.repoType,
@@ -248,7 +248,7 @@ internal class ImportDeliveryConfigTaskTests : JUnit5Minutests {
               it.remove("ref")
             }
           )
-          expectThat(result.status).isEqualTo(ExecutionStatus.SUCCEEDED)
+          expectThat(result.status).isEqualTo(SUCCEEDED)
           verify(exactly = 1) {
             scmService.getDeliveryConfigManifest(
               manifestLocation.repoType,
@@ -265,7 +265,7 @@ internal class ImportDeliveryConfigTaskTests : JUnit5Minutests {
       context("with no information in stage context") {
         test("uses trigger information and defaults to fill in the blanks") {
           val result = execute(mutableMapOf())
-          expectThat(result.status).isEqualTo(ExecutionStatus.SUCCEEDED)
+          expectThat(result.status).isEqualTo(SUCCEEDED)
           verify(exactly = 1) {
             scmService.getDeliveryConfigManifest(
               (trigger as GitTrigger).source,
@@ -444,5 +444,31 @@ internal class ImportDeliveryConfigTaskTests : JUnit5Minutests {
         }
       }
     }
+
+    context("malformed input") {
+      fixture {
+        Fixture(
+          trigger = DefaultTrigger("manual"),
+          manifestLocation = ManifestLocation(
+            repoType = "stash",
+            projectKey = "SPKR",
+            repositorySlug = "keeldemo",
+            directory = ".",
+            manifest = "",
+            ref = "refs/heads/master"
+          )
+        )
+      }
+
+      test("successfully retrieves manifest from SCM and publishes to keel") {
+        val result = execute(manifestLocation.toMap())
+
+        expectThat(result.status) isEqualTo SUCCEEDED
+
+        verify(exactly = 1) {
+          scmService.getDeliveryConfigManifest(any(), any(), any(), any(), "spinnaker.yml", any())
+        }
+      }
+    }
   }
 }