fix: keep the original template if template expansion fails (GoogleContainerTools#9503)

* fix: keep the original template if template expansion fails

* fix: keep the original template if template expansion fails

Author: ericzzzzzzz

integration/diagnose_test.go

@@ -25,6 +25,9 @@ import (
 	"text/template"
 
 	"github.com/GoogleContainerTools/skaffold/v2/integration/skaffold"
+	"github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/schema/latest"
+	"github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/schema/util"
+	"github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/yaml"
 	"github.com/GoogleContainerTools/skaffold/v2/testutil"
 )
 
@@ -83,27 +86,81 @@ func TestDiagnose(t *testing.T) {
 			configContents, err := os.ReadFile(filepath.Join(test.dir, "skaffold.yaml"))
 			t.CheckNoError(err)
 			templ, err := os.ReadFile(filepath.Join(test.dir, "diagnose.tmpl"))
+			t.CheckNoError(err)
 			tmpDir.Write("skaffold.yaml", string(configContents))
 			args := []string{"--yaml-only", "--output", tmpDir.Path(test.outputFile), "-f", tmpDir.Path("skaffold.yaml")}
 			args = append(args, test.args...)
 			skaffold.Diagnose(args...).
 				InDir(test.dir).RunOrFail(t.T)
-			t.CheckNoError(err)
 			outTemplate := template.Must(template.New("tmpl").Parse(string(templ)))
 			cwd, err := filepath.Abs(test.dir)
 			t.CheckNoError(err)
 			expected := &bytes.Buffer{}
 			outTemplate.Execute(expected, map[string]string{"Root": cwd})
 
 			outputPath := tmpDir.Path(test.outputFile)
-			t.CheckNoError(err)
 			out, err := os.ReadFile(outputPath)
 			t.CheckNoError(err)
 			t.CheckDeepEqual(expected.String(), string(out), testutil.YamlObj(t.T))
 		})
 	}
 }
 
