Merge pull request #1001 from maddisondavid/parse-tags-with-port-numbers

Correctly parse build tags that contain port numbers
GoogleContainerTools · Sep 24, 2018 · dfd7c0d · dfd7c0d
2 parents c7ff9d7 + face328
commit dfd7c0d
Show file tree

Hide file tree

Showing 4 changed files with 27 additions and 6 deletions.
diff --git a/pkg/skaffold/deploy/helm.go b/pkg/skaffold/deploy/helm.go
@@ -33,6 +33,7 @@ import (
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/tag"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/color"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/constants"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/v1alpha3"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
 	"github.com/pkg/errors"
@@ -133,8 +134,11 @@ func (h *HelmDeployer) deployRelease(ctx context.Context, out io.Writer, r v1alp
 	for k, v := range params {
 		setOpts = append(setOpts, "--set")
 		if r.ImageStrategy.HelmImageConfig.HelmConventionConfig != nil {
-			tagSplit := strings.Split(v.Tag, ":")
-			imageRepositoryTag := fmt.Sprintf("%s.repository=%s,%s.tag=%s", k, tagSplit[0], k, tagSplit[1])
+			dockerRef, err := docker.ParseReference(v.Tag)
+			if err != nil {
+				return nil, errors.Wrapf(err, "cannot parse the docker image reference %s", v.Tag)
+			}
+			imageRepositoryTag := fmt.Sprintf("%s.repository=%s,%s.tag=%s", k, dockerRef.BaseName, k, dockerRef.Tag)
 			setOpts = append(setOpts, imageRepositoryTag)
 		} else {
 			setOpts = append(setOpts, fmt.Sprintf("%s=%s", k, v.Tag))

diff --git a/pkg/skaffold/deploy/helm_test.go b/pkg/skaffold/deploy/helm_test.go
@@ -28,6 +28,7 @@ import (
 	"testing"
 
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/v1alpha3"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
 	"github.com/GoogleContainerTools/skaffold/testutil"
@@ -37,7 +38,7 @@ import (
 var testBuilds = []build.Artifact{
 	{
 		ImageName: "skaffold-helm",
-		Tag:       "skaffold-helm:3605e7bc17cf46e53f4d81c4cbc24e5b4c495184",
+		Tag:       "docker.io:5000/skaffold-helm:3605e7bc17cf46e53f4d81c4cbc24e5b4c495184",
 	},
 }
 
@@ -292,8 +293,12 @@ func TestHelmDeploy(t *testing.T) {
 				t:         t,
 				getResult: fmt.Errorf("not found"),
 				installMatcher: func(cmd *exec.Cmd) bool {
-					builds := strings.Split(testBuilds[0].Tag, ":")
-					expected := map[string]bool{fmt.Sprintf("image.repository=%s,image.tag=%s", builds[0], builds[1]): true}
+					dockerRef, err := docker.ParseReference(testBuilds[0].Tag)
+					if err != nil {
+						return false
+					}
+
+					expected := map[string]bool{fmt.Sprintf("image.repository=%s,image.tag=%s", dockerRef.BaseName, dockerRef.Tag): true}
 					for _, arg := range cmd.Args {
 						if expected[arg] {
 							return true

diff --git a/pkg/skaffold/docker/reference.go b/pkg/skaffold/docker/reference.go
@@ -21,6 +21,7 @@ import "github.com/docker/distribution/reference"
 // ImageReference is a parsed image name.
 type ImageReference struct {
 	BaseName       string
+	Tag            string
 	FullyQualified bool
 }
 
@@ -37,15 +38,18 @@ func ParseReference(image string) (*ImageReference, error) {
 	}
 
 	fullyQualified := false
+	tag := ""
 	switch n := r.(type) {
 	case reference.Tagged:
+		tag = n.Tag()
 		fullyQualified = n.Tag() != "latest"
 	case reference.Digested:
 		fullyQualified = true
 	}
 
 	return &ImageReference{
 		BaseName:       baseName,
+		Tag:            tag,
 		FullyQualified: fullyQualified,
 	}, nil
 }
diff --git a/pkg/skaffold/docker/reference_test.go b/pkg/skaffold/docker/reference_test.go
@@ -27,42 +27,49 @@ func TestParseReference(t *testing.T) {
 		description            string
 		image                  string
 		expectedName           string
+		expectedTag            string
 		expectedFullyQualified bool
 	}{
 		{
 			description:            "port and tag",
 			image:                  "host:1234/user/container:tag",
 			expectedName:           "host:1234/user/container",
+			expectedTag:            "tag",
 			expectedFullyQualified: true,
 		},
 		{
 			description:            "port",
 			image:                  "host:1234/user/container",
 			expectedName:           "host:1234/user/container",
+			expectedTag:            "",
 			expectedFullyQualified: false,
 		},
 		{
 			description:            "tag",
 			image:                  "host/user/container:tag",
 			expectedName:           "host/user/container",
+			expectedTag:            "tag",
 			expectedFullyQualified: true,
 		},
 		{
 			description:            "latest",
 			image:                  "host/user/container:latest",
 			expectedName:           "host/user/container",
+			expectedTag:            "latest",
 			expectedFullyQualified: false,
 		},
 		{
 			description:            "digest",
 			image:                  "gcr.io/k8s-skaffold/example@sha256:81daf011d63b68cfa514ddab7741a1adddd59d3264118dfb0fd9266328bb8883",
 			expectedName:           "gcr.io/k8s-skaffold/example",
+			expectedTag:            "",
 			expectedFullyQualified: true,
 		},
 		{
 			description:            "docker library",
 			image:                  "nginx:latest",
 			expectedName:           "nginx",
+			expectedTag:            "latest",
 			expectedFullyQualified: false,
 		},
 	}
@@ -72,7 +79,8 @@ func TestParseReference(t *testing.T) {
 			parsed, err := ParseReference(test.image)
 
 			testutil.CheckErrorAndDeepEqual(t, false, err, test.expectedName, parsed.BaseName)
-			testutil.CheckErrorAndDeepEqual(t, false, err, test.expectedFullyQualified, parsed.FullyQualified)
+			testutil.CheckDeepEqual(t, test.expectedTag, parsed.Tag)
+			testutil.CheckDeepEqual(t, test.expectedFullyQualified, parsed.FullyQualified)
 		})
 	}
 }