Fix issue where default-repo wasn't being added to artifact tags (#5341…

…) (#5397)
GoogleContainerTools · Mar 10, 2021 · 802ef8a · 802ef8a
1 parent 1cc7613
commit 802ef8a
Show file tree

Hide file tree

Showing 4 changed files with 96 additions and 8 deletions.
diff --git a/cmd/skaffold/app/cmd/deploy.go b/cmd/skaffold/app/cmd/deploy.go
@@ -59,7 +59,7 @@ func doDeploy(ctx context.Context, out io.Writer) error {
 		for _, cfg := range configs {
 			artifacts = append(artifacts, cfg.Build.Artifacts...)
 		}
-		buildArtifacts, err := getBuildArtifactsAndSetTags(r, artifacts)
+		buildArtifacts, err := getBuildArtifactsAndSetTags(artifacts, r.ApplyDefaultRepo)
 		if err != nil {
 			tips.PrintUseRunVsDeploy(out)
 			return err

diff --git a/cmd/skaffold/app/cmd/test.go b/cmd/skaffold/app/cmd/test.go
@@ -44,7 +44,7 @@ func doTest(ctx context.Context, out io.Writer) error {
 		for _, c := range configs {
 			artifacts = append(artifacts, c.Build.Artifacts...)
 		}
-		buildArtifacts, err := getBuildArtifactsAndSetTags(r, artifacts)
+		buildArtifacts, err := getBuildArtifactsAndSetTags(artifacts, r.ApplyDefaultRepo)
 		if err != nil {
 			tips.PrintForTest(out)
 			return err

diff --git a/cmd/skaffold/app/cmd/util.go b/cmd/skaffold/app/cmd/util.go
@@ -21,25 +21,32 @@ import (
 
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/tag"
-	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/runner"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
 )
 
-func getBuildArtifactsAndSetTags(r runner.Runner, artifacts []*latest.Artifact) ([]build.Artifact, error) {
+// DefaultRepoFn takes an image tag and returns either a new tag with the default repo prefixed, or the original tag if
+// no default repo is specified.
+type DefaultRepoFn func(string) (string, error)
+
+func getBuildArtifactsAndSetTags(artifacts []*latest.Artifact, defaulterFn DefaultRepoFn) ([]build.Artifact, error) {
 	buildArtifacts, err := mergeBuildArtifacts(fromBuildOutputFile.BuildArtifacts(), preBuiltImages.Artifacts(), artifacts)
 	if err != nil {
 		return nil, err
 	}
 
-	for _, artifact := range buildArtifacts {
-		tag, err := r.ApplyDefaultRepo(artifact.Tag)
+	return applyDefaultRepoToArtifacts(buildArtifacts, defaulterFn)
+}
+
+func applyDefaultRepoToArtifacts(artifacts []build.Artifact, defaulterFn DefaultRepoFn) ([]build.Artifact, error) {
+	for i := range artifacts {
+		updatedTag, err := defaulterFn(artifacts[i].Tag)
 		if err != nil {
 			return nil, err
 		}
-		artifact.Tag = tag
+		artifacts[i].Tag = updatedTag
 	}
 
-	return buildArtifacts, nil
+	return artifacts, nil
 }
 
 func mergeBuildArtifacts(fromFile, fromCLI []build.Artifact, artifacts []*latest.Artifact) ([]build.Artifact, error) {

diff --git a/cmd/skaffold/app/cmd/util_test.go b/cmd/skaffold/app/cmd/util_test.go
@@ -17,6 +17,8 @@ limitations under the License.
 package cmd
 
 import (
+	"errors"
+	"fmt"
 	"testing"
 
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build"
@@ -119,3 +121,82 @@ func TestGetArtifacts(t *testing.T) {
 		})
 	}
 }
+
+func Test_getBuildArtifactsAndSetTags(t *testing.T) {
+	tests := []struct {
+		description string
+		artifacts   []build.Artifact
+		expected    []build.Artifact
+		defaultRepo string
+		shouldErr   bool
+	}{
+		{
+			description: "no artifact without default-repo",
+			artifacts:   nil,
+			expected:    []build.Artifact(nil),
+		},
+		{
+			description: "single artifact without default-repo",
+			artifacts:   []build.Artifact{{ImageName: "image", Tag: "image:tag"}},
+			expected:    []build.Artifact{{ImageName: "image", Tag: "image:tag"}},
+		},
+		{
+			description: "multiple artifacts without default-repo",
+			artifacts: []build.Artifact{
+				{ImageName: "image1", Tag: "image1:tag"},
+				{ImageName: "image1", Tag: "image1:tag"},
+			},
+			expected: []build.Artifact{
+				{ImageName: "image1", Tag: "image1:tag"},
+				{ImageName: "image1", Tag: "image1:tag"},
+			},
+		},
+		{
+			description: "single artifact with default-repo",
+			artifacts:   []build.Artifact{{ImageName: "image", Tag: "image:tag"}},
+			expected:    []build.Artifact{{ImageName: "image", Tag: "example.com/test-repo/image:tag"}},
+			defaultRepo: "example.com/test-repo",
+		},
+		{
+			description: "multiple artifacts with default-repo",
+			artifacts: []build.Artifact{
+				{ImageName: "image1", Tag: "image1:tag"},
+				{ImageName: "image1", Tag: "image1:tag"},
+			},
+			expected: []build.Artifact{
+				{ImageName: "image1", Tag: "example.com/test-repo/image1:tag"},
+				{ImageName: "image1", Tag: "example.com/test-repo/image1:tag"},
+			},
+			defaultRepo: "example.com/test-repo",
+		},
+		{
+			description: "multiple artifacts with erring default-repo",
+			artifacts: []build.Artifact{
+				{ImageName: "image1", Tag: "image1:tag"},
+				{ImageName: "image1", Tag: "image1:tag"},
+			},
+			expected:    []build.Artifact(nil),
+			defaultRepo: "example.com/test-repo",
+			shouldErr:   true,
+		},
+	}
+	for _, test := range tests {
+		testutil.Run(t, test.description, func(t *testutil.T) {
+			artifacts, err := applyDefaultRepoToArtifacts(test.artifacts, func(s string) (string, error) {
+				if test.shouldErr {
+					// this seems counter-intuitive that we explicitly return an error when shouldErr is true,
+					// however this function is a callback, the test is ensuring the error from the callback is handled
+					// correctly
+					return "", errors.New("error")
+				}
+
+				if test.defaultRepo == "" {
+					return s, nil
+				}
+
+				return fmt.Sprintf("%s/%s", test.defaultRepo, s), nil
+			})
+			t.CheckErrorAndDeepEqual(test.shouldErr, err, test.expected, artifacts)
+		})
+	}
+}