+// During the schema upgrade(v2beta28->v2beta29),  Skaffold injects setTemplate fields into the configuration if a legacy Helm deployer is configured.
+// These injected fields contain templates, and we want to ensure that when expanding them with Go templates, the original field values remain unchanged
+// when environment variables are not set.  This is important because users who use the skaffold diagnose command on the old schema
+// with Helm might not be aware  of the existence of these templated fields, leading to templating failures.
+func TestDiagnoseTemplatingNotAllEnvsSet(t *testing.T) {
+	tests := []struct {
+		description string
+		dir         string
+		outputFile  string
+		args        []string
+		envs        map[string]string
+	}{
+		{
+			description: "apply replacements to templates in skaffold.yaml",
+			dir:         "testdata/diagnose/not-all-envs-set",
+			outputFile:  "abc.txt",
+			args:        []string{"--enable-templating"},
+			envs:        map[string]string{"AAA": "aaa"},
+		},
+	}
+
+	for _, test := range tests {
+		MarkIntegrationTest(t, CanRunWithoutGcp)
+		testutil.Run(t, test.description, func(t *testutil.T) {
+			if test.envs != nil {
+				for k, v := range test.envs {
+					t.Setenv(k, v)
+				}
+			}
+			tmpDir := testutil.NewTempDir(t.T)
+			configContents, err := os.ReadFile(filepath.Join(test.dir, "skaffold.yaml"))
+			t.CheckNoError(err)
+			tmpDir.Write("skaffold.yaml", string(configContents))
+			outputPath := tmpDir.Path(test.outputFile)
+			args := []string{"--yaml-only", "--output", outputPath, "-f", tmpDir.Path("skaffold.yaml")}
+			args = append(args, test.args...)
+			skaffold.Diagnose(args...).
+				InDir(test.dir).RunOrFail(t.T)
+			out, err := os.ReadFile(outputPath)
+			t.CheckNoError(err)
+			var conf latest.SkaffoldConfig
+			yaml.Unmarshal(out, &conf)
+			// templates unchanged
+			t.CheckDeepEqual(conf.Deploy.LegacyHelmDeploy.Releases[0].SetValueTemplates, util.FlatMap{"image.repository": "{{.IMAGE_REPO_test_image}}",
+				"image.tag": "{{.IMAGE_TAG_test_image}}@{{.IMAGE_DIGEST_test_image}}",
+			})
+			cwd, err := filepath.Abs(test.dir)
+			t.CheckNoError(err)
+
+			// templates successfully expanded.
+			t.CheckDeepEqual(conf.Deploy.LegacyHelmDeploy.Releases[0].ValuesFiles[0], cwd+"/aaa/test-values.yaml")
+		})
+	}
+}
+
 func folders(root string) ([]string, error) {
 	var folders []string
 

integration/testdata/diagnose/not-all-envs-set/skaffold.yaml

@@ -0,0 +1,19 @@
+apiVersion: skaffold/v2beta26
+kind: Config
+metadata:
+  name: cio-cloud-project-mgmt-api
+build:
+  artifacts:
+    - image: test-image
+deploy:
+  helm:
+    releases:
+      - name: test-release
+        artifactOverrides:
+          image: test-image
+        valuesFiles:
+          - '{{.AAA}}/test-values.yaml'
+        remoteChart: "oci://test-chart"
+        version: 1.21.2
+        imageStrategy:
+          helm: {}

pkg/skaffold/tags/templates.go

@@ -17,7 +17,6 @@ limitations under the License.
 package tags
 
 import (
-	"fmt"
 	"reflect"
 	"slices"
 	"strings"
@@ -118,13 +117,13 @@ func expandTemplate(v reflect.Value) error {
 	switch v.Kind() {
 	case reflect.String:
 		updated, err := util.ExpandEnvTemplate(v.String(), nil)
-		if strings.Contains(updated, "<no value>") {
-			return fmt.Errorf("environment variables missing for template keys")
-		}
 		if err != nil {
 			return err
 		}
-		v.SetString(updated)
+		// we want to keep the original template if expanding fails, otherwise update the value with expanded result.
+		if !strings.Contains(updated, "<no value>") {
+			v.SetString(updated)
+		}
 	case reflect.Ptr:
 		return expandTemplate(v.Elem())
 	case reflect.Slice, reflect.Array:
@@ -142,13 +141,13 @@ func expandTemplate(v reflect.Value) error {
 				}
 			} else if vv.Kind() == reflect.String {
 				updated, err := util.ExpandEnvTemplate(vv.String(), nil)
-				if strings.Contains(updated, "<no value>") {
-					return fmt.Errorf("environment variables missing for template keys")
-				}
 				if err != nil {
 					return err
 				}
-				v.SetMapIndex(key, reflect.ValueOf(updated))
+				// we want to keep the original template if expanding fails, otherwise update the value with expanded result.
+				if !strings.Contains(updated, "<no value>") {
+					v.SetMapIndex(key, reflect.ValueOf(updated))
+				}
 			}
 		}
 	}

pkg/skaffold/tags/templates_test.go

@@ -75,11 +75,10 @@ func TestApplyTemplates(t *testing.T) {
 			envs:    map[string]string{"SECOND": "second", "THIRD": "third"},
 		},
 		{
-			name:    "Map of strings, no value",
-			input:   testStruct{MapString: map[string]string{"first": "first", "second": "{{.SECOND}}", "third": "{{.THIRD}}"}},
-			want:    testStruct{MapString: map[string]string{"first": "first", "second": "{{.SECOND}}", "third": "{{.THIRD}}"}},
-			wantErr: true,
-			envs:    map[string]string{},
+			name:  "Map of strings, keep the original template",
+			input: testStruct{MapString: map[string]string{"first": "first", "second": "{{.SECOND}}", "third": "{{.THIRD}}"}},
+			want:  testStruct{MapString: map[string]string{"first": "first", "second": "{{.SECOND}}", "third": "{{.THIRD}}"}},
+			envs:  map[string]string{},
 		},
 		{
 			name:    "Map of pointers to strings",
@@ -108,14 +107,13 @@ func TestApplyTemplates(t *testing.T) {
 			wantErr: false,
 		},
 		{
-			name: "string not found - <no value>",
+			name: "string not found - keep the original template",
 			input: testStruct{
 				SimpleString: "{{ .ENV_VAR }}",
 			},
 			want: testStruct{
 				SimpleString: "{{ .ENV_VAR }}",
 			},
-			wantErr: true,
 		},
 		{
 			name: "invalid template",