Remove PreBuiltImageBuilder and make it a function instead.

For GoogleContainerTools#922, i am making changes to `skaffold deploy` to  not run `skaffold build`.
`--images` flag is only used `deploy` flow.
The way it works is when `--images` flag is present, runner creates a
`PreBuiltImageBuilder`. All it does is, parses the images and converts
them to build.Artifact. If during deploy, an image in the list was also
built, then it uses the built image tag vs the tag present on the
commmand line flag.

In this change, we remove creating a PreBuiltImageBuilder in the runner.
(Remember this flag is only available for deploy and hence other command
won't get affected)
The `--images` will now be converted to build.Artifact in the
runner.Deploy flow.

examples/kustomize/patch.yaml

@@ -11,3 +11,5 @@ spec:
         command:
           - sleep
           - "3600"
+      - name: kustomize-test1
+        image: index.docker.io/library/nginx

integration/deploy_test.go

@@ -31,10 +31,11 @@ func TestDeploy(t *testing.T) {
 	ns, client, deleteNs := SetupNamespace(t)
 	defer deleteNs()
 
-	skaffold.Deploy("--images", "index.docker.io/library/busybox:1").InDir("examples/kustomize").InNs(ns.Name).RunOrFail(t)
+	skaffold.Deploy("--images", "index.docker.io/library/busybox:1", "--images", "index.docker.io/library/nginx:2").InDir("examples/kustomize").InNs(ns.Name).RunOrFail(t)
 
 	dep := client.GetDeployment("kustomize-test")
 	testutil.CheckDeepEqual(t, "index.docker.io/library/busybox:1", dep.Spec.Template.Spec.Containers[0].Image)
+	testutil.CheckDeepEqual(t, "index.docker.io/library/nginx:2", dep.Spec.Template.Spec.Containers[1].Image)
 
 	skaffold.Delete().InDir("examples/kustomize").InNs(ns.Name).RunOrFail(t)
 }

integration/examples/kustomize/patch.yaml

@@ -11,3 +11,5 @@ spec:
         command:
           - sleep
           - "3600"
+      - name: kustomize-test1
+        image: index.docker.io/library/nginx

integration/examples/kustomize/t.t

@@ -0,0 +1 @@
+{[]}

pkg/skaffold/runner/prebuilt.go

@@ -0,0 +1,38 @@
+/*
+Copyright 2019 The Skaffold Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package runner
+
+import (
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker"
+)
+
+func convertImagesToArtifact(images []string) ([]build.Artifact, error) {
+	var artifacts []build.Artifact
+
+	for _, tag := range images {
+		parsed, err := docker.ParseReference(tag)
+		if err != nil {
+			return nil, err
+		}
+		artifacts = append(artifacts, build.Artifact{
+			ImageName: parsed.BaseName,
+			Tag:       tag,
+		})
+	}
+	return artifacts, nil
+}

pkg/skaffold/build/prebuilt_test.go → pkg/skaffold/runner/prebuilt_test.go

@@ -14,25 +14,20 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-package build
+package runner
 
 import (
-	"context"
-	"io/ioutil"
 	"testing"
 
-	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/config"
-	runcontext "github.com/GoogleContainerTools/skaffold/pkg/skaffold/runner/context"
-	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build"
 	"github.com/GoogleContainerTools/skaffold/testutil"
 )
 
 func TestPreBuiltImagesBuilder(t *testing.T) {
 	var tests = []struct {
 		description string
 		images      []string
-		artifacts   []*latest.Artifact
-		expected    []Artifact
+		expected    []build.Artifact
 		shouldErr   bool
 	}{
 		{
@@ -41,67 +36,27 @@ func TestPreBuiltImagesBuilder(t *testing.T) {
 				"skaffold/image1:tag1",
 				"skaffold/image2:tag2",
 			},
-			artifacts: []*latest.Artifact{
-				{ImageName: "skaffold/image1"},
-				{ImageName: "skaffold/image2"},
-			},
-			expected: []Artifact{
-				{ImageName: "skaffold/image1", Tag: "skaffold/image1:tag1"},
-				{ImageName: "skaffold/image2", Tag: "skaffold/image2:tag2"},
-			},
-		},
-		{
-			description: "images in reverse order",
-			images: []string{
-				"skaffold/image2:tag2",
-				"skaffold/image1:tag1",
-			},
-			artifacts: []*latest.Artifact{
-				{ImageName: "skaffold/image1"},
-				{ImageName: "skaffold/image2"},
-			},
-			expected: []Artifact{
+			expected: []build.Artifact{
 				{ImageName: "skaffold/image1", Tag: "skaffold/image1:tag1"},
 				{ImageName: "skaffold/image2", Tag: "skaffold/image2:tag2"},
 			},
 		},
-		{
-			description: "missing image",
-			images: []string{
-				"skaffold/image1:tag1",
-			},
-			artifacts: []*latest.Artifact{
-				{ImageName: "skaffold/image1"},
-				{ImageName: "skaffold/image2"},
-			},
-			shouldErr: true,
-		},
 		{
 			// Should we support that? It is used in kustomize example.
 			description: "additional image",
 			images: []string{
 				"busybox:1",
 				"skaffold/image1:tag1",
 			},
-			artifacts: []*latest.Artifact{
-				{ImageName: "skaffold/image1"},
-			},
-			expected: []Artifact{
-				{ImageName: "skaffold/image1", Tag: "skaffold/image1:tag1"},
+			expected: []build.Artifact{
 				{ImageName: "busybox", Tag: "busybox:1"},
+				{ImageName: "skaffold/image1", Tag: "skaffold/image1:tag1"},
 			},
 		},
 	}
 	for _, test := range tests {
 		t.Run(test.description, func(t *testing.T) {
-			builder := NewPreBuiltImagesBuilder(&runcontext.RunContext{
-				Opts: &config.SkaffoldOptions{
-					PreBuiltImages: test.images,
-				},
-			})
-
-			bRes, err := builder.Build(context.Background(), ioutil.Discard, nil, test.artifacts)
-
+			bRes, err := convertImagesToArtifact(test.images)
 			testutil.CheckErrorAndDeepEqual(t, test.shouldErr, err, test.expected, bRes)
 		})
 	}

pkg/skaffold/runner/runner.go

@@ -133,10 +133,6 @@ func getBuilder(runCtx *runcontext.RunContext) (build.Builder, error) {
 		logrus.Debugln("Using builder plugins")
 		return plugin.NewPluginBuilder(runCtx)
 
-	case len(runCtx.Opts.PreBuiltImages) > 0:
-		logrus.Debugln("Using pre-built images")
-		return build.NewPreBuiltImagesBuilder(runCtx), nil
-
 	case runCtx.Cfg.Build.LocalBuild != nil:
 		logrus.Debugln("Using builder: local")
 		return local.NewBuilder(runCtx)
@@ -336,6 +332,15 @@ func (r *SkaffoldRunner) BuildAndTest(ctx context.Context, out io.Writer, artifa
 
 // Deploy deploys the given artifacts
 func (r *SkaffoldRunner) Deploy(ctx context.Context, out io.Writer, artifacts []build.Artifact) error {
+	if len(r.runCtx.Opts.PreBuiltImages) > 0 {
+		logrus.Debugln("Deploy using pre-built images")
+		pBuilts, err := convertImagesToArtifact(r.runCtx.Opts.PreBuiltImages)
+		if err != nil {
+			return errors.Wrap(err, "deploy failed")
+		}
+		// Merge current build artifacts with previously built
+		artifacts = build.MergeWithPreviousBuilds(artifacts, pBuilts)
+	}
 	err := r.Deployer.Deploy(ctx, out, artifacts, r.labellers)
 	r.hasDeployed = true
 	return err