fix(manifests): revert manifest feat flag for disabling failing evalu…

…ations (#3821)

Co-authored-by: Zach Smith <zachsmith@Zachs-MBP-2.attlocal.net>

orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/manifest/ManifestEvaluator.java

@@ -49,7 +49,6 @@
 import lombok.Getter;
 import lombok.RequiredArgsConstructor;
 import org.springframework.beans.factory.annotation.Autowired;
-import org.springframework.beans.factory.annotation.Value;
 import org.springframework.stereotype.Component;
 import org.yaml.snakeyaml.Yaml;
 import org.yaml.snakeyaml.constructor.SafeConstructor;
@@ -67,20 +66,17 @@ public class ManifestEvaluator implements CloudProviderAware {
   private final ContextParameterProcessor contextParameterProcessor;
   private final OortService oortService;
   private final RetrySupport retrySupport;
-  private final Boolean failOnUnknownKeys;
 
   @Autowired
   public ManifestEvaluator(
       ArtifactUtils artifactUtils,
       ContextParameterProcessor contextParameterProcessor,
       OortService oortService,
-      RetrySupport retrySupport,
-      @Value("${manifest.evaluator.failOnUnknownKeys:true}") Boolean failOnUnknownKeys) {
+      RetrySupport retrySupport) {
     this.artifactUtils = artifactUtils;
     this.contextParameterProcessor = contextParameterProcessor;
     this.oortService = oortService;
     this.retrySupport = retrySupport;
-    this.failOnUnknownKeys = failOnUnknownKeys;
   }
 
   @RequiredArgsConstructor
@@ -199,7 +195,7 @@ private ImmutableList<Map<Object, Object>> getSpelEvaluatedManifests(
             contextParameterProcessor.buildExecutionContext(stage),
             /* allowUnknownKeys= */ true);
 
-    if (failOnUnknownKeys && processorResult.containsKey(PipelineExpressionEvaluator.SUMMARY)) {
+    if (processorResult.containsKey(PipelineExpressionEvaluator.SUMMARY)) {
       throw new IllegalStateException(
           String.format(
               "Failure evaluating manifest expressions: %s",

orca-clouddriver/src/test/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/providers/kubernetes/KubernetesJobRunnerSpec.groovy

@@ -43,8 +43,7 @@ class KubernetesJobRunnerSpec extends Specification {
         },
         Mock(ContextParameterProcessor),
         Mock(OortService),
-        new RetrySupport(),
-       true
+        new RetrySupport()
     )
     def stage = new StageExecutionImpl(PipelineExecutionImpl.newPipeline("test"), "runJob", [
       credentials: "abc", cloudProvider: "kubernetes",
@@ -79,8 +78,7 @@ class KubernetesJobRunnerSpec extends Specification {
         },
         Mock(ContextParameterProcessor),
         Mock(OortService),
-        new RetrySupport(),
-       true
+        new RetrySupport()
     )
     def stage = new StageExecutionImpl(PipelineExecutionImpl.newPipeline("test"), "runJob", [
       credentials: "abc", cloudProvider: "kubernetes",
@@ -111,7 +109,7 @@ class KubernetesJobRunnerSpec extends Specification {
     ContextParameterProcessor contextParameterProcessor = Mock(ContextParameterProcessor)
     RetrySupport retrySupport = new RetrySupport()
     ManifestEvaluator manifestEvaluator = new ManifestEvaluator(
-      artifactUtils, contextParameterProcessor, oortService, retrySupport, true
+      artifactUtils, contextParameterProcessor, oortService, retrySupport
     )
     def stage = new StageExecutionImpl(PipelineExecutionImpl.newPipeline("test"), "runJob", [
       credentials: "abc", cloudProvider: "kubernetes",

orca-clouddriver/src/test/java/com/netflix/spinnaker/orca/clouddriver/tasks/manifest/ManifestEvaluatorTest.java

@@ -18,11 +18,6 @@
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
-import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
-import static org.mockito.ArgumentMatchers.anyBoolean;
-import static org.mockito.ArgumentMatchers.anyMap;
-import static org.mockito.ArgumentMatchers.isNull;
-import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verifyNoInteractions;
 import static org.mockito.Mockito.when;
 
@@ -33,7 +28,6 @@
 import com.netflix.spinnaker.orca.clouddriver.OortService;
 import com.netflix.spinnaker.orca.clouddriver.tasks.manifest.ManifestContext.BindArtifact;
 import com.netflix.spinnaker.orca.clouddriver.tasks.manifest.ManifestContext.Source;
-import com.netflix.spinnaker.orca.pipeline.expressions.PipelineExpressionEvaluator;
 import com.netflix.spinnaker.orca.pipeline.model.StageExecutionImpl;
 import com.netflix.spinnaker.orca.pipeline.util.ArtifactUtils;
 import com.netflix.spinnaker.orca.pipeline.util.ContextParameterProcessor;
@@ -52,8 +46,7 @@ final class ManifestEvaluatorTest {
   private ManifestEvaluator manifestEvaluator;
 
   @Mock private ArtifactUtils artifactUtils;
-  private ContextParameterProcessor contextParameterProcessor =
-      mock(ContextParameterProcessor.class);
+  @Mock private ContextParameterProcessor contextParameterProcessor;
   @Mock private OortService oortService;
 
   private final List<Map<Object, Object>> manifests =
@@ -63,10 +56,7 @@ final class ManifestEvaluatorTest {
               .build());
 
   private final TypedString manifestString =
-      new TypedString("{\"metadata\": {\"name\": \"my-manifest\"}}");
-
-  private final TypedString spelManifestString =
-      new TypedString("{\"metadata\": {\"name\": \"${manifest}\"}}");
+      new TypedString("{'metadata': {'name': 'my-manifest'}}");
 
   private final String manifestsWithEmptyDocument =
       "---\n"
@@ -80,7 +70,7 @@ final class ManifestEvaluatorTest {
   void setup() {
     manifestEvaluator =
         new ManifestEvaluator(
-            artifactUtils, contextParameterProcessor, oortService, new RetrySupport(), true);
+            artifactUtils, contextParameterProcessor, oortService, new RetrySupport());
   }
 
   @Test
@@ -277,72 +267,4 @@ void optionalArtifacts() {
     ManifestEvaluator.Result result = manifestEvaluator.evaluate(stage, context);
     assertThat(result.getOptionalArtifacts()).isEqualTo(optionalArtifacts);
   }
-
-  @Test
-  void shouldThrowExceptionWhenFailedEvaluatingManifestExpressions() {
-    StageExecutionImpl stage = new StageExecutionImpl();
-    Artifact manifestArtifact =
-        Artifact.builder()
-            .artifactAccount("my-artifact-account")
-            .name("my-manifest-artifact")
-            .build();
-    DeployManifestContext context =
-        DeployManifestContext.builder()
-            .manifestArtifactId("my-manifest-artifact-id")
-            .skipExpressionEvaluation(false)
-            .source(Source.Artifact)
-            .build();
-
-    Map<String, Object> processorResult =
-        ImmutableMap.of(
-            PipelineExpressionEvaluator.SUMMARY,
-            "error",
-            "manifests",
-            ImmutableList.of(spelManifestString));
-
-    when(artifactUtils.getBoundArtifactForStage(stage, "my-manifest-artifact-id", null))
-        .thenReturn(manifestArtifact);
-    when(oortService.fetchArtifact(manifestArtifact))
-        .thenReturn(new Response("http://my-url", 200, "", ImmutableList.of(), spelManifestString));
-    when(contextParameterProcessor.process(anyMap(), isNull(), anyBoolean()))
-        .thenReturn(processorResult);
-
-    assertThatThrownBy(() -> manifestEvaluator.evaluate(stage, context))
-        .isInstanceOf(IllegalStateException.class);
-  }
-
-  @Test
-  void shouldSucceedWhenFailedEvaluatingManifestExpressionsWithFlagSet() {
-    ManifestEvaluator manifestEvaluator =
-        new ManifestEvaluator(
-            artifactUtils, contextParameterProcessor, oortService, new RetrySupport(), false);
-    StageExecutionImpl stage = new StageExecutionImpl();
-    Artifact manifestArtifact =
-        Artifact.builder()
-            .artifactAccount("my-artifact-account")
-            .name("my-manifest-artifact")
-            .build();
-    DeployManifestContext context =
-        DeployManifestContext.builder()
-            .manifestArtifactId("my-manifest-artifact-id")
-            .skipExpressionEvaluation(false)
-            .source(Source.Artifact)
-            .build();
-
-    Map<String, Object> processorResult =
-        ImmutableMap.of(
-            PipelineExpressionEvaluator.SUMMARY,
-            "error",
-            "manifests",
-            ImmutableList.of(spelManifestString));
-
-    when(artifactUtils.getBoundArtifactForStage(stage, "my-manifest-artifact-id", null))
-        .thenReturn(manifestArtifact);
-    when(oortService.fetchArtifact(manifestArtifact))
-        .thenReturn(new Response("http://my-url", 200, "", ImmutableList.of(), spelManifestString));
-    when(contextParameterProcessor.process(anyMap(), isNull(), anyBoolean()))
-        .thenReturn(processorResult);
-
-    assertDoesNotThrow(() -> manifestEvaluator.evaluate(stage, context));
-  }
 }