More feedback from review

cmd/skaffold/app/cmd/cmd.go

@@ -65,19 +65,19 @@ func SetUpLogs(out io.Writer, level string) error {
 	return nil
 }
 
-func runSkaffold(out io.Writer, defaultConfig *config.SkaffoldConfig, filename string) error {
+func runSkaffold(out io.Writer, dev bool, filename string) error {
 	f, err := os.Open(filename)
 	if err != nil {
 		return errors.Wrap(err, "opening skaffold config")
 	}
 	defer f.Close()
 
-	cfg, err := config.Parse(defaultConfig, f)
+	cfg, err := config.Parse(f)
 	if err != nil {
 		return errors.Wrap(err, "parsing skaffold config")
 	}
 
-	r, err := runner.NewForConfig(out, cfg)
+	r, err := runner.NewForConfig(out, dev, cfg)
 	if err != nil {
 		return errors.Wrap(err, "getting skaffold config")
 	}

cmd/skaffold/app/cmd/dev.go

@@ -19,8 +19,6 @@ package cmd
 import (
 	"io"
 
-	"github.com/GoogleCloudPlatform/skaffold/pkg/skaffold/config"
-
 	"github.com/sirupsen/logrus"
 	"github.com/spf13/cobra"
 )
@@ -30,7 +28,7 @@ func NewCmdDev(out io.Writer) *cobra.Command {
 		Use:   "dev",
 		Short: "runs a pipeline file in development mode",
 		Run: func(cmd *cobra.Command, args []string) {
-			if err := runSkaffold(out, &config.DevelopmentConfig, filename); err != nil {
+			if err := runSkaffold(out, true, filename); err != nil {
 				logrus.Errorf("run: %s", err)
 			}
 		},

cmd/skaffold/app/cmd/run.go

@@ -28,7 +28,7 @@ func NewCmdRun(out io.Writer) *cobra.Command {
 		Use:   "run",
 		Short: "runs a pipeline file",
 		Run: func(cmd *cobra.Command, args []string) {
-			if err := runSkaffold(out, nil, filename); err != nil {
+			if err := runSkaffold(out, false, filename); err != nil {
 				logrus.Errorf("run: %s", err)
 			}
 		},

pkg/skaffold/config/config.go

@@ -33,8 +33,6 @@ type SkaffoldConfig struct {
 	APIVersion string `yaml:"apiVersion"`
 	Kind       string `yaml:"kind"`
 
-	Watch bool `yaml:"watch"`
-
 	Build  BuildConfig  `yaml:"build"`
 	Deploy DeployConfig `yaml:"deploy"`
 }
@@ -77,24 +75,16 @@ type Artifact struct {
 	Workspace      string `yaml:"workspace"`
 }
 
-// DevelopmentConfig is the default development configuration for skaffold dev mode
-var DevelopmentConfig = SkaffoldConfig{
-	Watch: true,
-}
-
 // Parse reads from an io.Reader and unmarshals the result into a SkaffoldConfig.
 // The default config argument provides default values for the config,
 // which can be overridden if present in the config file.
-func Parse(defaultConfig *SkaffoldConfig, config io.Reader) (*SkaffoldConfig, error) {
+func Parse(config io.Reader) (*SkaffoldConfig, error) {
 	var b bytes.Buffer
 	if _, err := b.ReadFrom(config); err != nil {
 		return nil, errors.Wrap(err, "reading config")
 	}
 
 	var cfg SkaffoldConfig
-	if defaultConfig != nil {
-		cfg = *defaultConfig
-	}
 	if err := yaml.Unmarshal(b.Bytes(), &cfg); err != nil {
 		return nil, err
 	}

pkg/skaffold/config/config_test.go

@@ -59,34 +59,13 @@ var configA = &SkaffoldConfig{
 	},
 }
 
-var configB = &SkaffoldConfig{
-	APIVersion: "skaffold/v1",
-	Kind:       "Config",
-	Watch:      true,
-	Build: BuildConfig{
-		Artifacts: []*Artifact{
-			{
-				ImageName: "example",
-				Workspace: "./examples/app",
-			},
-		},
-	},
-	Deploy: DeployConfig{
-		Name: "example",
-		Parameters: map[string]string{
-			"key": "value",
-		},
-	},
-}
-
 func TestParseConfig(t *testing.T) {
 	var tests = []struct {
-		description   string
-		config        string
-		defaultConfig *SkaffoldConfig
-		expected      *SkaffoldConfig
-		badReader     bool
-		shouldErr     bool
+		description string
+		config      string
+		expected    *SkaffoldConfig
+		badReader   bool
+		shouldErr   bool
 	}{
 		{
 			description: "Parse config",
@@ -98,12 +77,6 @@ func TestParseConfig(t *testing.T) {
 			config:      badConfigA,
 			shouldErr:   true,
 		},
-		{
-			description:   "default config",
-			defaultConfig: &SkaffoldConfig{Watch: true},
-			config:        rawConfigA,
-			expected:      configB,
-		},
 		{
 			description: "bad reader",
 			badReader:   true,
@@ -118,7 +91,7 @@ func TestParseConfig(t *testing.T) {
 			if test.badReader {
 				r = testutil.BadReader{}
 			}
-			cfg, err := Parse(test.defaultConfig, r)
+			cfg, err := Parse(r)
 			testutil.CheckErrorAndDeepEqual(t, test.shouldErr, err, test.expected, cfg)
 		})
 	}

pkg/skaffold/runner/runner.go

@@ -26,7 +26,6 @@ import (
 	"github.com/GoogleCloudPlatform/skaffold/pkg/skaffold/constants"
 	"github.com/GoogleCloudPlatform/skaffold/pkg/skaffold/watch"
 	"github.com/pkg/errors"
-	"github.com/sirupsen/logrus"
 )
 
 // SkaffoldRunner is responsible for running the skaffold build and deploy pipeline.
@@ -35,14 +34,17 @@ type SkaffoldRunner struct {
 	tag.Tagger
 	watch.Watcher
 
-	config *config.SkaffoldConfig
-	cancel chan struct{}
+	devMode bool
+
+	config     *config.SkaffoldConfig
+	watchReady chan *watch.WatchEvent
+	cancel     chan struct{}
 
 	out io.Writer
 }
 
 // NewForConfig returns a new SkaffoldRunner for a SkaffoldConfig
-func NewForConfig(out io.Writer, cfg *config.SkaffoldConfig) (*SkaffoldRunner, error) {
+func NewForConfig(out io.Writer, dev bool, cfg *config.SkaffoldConfig) (*SkaffoldRunner, error) {
 	builder, err := getBuilder(&cfg.Build)
 	if err != nil {
 		return nil, errors.Wrap(err, "parsing skaffold build config")
@@ -56,6 +58,7 @@ func NewForConfig(out io.Writer, cfg *config.SkaffoldConfig) (*SkaffoldRunner, e
 		Builder: builder,
 		Tagger:  tagger,
 		Watcher: &watch.FSWatcher{}, //TODO(@r2d4): should this be configurable?
+		devMode: dev,
 		cancel:  make(chan struct{}, 1),
 		out:     out,
 	}, nil
@@ -68,10 +71,6 @@ func getBuilder(cfg *config.BuildConfig) (build.Builder, error) {
 	return nil, fmt.Errorf("Unknown builder for config %+v", cfg)
 }
 
-func getWatcher(cfg *config.SkaffoldConfig) (watch.Watcher, error) {
-	return &watch.FSWatcher{}, nil
-}
-
 func newTaggerForConfig(tagStrategy string) (tag.Tagger, error) {
 	switch tagStrategy {
 	case constants.TagStrategySha256:
@@ -83,28 +82,29 @@ func newTaggerForConfig(tagStrategy string) (tag.Tagger, error) {
 
 // Run runs the skaffold build and deploy pipeline.
 func (r *SkaffoldRunner) Run() error {
-	for {
-		if r.config.Watch {
-			_, err := r.Watch(r.config.Build.Artifacts, nil, r.cancel)
-			if err != nil {
-				return errors.Wrap(err, "watch step")
-			}
-		}
+	if r.devMode {
+		return r.dev()
+	}
+	return r.run()
+}
 
-		_, err := r.Builder.Run(r.out, r.Tagger)
+func (r *SkaffoldRunner) dev() error {
+	for {
+		evt, err := r.Watch(r.config.Build.Artifacts, r.watchReady, r.cancel)
 		if err != nil {
-			if r.config.Watch {
-				logrus.Warn("Build step error: %s", err)
-			} else {
-				return errors.Wrap(err, "build step")
-			}
+			return errors.Wrap(err, "running watch")
 		}
-
-		// Deploy
-		if !r.config.Watch {
-			break
+		if evt.EventType == watch.WatchStop {
+			return nil
+		}
+		if err := r.run(); err != nil {
+			return errors.Wrap(err, "running build and deploy")
 		}
 	}
+}
 
-	return nil
+func (r *SkaffoldRunner) run() error {
+	_, err := r.Builder.Run(r.out, r.Tagger)
+	// Deploy
+	return err
 }

pkg/skaffold/runner/runner_test.go

@@ -27,7 +27,9 @@ import (
 	"github.com/GoogleCloudPlatform/skaffold/pkg/skaffold/build/tag"
 	"github.com/GoogleCloudPlatform/skaffold/pkg/skaffold/config"
 	"github.com/GoogleCloudPlatform/skaffold/pkg/skaffold/constants"
+	"github.com/GoogleCloudPlatform/skaffold/pkg/skaffold/watch"
 	testutil "github.com/GoogleCloudPlatform/skaffold/test"
+	"github.com/sirupsen/logrus"
 )
 
 type TestBuilder struct {
@@ -39,11 +41,32 @@ func (t *TestBuilder) Run(io.Writer, tag.Tagger) (*build.BuildResult, error) {
 	return t.res, t.err
 }
 
+type TestWatcher struct {
+	res []*watch.WatchEvent
+	err error
+
+	current int
+}
+
+func NewTestWatch(err error, res ...*watch.WatchEvent) *TestWatcher {
+	return &TestWatcher{res: res, err: err}
+}
+
+func (t *TestWatcher) Watch(artifacts []*config.Artifact, ready chan *watch.WatchEvent, cancel chan struct{}) (*watch.WatchEvent, error) {
+	if t.current > len(t.res)-1 {
+		logrus.Fatalf("Called watch too many times. WatchEvents %d, Current: %d", len(t.res)-1, t.current)
+	}
+	ret := t.res[t.current]
+	t.current = t.current + 1
+	return ret, t.err
+}
+
 func TestNewForConfig(t *testing.T) {
 	var tests = []struct {
 		description string
 		config      *config.SkaffoldConfig
 		shouldErr   bool
+		sendCancel  bool
 		expected    interface{}
 	}{
 		{
@@ -81,7 +104,7 @@ func TestNewForConfig(t *testing.T) {
 	}
 	for _, test := range tests {
 		t.Run(test.description, func(t *testing.T) {
-			cfg, err := NewForConfig(&bytes.Buffer{}, test.config)
+			cfg, err := NewForConfig(&bytes.Buffer{}, false, test.config)
 			testutil.CheckError(t, test.shouldErr, err)
 			if cfg != nil {
 				testutil.CheckErrorAndTypeEquality(t, test.shouldErr, err, test.expected, cfg.Builder)
@@ -94,6 +117,7 @@ func TestRun(t *testing.T) {
 	var tests = []struct {
 		description string
 		runner      *SkaffoldRunner
+		devmode     bool
 		shouldErr   bool
 	}{
 		{
@@ -104,7 +128,8 @@ func TestRun(t *testing.T) {
 					res: &build.BuildResult{},
 					err: nil,
 				},
-				Tagger: &tag.ChecksumTagger{},
+				devMode: false,
+				Tagger:  &tag.ChecksumTagger{},
 			},
 		},
 		{
@@ -118,6 +143,52 @@ func TestRun(t *testing.T) {
 			},
 			shouldErr: true,
 		},
+		{
+			description: "run dev mode",
+			runner: &SkaffoldRunner{
+				config: &config.SkaffoldConfig{},
+				Builder: &TestBuilder{
+					res: &build.BuildResult{},
+					err: nil,
+				},
+				Watcher:    NewTestWatch(nil, &watch.WatchEvent{}, watch.WatchStopEvent),
+				devMode:    true,
+				cancel:     make(chan struct{}, 1),
+				watchReady: make(chan *watch.WatchEvent, 1),
+				Tagger:     &tag.ChecksumTagger{},
+			},
+		},
+		{
+			description: "run dev mode build error",
+			runner: &SkaffoldRunner{
+				config: &config.SkaffoldConfig{},
+				Builder: &TestBuilder{
+					err: fmt.Errorf(""),
+				},
+				Watcher:    NewTestWatch(nil, &watch.WatchEvent{}, watch.WatchStopEvent),
+				devMode:    true,
+				cancel:     make(chan struct{}, 1),
+				watchReady: make(chan *watch.WatchEvent, 1),
+				Tagger:     &tag.ChecksumTagger{},
+			},
+			shouldErr: true,
+		},
+		{
+			description: "bad watch dev mode",
+			runner: &SkaffoldRunner{
+				config: &config.SkaffoldConfig{},
+				Builder: &TestBuilder{
+					res: &build.BuildResult{},
+					err: nil,
+				},
+				Watcher:    NewTestWatch(fmt.Errorf(""), nil),
+				devMode:    true,
+				cancel:     make(chan struct{}, 1),
+				watchReady: make(chan *watch.WatchEvent, 1),
+				Tagger:     &tag.ChecksumTagger{},
+			},
+			shouldErr: true,
+		},
 	}
 
 	for _, test := range tests {

pkg/skaffold/watch/watch.go

@@ -52,6 +52,11 @@ const (
 	WatchStop  = "WatchStop"
 )
 
+var (
+	WatchStopEvent  = &WatchEvent{EventType: WatchStop}
+	WatchStartEvent = &WatchEvent{EventType: WatchReady}
+)
+
 //TODO(@r2d4): Figure out best UX to support configuring this blacklist
 var ignoredPrefixes = []string{"vendor", ".git"}
 
@@ -68,7 +73,7 @@ func (f *FSWatcher) Watch(artifacts []*config.Artifact, ready chan *WatchEvent,
 		}
 	}
 	if ready != nil {
-		ready <- &WatchEvent{EventType: WatchReady}
+		ready <- WatchStartEvent
 	}
 	for {
 		select {
@@ -81,7 +86,7 @@ func (f *FSWatcher) Watch(artifacts []*config.Artifact, ready chan *WatchEvent,
 			}, nil
 		case <-cancel:
 			logrus.Info("Watch canceled")
-			return &WatchEvent{EventType: WatchStop}, nil
+			return WatchStopEvent, nil
 		}
 	}
 }