Merge pull request #2887 from michaelbeaumont/mb/helm_fixes

Fix helm deployer with imageStrategy helm and fix test runner

pkg/skaffold/deploy/helm.go

@@ -277,23 +277,10 @@ func (h *HelmDeployer) deployRelease(ctx context.Context, out io.Writer, r lates
 	for k, v := range params {
 		var value string
 
-		if cfg := r.ImageStrategy.HelmImageConfig.HelmConventionConfig; cfg != nil {
-			dockerRef, err := docker.ParseReference(v.Tag)
-			if err != nil {
-				return nil, errors.Wrapf(err, "cannot parse the image reference %s", v.Tag)
-			}
-
-			if cfg.ExplicitRegistry {
-				if dockerRef.Domain == "" {
-					return nil, errors.Wrapf(err, "image reference %s has no domain", v.Tag)
-				}
-
-				value = fmt.Sprintf("%[1]s.registry=%s,%[1]s.repository=%s,%[1]s.tag=%s", k, dockerRef.Domain, dockerRef.Path, v.Tag)
-			} else {
-				value = fmt.Sprintf("%[1]s.repository=%s,%[1]s.tag=%s", k, dockerRef.BaseName, v.Tag)
-			}
-		} else {
-			value = fmt.Sprintf("%s=%s", k, v.Tag)
+		cfg := r.ImageStrategy.HelmImageConfig.HelmConventionConfig
+		value, err = getImageSetValueFromHelmStrategy(cfg, k, v.Tag)
+		if err != nil {
+			return nil, err
 		}
 
 		valuesSet[v.Tag] = true
@@ -417,6 +404,34 @@ func (h *HelmDeployer) getReleaseInfo(ctx context.Context, release string) (*buf
 	return bufio.NewReader(&releaseInfo), nil
 }
 
+func getImageSetValueFromHelmStrategy(cfg *latest.HelmConventionConfig, valueName string, tag string) (string, error) {
+	if cfg != nil {
+		dockerRef, err := docker.ParseReference(tag)
+		if err != nil {
+			return "", errors.Wrapf(err, "cannot parse the image reference %s", tag)
+		}
+
+		if cfg.ExplicitRegistry {
+			if dockerRef.Domain == "" {
+				return "", errors.New(fmt.Sprintf("image reference %s has no domain", tag))
+			}
+			return fmt.Sprintf(
+				"%[1]s.registry=%[2]s,%[1]s.repository=%[3]s,%[1]s.tag=%[4]s",
+				valueName,
+				dockerRef.Domain,
+				dockerRef.Path,
+				dockerRef.Tag,
+			), nil
+		}
+		return fmt.Sprintf(
+			"%[1]s.repository=%[2]s,%[1]s.tag=%[3]s",
+			valueName, dockerRef.BaseName,
+			dockerRef.Tag,
+		), nil
+	}
+	return fmt.Sprintf("%s=%s", valueName, tag), nil
+}
+
 // Retrieve info about all releases using helm get
 // Skaffold labels will be applied to each deployed k8s object
 // Since helm isn't always consistent with retrieving results, don't return errors here

pkg/skaffold/deploy/helm_test.go

@@ -541,7 +541,7 @@ func (m *MockHelm) RunCmd(c *exec.Cmd) error {
 	case "get":
 		return m.getResult
 	case "install":
-		if m.upgradeMatcher != nil && !m.installMatcher(c) {
+		if m.installMatcher != nil && !m.installMatcher(c) {
 			m.t.Errorf("install matcher failed to match commands: %+v", c.Args)
 		}
 		return m.installResult
@@ -674,6 +674,69 @@ func TestHelmDependencies(t *testing.T) {
 	}
 }
 
+func TestGetImageSetValueFromHelmStrategy(t *testing.T) {
+	tests := []struct {
+		description string
+		valueName   string
+		tag         string
+		expected    string
+		strategy    *latest.HelmConventionConfig
+		shouldErr   bool
+	}{
+		{
+			description: "Helm set values with no convention config",
+			valueName:   "image",
+			tag:         "skaffold-helm:1.0.0",
+			expected:    "image=skaffold-helm:1.0.0",
+			strategy:    nil,
+			shouldErr:   false,
+		},
+		{
+			description: "Helm set values with helm conventions",
+			valueName:   "image",
+			tag:         "skaffold-helm:1.0.0",
+			expected:    "image.repository=skaffold-helm,image.tag=1.0.0",
+			strategy:    &latest.HelmConventionConfig{},
+			shouldErr:   false,
+		},
+		{
+			description: "Helm set values with helm conventions and explicit registry value",
+			valueName:   "image",
+			tag:         "docker.io/skaffold-helm:1.0.0",
+			expected:    "image.registry=docker.io,image.repository=skaffold-helm,image.tag=1.0.0",
+			strategy: &latest.HelmConventionConfig{
+				ExplicitRegistry: true,
+			},
+			shouldErr: false,
+		},
+		{
+			description: "Invalid tag with helm conventions",
+			valueName:   "image",
+			tag:         "skaffold-helm:1.0.0,0",
+			expected:    "",
+			strategy:    &latest.HelmConventionConfig{},
+			shouldErr:   true,
+		},
+		{
+			description: "Helm set values with helm conventions and explicit registry value, but missing in tag",
+			valueName:   "image",
+			tag:         "skaffold-helm:1.0.0",
+			expected:    "",
+			strategy: &latest.HelmConventionConfig{
+				ExplicitRegistry: true,
+			},
+			shouldErr: true,
+		},
+	}
+	for _, test := range tests {
+		testutil.Run(t, test.description, func(t *testutil.T) {
+			values, err := getImageSetValueFromHelmStrategy(test.strategy, test.valueName, test.tag)
+			t.CheckError(test.shouldErr, err)
+			t.CheckDeepEqual(test.expected, values)
+		})
+	}
+}
+
 func TestExpandPaths(t *testing.T) {
 	homedir.DisableCache = true // for testing only