fix(helm): teach rosco to write helm overrides to a file

rosco-manifests/src/main/java/com/netflix/spinnaker/rosco/manifests/HelmBakeTemplateUtils.java

@@ -126,12 +126,17 @@ protected List<String> buildOverrideList(Map<String, Object> overrides) {
         .collect(Collectors.toList());
   }
 
+  /** Accessor for whether to expand artifact reference URIs */
+  protected boolean isExpandArtifactReferenceURIs() {
+    return (artifactStore != null && helmConfig.isExpandOverrides());
+  }
+
   /**
    * In the event that we encounter and ArtifactReferenceURI, we want to pull down that artifact
    * instead of using the raw URI as a value for helm.
    */
-  private Object expandArtifactReferenceURIs(Object value) {
-    if (artifactStore == null || !(helmConfig.isExpandOverrides() && value instanceof String)) {
+  protected Object expandArtifactReferenceURIs(Object value) {
+    if (!isExpandArtifactReferenceURIs() || !(value instanceof String)) {
       return value;
     }
 

rosco-manifests/src/main/java/com/netflix/spinnaker/rosco/manifests/helm/HelmTemplateUtils.java

@@ -182,11 +182,22 @@ private Path writeOverridesToFile(
       BakeManifestEnvironment env, Map<String, Object> overrides, boolean rawOverrides) {
     String fileName = OVERRIDES_FILE_PREFIX + UUID.randomUUID().toString() + YML_FILE_EXTENSION;
     Path filePath = env.resolvePath(fileName);
-    if (!rawOverrides) {
+
+    // If rawOverrides is true and expansion of artifact store references is
+    // disabled, there's no need to traverse the overrides map.  Use it as is.
+    //
+    // In other words, if we're meant to stringify overrides, or expand
+    // references, traverse the overrides map and do so.
+    if (!rawOverrides || isExpandArtifactReferenceURIs()) {
       overrides =
           overrides.entrySet().stream()
               .collect(
-                  Collectors.toMap(Map.Entry::getKey, entry -> String.valueOf(entry.getValue())));
+                  Collectors.toMap(
+                      Map.Entry::getKey,
+                      entry -> {
+                        Object newValue = expandArtifactReferenceURIs(entry.getValue());
+                        return rawOverrides ? newValue : String.valueOf(newValue);
+                      }));
     }
     try (Writer writer = Files.newBufferedWriter(filePath)) {
       yamlObjectMapper.writeValue(writer, overrides);

rosco-manifests/src/test/java/com/netflix/spinnaker/rosco/manifests/helm/HelmTemplateUtilsTest.java

@@ -68,6 +68,7 @@
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.Arguments;
 import org.junit.jupiter.params.provider.MethodSource;
+import org.junit.jupiter.params.provider.ValueSource;
 import org.springframework.http.HttpStatus;
 
 final class HelmTemplateUtilsTest {
@@ -84,15 +85,16 @@ final class HelmTemplateUtilsTest {
   /** Configuration for the default values in the event artifact store had been turned off. */
   private ArtifactStoreConfigurationProperties artifactStoreConfig;
 
+  private ArtifactStoreConfigurationProperties.HelmConfig helmConfig;
+
   @BeforeEach
   void init(TestInfo testInfo) {
     System.out.println("--------------- Test " + testInfo.getDisplayName());
 
     artifactDownloader = mock(ArtifactDownloader.class);
     helmConfigurationProperties = new RoscoHelmConfigurationProperties();
     artifactStoreConfig = new ArtifactStoreConfigurationProperties();
-    ArtifactStoreConfigurationProperties.HelmConfig helmConfig =
-        new ArtifactStoreConfigurationProperties.HelmConfig();
+    helmConfig = new ArtifactStoreConfigurationProperties.HelmConfig();
     artifactStoreConfig.setHelm(helmConfig);
     helmConfig.setExpandOverrides(false);
     helmTemplateUtils =
@@ -451,15 +453,25 @@ public void buildBakeRecipeIncludingCRDsWithHelm3() throws IOException {
     }
   }
 
-  @ParameterizedTest
+  @ParameterizedTest(
+      name =
+          "ensureOverrides reference: {0} overrides: {1} expected: {2} artifactStoreNull: {3}, expandOverrides: {4}, overridesInValuesFile: {5}")
   @MethodSource("ensureOverrides")
   public void ensureOverridesGetEvaluated(
       String reference,
       Map<String, Object> overrides,
-      String expected,
+      Map<String, Object> expectedMap,
       Boolean artifactStoreNull,
-      Boolean expandOverrides)
+      Boolean expandOverrides,
+      Boolean overridesInValuesFile)
       throws IOException {
+
+    // Assume that expectedMap has one element, and turn it into an expected argument for
+    // --set/--set-string
+    assertThat(expectedMap).hasSize(1);
+    Map.Entry<String, Object> entry = expectedMap.entrySet().iterator().next();
+    String expected = entry.getKey() + "=" + entry.getValue();
+
     ArtifactStore artifactStore = null;
     if (!artifactStoreNull) {
       artifactStore = mock(ArtifactStore.class);
@@ -468,6 +480,12 @@ public void ensureOverridesGetEvaluated(
     RoscoHelmConfigurationProperties helmConfigurationProperties =
         new RoscoHelmConfigurationProperties();
 
+    if (overridesInValuesFile) {
+      // Set the threshold so that rosco always uses a values file for
+      // overrides, no matter their size.
+      helmConfigurationProperties.setOverridesFileThreshold(1);
+    }
+
     // do not use the artifactStoreConfig field as we are trying to test various
     // values of expandOverrides
     ArtifactStoreConfigurationProperties artifactStoreConfiguration =
@@ -490,15 +508,62 @@ public void ensureOverridesGetEvaluated(
     try (BakeManifestEnvironment env = BakeManifestEnvironment.create()) {
       BakeRecipe recipe =
           helmTemplateUtils.buildCommand(request, List.of(), Path.of("template_path"), env);
-      assertThat(recipe.getCommand().contains(expected)).isTrue();
+      if (overridesInValuesFile) {
+        List<String> helmTemplateCommand = recipe.getCommand();
+        assertThat(Collections.frequency(helmTemplateCommand, "--values")).isEqualTo(1);
+        String lastValuesArgument =
+            helmTemplateCommand.get(helmTemplateCommand.lastIndexOf("--values") + 1);
+        assertThat(lastValuesArgument).matches(OVERRIDES_FILE_PATH_PATTERN);
+        List<String> overridesYamlContents = Files.readAllLines(Path.of(lastValuesArgument));
+        assertThat(helmTemplateCommand).doesNotContain("--set");
+        assertThat(helmTemplateCommand).doesNotContain("--set-string");
+        assertThat(helmTemplateCommand).contains("--values");
+        assertThat(
+                new ObjectMapper(new YAMLFactory())
+                    .readValue(
+                        String.join(System.lineSeparator(), overridesYamlContents),
+                        new TypeReference<Map<String, Object>>() {}))
+            .isEqualTo(expectedMap);
+      } else {
+        assertThat(recipe.getCommand().contains(expected)).isTrue();
+      }
     }
   }
 
   private static Stream<Arguments> ensureOverrides() {
     return Stream.of(
-        Arguments.of("test", Map.of("foo", "ref://bar/baz"), "foo=test", false, true),
-        Arguments.of("test", Map.of("foo", "ref://bar/baz"), "foo=ref://bar/baz", false, false),
-        Arguments.of("test", Map.of("foo", "ref://bar/baz"), "foo=ref://bar/baz", true, false));
+        Arguments.of(
+            "test", Map.of("foo", "ref://bar/baz"), Map.of("foo", "test"), false, true, false),
+        Arguments.of(
+            "test", Map.of("foo", "ref://bar/baz"), Map.of("foo", "test"), false, true, true),
+        Arguments.of(
+            "test",
+            Map.of("foo", "ref://bar/baz"),
+            Map.of("foo", "ref://bar/baz"),
+            false,
+            false,
+            false),
+        Arguments.of(
+            "test",
+            Map.of("foo", "ref://bar/baz"),
+            Map.of("foo", "ref://bar/baz"),
+            false,
+            false,
+            true),
+        Arguments.of(
+            "test",
+            Map.of("foo", "ref://bar/baz"),
+            Map.of("foo", "ref://bar/baz"),
+            true,
+            false,
+            false),
+        Arguments.of(
+            "test",
+            Map.of("foo", "ref://bar/baz"),
+            Map.of("foo", "ref://bar/baz"),
+            true,
+            false,
+            true));
   }
 
   @ParameterizedTest
@@ -626,10 +691,27 @@ public void testOverrideThresholdExceedsLimitWithRawOverridesAsFalse() throws IO
     }
   }
 
-  @Test
-  public void testOverrideThresholdExceedsLimitWithRawOverridesAsTrue() throws IOException {
+  @ParameterizedTest(
+      name = "testOverrideThresholdExceedsLimitWithRawOverridesAsTrue: expandOverrides {0}")
+  @ValueSource(booleans = {true, false})
+  public void testOverrideThresholdExceedsLimitWithRawOverridesAsTrue(boolean expandOverrides)
+      throws IOException {
     Artifact chartArtifact = Artifact.builder().name("test-artifact").build();
     Artifact valuesArtifact = Artifact.builder().name("test-artifact_values").build();
+
+    ArtifactStore artifactStore = null;
+    if (expandOverrides) {
+      artifactStore = mock(ArtifactStore.class);
+    }
+    helmConfig.setExpandOverrides(expandOverrides);
+
+    helmTemplateUtils =
+        new HelmTemplateUtils(
+            artifactDownloader,
+            Optional.ofNullable(artifactStore),
+            artifactStoreConfig,
+            helmConfigurationProperties);
+
     bakeManifestRequest = new HelmBakeManifestRequest();
     bakeManifestRequest.setInputArtifacts(ImmutableList.of(chartArtifact, valuesArtifact));
     bakeManifestRequest.setOverrides(