Refactor the SkaffoldRunner

cmd/skaffold/app/cmd/build.go

@@ -23,9 +23,9 @@ import (
 	"time"
 
 	"github.com/GoogleContainerTools/skaffold/cmd/skaffold/app/flags"
-	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/color"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/config"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/runner"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
 	"github.com/pkg/errors"
 	"github.com/spf13/cobra"
@@ -37,11 +37,6 @@ var (
 	buildFormatFlag = flags.NewTemplateFlag("{{json .}}", flags.BuildOutput{})
 )
 
-// For testing
-var (
-	createRunnerAndBuildFunc = createRunnerAndBuild
-)
-
 // NewCmdBuild describes the CLI command to build artifacts.
 func NewCmdBuild(out io.Writer) *cobra.Command {
 	return NewCmd(out, "build").
@@ -66,29 +61,18 @@ func doBuild(ctx context.Context, out io.Writer) error {
 		color.Default.Fprintln(buildOut, "Complete in", time.Since(start))
 	}()
 
-	bRes, err := createRunnerAndBuildFunc(ctx, buildOut)
-	if err != nil {
-		return alwaysSucceedWhenCancelled(ctx, err)
-	}
+	return withRunner(ctx, func(r runner.Runner, config *latest.SkaffoldConfig) error {
+		bRes, err := r.BuildAndTest(ctx, buildOut, targetArtifacts(opts, config))
 
-	if quietFlag {
-		cmdOut := flags.BuildOutput{Builds: bRes}
-		if err := buildFormatFlag.Template().Execute(out, cmdOut); err != nil {
-			return errors.Wrap(err, "executing template")
+		if quietFlag {
+			cmdOut := flags.BuildOutput{Builds: bRes}
+			if err := buildFormatFlag.Template().Execute(out, cmdOut); err != nil {
+				return errors.Wrap(err, "executing template")
+			}
 		}
-	}
-
-	return nil
-}
-
-func createRunnerAndBuild(ctx context.Context, buildOut io.Writer) ([]build.Artifact, error) {
-	runner, config, err := newRunner(opts)
-	if err != nil {
-		return nil, errors.Wrap(err, "creating runner")
-	}
-	defer runner.RPCServerShutdown()
 
-	return runner.BuildAndTest(ctx, buildOut, targetArtifacts(opts, config))
+		return err
+	})
 }
 
 func targetArtifacts(opts *config.SkaffoldOptions, cfg *latest.SkaffoldConfig) []*latest.Artifact {

cmd/skaffold/app/cmd/build_test.go

@@ -26,49 +26,60 @@ import (
 
 	"github.com/GoogleContainerTools/skaffold/cmd/skaffold/app/flags"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/config"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/runner"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
 	"github.com/GoogleContainerTools/skaffold/testutil"
 )
 
+type mockRunner struct {
+	runner.Runner
+}
+
+func (r *mockRunner) BuildAndTest(ctx context.Context, out io.Writer, artifacts []*latest.Artifact) ([]build.Artifact, error) {
+	return []build.Artifact{{
+		ImageName: "gcr.io/skaffold/example",
+		Tag:       "test",
+	}}, nil
+}
+
+func (r *mockRunner) Stop() error {
+	return nil
+}
+
 func TestQuietFlag(t *testing.T) {
-	mockCreateRunner := func(context.Context, io.Writer) ([]build.Artifact, error) {
-		return []build.Artifact{{
-			ImageName: "gcr.io/skaffold/example",
-			Tag:       "test",
-		}}, nil
+	mockCreateRunner := func(opts *config.SkaffoldOptions) (runner.Runner, *latest.SkaffoldConfig, error) {
+		return &mockRunner{}, &latest.SkaffoldConfig{}, nil
 	}
 
 	var tests = []struct {
 		description    string
 		template       string
 		expectedOutput []byte
-		mock           func(context.Context, io.Writer) ([]build.Artifact, error)
 		shouldErr      bool
 	}{
 		{
 			description:    "quiet flag print build images with no template",
 			expectedOutput: []byte(`{"builds":[{"imageName":"gcr.io/skaffold/example","tag":"test"}]}`),
 			shouldErr:      false,
-			mock:           mockCreateRunner,
 		},
 		{
 			description:    "quiet flag print build images applies pattern specified in template ",
 			template:       "{{range .Builds}}{{.ImageName}} -> {{.Tag}}\n{{end}}",
 			expectedOutput: []byte("gcr.io/skaffold/example -> test\n"),
 			shouldErr:      false,
-			mock:           mockCreateRunner,
 		},
 		{
 			description:    "build errors out when incorrect template specified",
 			template:       "{{.Incorrect}}",
 			expectedOutput: nil,
 			shouldErr:      true,
-			mock:           mockCreateRunner,
 		},
 	}
 	for _, test := range tests {
 		testutil.Run(t, test.description, func(t *testutil.T) {
 			t.Override(&quietFlag, true)
-			t.Override(&createRunnerAndBuildFunc, test.mock)
+			t.Override(&createRunner, mockCreateRunner)
 			if test.template != "" {
 				t.Override(&buildFormatFlag, flags.NewTemplateFlag(test.template, flags.BuildOutput{}))
 			}
@@ -83,19 +94,16 @@ func TestQuietFlag(t *testing.T) {
 }
 
 func TestRunBuild(t *testing.T) {
-	errRunner := func(context.Context, io.Writer) ([]build.Artifact, error) {
-		return nil, errors.New("some error")
+	errRunner := func(opts *config.SkaffoldOptions) (runner.Runner, *latest.SkaffoldConfig, error) {
+		return nil, nil, errors.New("some error")
 	}
-	mockCreateRunner := func(context.Context, io.Writer) ([]build.Artifact, error) {
-		return []build.Artifact{{
-			ImageName: "gcr.io/skaffold/example",
-			Tag:       "test",
-		}}, nil
+	mockCreateRunner := func(opts *config.SkaffoldOptions) (runner.Runner, *latest.SkaffoldConfig, error) {
+		return &mockRunner{}, &latest.SkaffoldConfig{}, nil
 	}
 
 	var tests = []struct {
 		description string
-		mock        func(context.Context, io.Writer) ([]build.Artifact, error)
+		mock        func(opts *config.SkaffoldOptions) (runner.Runner, *latest.SkaffoldConfig, error)
 		shouldErr   bool
 	}{
 		{
@@ -111,7 +119,7 @@ func TestRunBuild(t *testing.T) {
 	}
 	for _, test := range tests {
 		testutil.Run(t, test.description, func(t *testutil.T) {
-			t.Override(&createRunnerAndBuildFunc, test.mock)
+			t.Override(&createRunner, test.mock)
 
 			err := doBuild(context.Background(), ioutil.Discard)
 

cmd/skaffold/app/cmd/delete.go

@@ -34,7 +34,7 @@ func NewCmdDelete(out io.Writer) *cobra.Command {
 }
 
 func doDelete(ctx context.Context, out io.Writer) error {
-	return withRunner(ctx, func(r *runner.SkaffoldRunner, _ *latest.SkaffoldConfig) error {
-		return r.Deployer.Cleanup(ctx, out)
+	return withRunner(ctx, func(r runner.Runner, _ *latest.SkaffoldConfig) error {
+		return r.Cleanup(ctx, out)
 	})
 }

cmd/skaffold/app/cmd/deploy.go

@@ -47,11 +47,11 @@ E.g. build.out created by running skaffold build --quiet {{json .}} > build.out`
 }
 
 func doDeploy(ctx context.Context, out io.Writer) error {
-	return withRunner(ctx, func(r *runner.SkaffoldRunner, _ *latest.SkaffoldConfig) error {
+	return withRunner(ctx, func(r runner.Runner, _ *latest.SkaffoldConfig) error {
 		// If the BuildArtifacts contains an image in the preBuilt list,
 		// use image from BuildArtifacts instead
 		deployArtifacts := build.MergeWithPreviousBuilds(buildOutputFile.BuildArtifacts(), preBuiltImages.Artifacts())
 
-		return r.Deploy(ctx, out, deployArtifacts)
+		return r.DeployAndLog(ctx, out, deployArtifacts)
 	})
 }

cmd/skaffold/app/cmd/dev.go

@@ -21,7 +21,7 @@ import (
 	"io"
 
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/runner"
-	"github.com/pkg/errors"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
 	"github.com/sirupsen/logrus"
 	"github.com/spf13/cobra"
 	"github.com/spf13/pflag"
@@ -63,30 +63,29 @@ func doDev(ctx context.Context, out io.Writer) error {
 		case <-ctx.Done():
 			return nil
 		default:
-			r, config, err := newRunner(opts)
-			if err != nil {
-				return errors.Wrap(err, "creating runner")
-			}
+			err := withRunner(ctx, func(r runner.Runner, config *latest.SkaffoldConfig) error {
+				err := r.Dev(ctx, out, config.Build.Artifacts)
 
-			err = r.Dev(ctx, out, config.Build.Artifacts)
-			if r.HasDeployed() {
-				cleanup = func() {
-					if err := r.Cleanup(context.Background(), out); err != nil {
-						logrus.Warnln("deployer cleanup:", err)
+				if r.HasDeployed() {
+					cleanup = func() {
+						if err := r.Cleanup(context.Background(), out); err != nil {
+							logrus.Warnln("deployer cleanup:", err)
+						}
 					}
 				}
-			}
-			if r.HasBuilt() {
-				prune = func() {
-					if err := r.Prune(context.Background(), out); err != nil {
-						logrus.Warnln("builder cleanup:", err)
+
+				if r.HasBuilt() {
+					prune = func() {
+						if err := r.Prune(context.Background(), out); err != nil {
+							logrus.Warnln("builder cleanup:", err)
+						}
 					}
 				}
-			}
 
-			if err != nil && errors.Cause(err) != runner.ErrorConfigurationChanged {
-				r.RPCServerShutdown()
-				return alwaysSucceedWhenCancelled(ctx, err)
+				return err
+			})
+			if err != nil {
+				return err
 			}
 		}
 	}

cmd/skaffold/app/cmd/diagnose.go

@@ -44,7 +44,7 @@ func NewCmdDiagnose(out io.Writer) *cobra.Command {
 }
 
 func doDiagnose(ctx context.Context, out io.Writer) error {
-	return withRunner(ctx, func(r *runner.SkaffoldRunner, config *latest.SkaffoldConfig) error {
+	return withRunner(ctx, func(r runner.Runner, config *latest.SkaffoldConfig) error {
 		fmt.Fprintln(out, "Skaffold version:", version.Get().GitCommit)
 		fmt.Fprintln(out, "Configuration version:", config.APIVersion)
 		fmt.Fprintln(out, "Number of artifacts:", len(config.Build.Artifacts))

cmd/skaffold/app/cmd/run.go

@@ -23,6 +23,7 @@ import (
 	"github.com/GoogleContainerTools/skaffold/cmd/skaffold/app/tips"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/runner"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
+	"github.com/pkg/errors"
 	"github.com/spf13/cobra"
 	"github.com/spf13/pflag"
 )
@@ -39,8 +40,13 @@ func NewCmdRun(out io.Writer) *cobra.Command {
 }
 
 func doRun(ctx context.Context, out io.Writer) error {
-	return withRunner(ctx, func(r *runner.SkaffoldRunner, config *latest.SkaffoldConfig) error {
-		err := r.Run(ctx, out, config.Build.Artifacts)
+	return withRunner(ctx, func(r runner.Runner, config *latest.SkaffoldConfig) error {
+		bRes, err := r.BuildAndTest(ctx, out, config.Build.Artifacts)
+		if err != nil {
+			return errors.Wrap(err, "failed to build")
+		}
+
+		err = r.DeployAndLog(ctx, out, bRes)
 		if err == nil {
 			tips.PrintForRun(out, opts)
 		}

cmd/skaffold/app/cmd/runner.go

@@ -34,19 +34,22 @@ import (
 	"github.com/sirupsen/logrus"
 )
 
-func withRunner(ctx context.Context, action func(*runner.SkaffoldRunner, *latest.SkaffoldConfig) error) error {
-	runner, config, err := newRunner(opts)
+// For tests
+var createRunner = createNewRunner
+
+func withRunner(ctx context.Context, action func(runner.Runner, *latest.SkaffoldConfig) error) error {
+	runner, config, err := createRunner(opts)
 	if err != nil {
 		return errors.Wrap(err, "creating runner")
 	}
-	defer runner.RPCServerShutdown()
+	defer runner.Stop()
 
 	err = action(runner, config)
 	return alwaysSucceedWhenCancelled(ctx, err)
 }
 
-// newRunner creates a SkaffoldRunner and returns the SkaffoldConfig associated with it.
-func newRunner(opts *config.SkaffoldOptions) (*runner.SkaffoldRunner, *latest.SkaffoldConfig, error) {
+// createNewRunner creates a Runner and returns the SkaffoldConfig associated with it.
+func createNewRunner(opts *config.SkaffoldOptions) (runner.Runner, *latest.SkaffoldConfig, error) {
 	parsed, err := schema.ParseConfig(opts.ConfigurationFile, true)
 	if err != nil {
 		// If the error is NOT that the file doesn't exist, then we warn the user

cmd/skaffold/app/cmd/runner_test.go

@@ -25,7 +25,7 @@ import (
 	"github.com/GoogleContainerTools/skaffold/testutil"
 )
 
-func TestNewRunner(t *testing.T) {
+func TestCreateNewRunner(t *testing.T) {
 	tests := []struct {
 		description   string
 		config        string
@@ -85,7 +85,7 @@ func TestNewRunner(t *testing.T) {
 				Write("skaffold.yaml", fmt.Sprintf("apiVersion: %s\nkind: Config\n%s", latest.Version, test.config)).
 				Chdir()
 
-			_, _, err := newRunner(test.options)
+			_, _, err := createNewRunner(test.options)
 
 			t.CheckError(test.shouldErr, err)
 			if test.expectedError != "" {

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

@@ -26,7 +26,7 @@ import (
 	"github.com/sirupsen/logrus"
 )
 
-// BuildAndTest builds artifacts and runs tests on built artifacts
+// BuildAndTest builds and tests a list of artifacts.
 func (r *SkaffoldRunner) BuildAndTest(ctx context.Context, out io.Writer, artifacts []*latest.Artifact) ([]build.Artifact, error) {
 	tags, err := r.imageTags(ctx, out, artifacts)
 	if err != nil {
@@ -39,7 +39,7 @@ func (r *SkaffoldRunner) BuildAndTest(ctx context.Context, out io.Writer, artifa
 		return nil, errors.Wrap(err, "retrieving cached artifacts")
 	}
 
-	bRes, err := r.Build(ctx, out, tags, artifactsToBuild)
+	bRes, err := r.Builder.Build(ctx, out, tags, artifactsToBuild)
 	if err != nil {
 		return nil, errors.Wrap(err, "build failed")
 	}
@@ -48,11 +48,46 @@ func (r *SkaffoldRunner) BuildAndTest(ctx context.Context, out io.Writer, artifa
 	if err := r.cache.CacheArtifacts(ctx, artifacts, bRes); err != nil {
 		logrus.Warnf("error caching artifacts: %v", err)
 	}
+
 	if !r.runCtx.Opts.SkipTests {
-		if err = r.Test(ctx, out, bRes); err != nil {
+		if err = r.Tester.Test(ctx, out, bRes); err != nil {
 			return nil, errors.Wrap(err, "test failed")
 		}
 	}
 
-	return bRes, err
+	// Update which images are logged.
+	for _, build := range bRes {
+		r.imageList.Add(build.Tag)
+	}
+
+	// Make sure all artifacts are redeployed. Not only those that were just built.
+	r.builds = build.MergeWithPreviousBuilds(bRes, r.builds)
+	return bRes, nil
+}
+
+// DeployAndLog deploys a list of already built artifacts and optionally show the logs.
+func (r *SkaffoldRunner) DeployAndLog(ctx context.Context, out io.Writer, artifacts []build.Artifact) error {
+	if !r.runCtx.Opts.Tail {
+		return r.Deploy(ctx, out, artifacts)
+	}
+
+	var imageNames []string
+	for _, artifact := range artifacts {
+		imageNames = append(imageNames, artifact.ImageName)
+	}
+
+	logger := r.newLoggerForImages(out, imageNames)
+	defer logger.Stop()
+
+	if err := logger.Start(ctx); err != nil {
+		return errors.Wrap(err, "starting logger")
+	}
+
+	if err := r.Deploy(ctx, out, artifacts); err != nil {
+		return err
+	}
+
+	<-ctx.Done()
+
+	return nil
 }