diff --git a/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/manifest/PatchManifestContext.java b/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/manifest/PatchManifestContext.java index 4019ab62e5..d777e3913b 100644 --- a/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/manifest/PatchManifestContext.java +++ b/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/manifest/PatchManifestContext.java @@ -16,13 +16,14 @@ package com.netflix.spinnaker.orca.clouddriver.tasks.manifest; +import com.fasterxml.jackson.annotation.JsonProperty; import com.fasterxml.jackson.databind.annotation.JsonDeserialize; import com.fasterxml.jackson.databind.annotation.JsonPOJOBuilder; +import com.google.common.collect.ImmutableList; import com.netflix.spinnaker.kork.artifacts.model.Artifact; -import java.util.Collections; import java.util.List; import java.util.Map; -import javax.annotation.Nullable; +import javax.annotation.Nonnull; import lombok.Builder; import lombok.Value; @@ -30,7 +31,7 @@ @JsonDeserialize(builder = PatchManifestContext.PatchManifestContextBuilder.class) @Builder(builderClassName = "PatchManifestContextBuilder", toBuilder = true) public class PatchManifestContext implements ManifestContext { - private Map patchBody; + private Object patchBody; private Source source; private String manifestArtifactId; @@ -43,10 +44,49 @@ public class PatchManifestContext implements ManifestContext { @Builder.Default private boolean skipExpressionEvaluation = false; - @Nullable + @Builder.Default + private PatchManifestContext.Options options = PatchManifestContext.Options.builder().build(); + + @Builder(builderClassName = "OptionsBuilder", toBuilder = true) + @JsonDeserialize(builder = PatchManifestContext.Options.OptionsBuilder.class) + @Value + static class Options { + @Builder.Default private MergeStrategy mergeStrategy = MergeStrategy.STRATEGIC; + @Builder.Default private boolean record = true; + + @JsonPOJOBuilder(withPrefix = "") + static class OptionsBuilder {} + } + + public enum MergeStrategy { + @JsonProperty("strategic") + STRATEGIC, + + @JsonProperty("json") + JSON, + + @JsonProperty("merge") + MERGE + } + + @Nonnull @Override public List> getManifests() { - return Collections.singletonList(patchBody); + /* + * Clouddriver expects patchBody to be a List when the mergeStrategy is json, and a Map otherwise. + * Prior to Spinnaker 1.15, Deck sent either a List or Map depending on merge strategy, and Orca + * deserialized it as an Object. In Spinnaker 1.15 and 1.16, a bug was introduced where patchBody + * was deserialized as a Map regardless of merge strategy. Starting in 1.17, Deck will send a List + * regardless of merge strategy, but we should handle receiving a Map from pipelines configured + * in 1.15 and 1.16. + */ + if (patchBody == null) { + return ImmutableList.of(); + } + if (patchBody instanceof List) { + return ImmutableList.copyOf((List) patchBody); + } + return ImmutableList.of((Map) patchBody); } @JsonPOJOBuilder(withPrefix = "") diff --git a/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/manifest/PatchManifestTask.java b/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/manifest/PatchManifestTask.java index 3000bb6126..2d708136ef 100644 --- a/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/manifest/PatchManifestTask.java +++ b/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/manifest/PatchManifestTask.java @@ -19,8 +19,10 @@ import com.netflix.spinnaker.orca.Task; import com.netflix.spinnaker.orca.TaskResult; import com.netflix.spinnaker.orca.clouddriver.tasks.AbstractCloudProviderAwareTask; +import com.netflix.spinnaker.orca.clouddriver.tasks.manifest.PatchManifestContext.MergeStrategy; import com.netflix.spinnaker.orca.pipeline.model.Stage; import java.util.HashMap; +import java.util.List; import java.util.Map; import javax.annotation.Nonnull; import lombok.RequiredArgsConstructor; @@ -39,11 +41,22 @@ public class PatchManifestTask extends AbstractCloudProviderAwareTask implements @Override public TaskResult execute(@Nonnull Stage stage) { PatchManifestContext context = stage.mapTo(PatchManifestContext.class); + MergeStrategy mergeStrategy = context.getOptions().getMergeStrategy(); ManifestEvaluator.Result result = manifestEvaluator.evaluate(stage, context); + List> patchBody = result.getManifests(); + + if (patchBody == null || patchBody.isEmpty()) { + throw new IllegalArgumentException( + "The Patch (Manifest) stage requires a valid patch body. Please add a patch body inline or with an artifact."); + } + if (mergeStrategy != MergeStrategy.JSON && patchBody.size() > 1) { + throw new IllegalArgumentException( + "Only one patch object is valid when patching with `strategic` and `merge` patch strategies."); + } Map task = new HashMap<>(stage.getContext()); task.put("source", "text"); - task.put("patchBody", result.getManifests().get(0)); + task.put("patchBody", mergeStrategy == MergeStrategy.JSON ? patchBody : patchBody.get(0)); task.put("requiredArtifacts", result.getRequiredArtifacts()); task.put("allArtifacts", result.getOptionalArtifacts()); diff --git a/orca-clouddriver/src/test/java/com/netflix/spinnaker/orca/clouddriver/tasks/manifest/ManifestContextTest.java b/orca-clouddriver/src/test/java/com/netflix/spinnaker/orca/clouddriver/tasks/manifest/ManifestContextTest.java index 5c717055d5..75fa67fab9 100644 --- a/orca-clouddriver/src/test/java/com/netflix/spinnaker/orca/clouddriver/tasks/manifest/ManifestContextTest.java +++ b/orca-clouddriver/src/test/java/com/netflix/spinnaker/orca/clouddriver/tasks/manifest/ManifestContextTest.java @@ -55,7 +55,7 @@ void deserialize() throws IOException { } @Test - void deserializePatchManifestNoArtifacts() throws IOException { + void deserializeInlineMapPatchManifest() throws IOException { String json = "{\n" + " \"manifestArtifactAccount\": \"account\",\n" @@ -70,8 +70,33 @@ void deserializePatchManifestNoArtifacts() throws IOException { PatchManifestContext context = new ObjectMapper().readValue(json, PatchManifestContext.class); assertThat(context.getSource()).isEqualTo(ManifestContext.Source.Text); assertThat(context.getManifestArtifactAccount()).isEqualTo("account"); - assertThat(context.getPatchBody()).containsOnlyKeys("spec"); - assertThat(context.getPatchBody().get("spec")) + assertThat(context.getManifests()).isNotNull(); + assertThat(context.getManifests().get(0)).containsOnlyKeys("spec"); + assertThat(context.getManifests().get(0).get("spec")) + .isEqualTo(Collections.singletonMap("replicas", "3")); + } + + @Test + void deserializeInlineListPatchManifest() throws IOException { + String json = + "{\n" + + " \"manifestArtifactAccount\": \"account\",\n" + + " \"source\": \"text\",\n" + + " \"patchBody\": [\n" + + " {\n" + + " \"spec\": {\n" + + " \"replicas\": \"3\"\n" + + " }\n" + + " }\n" + + " ]\n" + + "}"; + + PatchManifestContext context = new ObjectMapper().readValue(json, PatchManifestContext.class); + assertThat(context.getSource()).isEqualTo(ManifestContext.Source.Text); + assertThat(context.getManifestArtifactAccount()).isEqualTo("account"); + assertThat(context.getManifests()).isNotNull(); + assertThat(context.getManifests().get(0)).containsOnlyKeys("spec"); + assertThat(context.getManifests().get(0).get("spec")) .isEqualTo(Collections.singletonMap("replicas", "3")); } }