From 05a57d09c8cd1793b1670c8e8f3b0206cddadb4b Mon Sep 17 00:00:00 2001 From: David Gageot Date: Thu, 7 Jun 2018 09:04:54 +0200 Subject: [PATCH 1/2] Extract duplicated code to create runner Signed-off-by: David Gageot --- cmd/skaffold/app/cmd/build.go | 8 +------ cmd/skaffold/app/cmd/delete.go | 8 +------ cmd/skaffold/app/cmd/deploy.go | 8 +------ cmd/skaffold/app/cmd/dev.go | 8 +------ cmd/skaffold/app/cmd/run.go | 8 +------ cmd/skaffold/app/cmd/runner.go | 38 ++++++++++++++++++++++++++++++++++ 6 files changed, 43 insertions(+), 35 deletions(-) create mode 100644 cmd/skaffold/app/cmd/runner.go diff --git a/cmd/skaffold/app/cmd/build.go b/cmd/skaffold/app/cmd/build.go index e54a749367a..67981274691 100644 --- a/cmd/skaffold/app/cmd/build.go +++ b/cmd/skaffold/app/cmd/build.go @@ -23,7 +23,6 @@ import ( "github.com/GoogleContainerTools/skaffold/cmd/skaffold/app/flags" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/runner" "github.com/pkg/errors" "github.com/spf13/cobra" ) @@ -57,12 +56,7 @@ type BuildOutput struct { func runBuild(out io.Writer, filename string) error { ctx := context.Background() - config, err := readConfiguration(filename) - if err != nil { - return errors.Wrap(err, "reading configuration") - } - - runner, err := runner.NewForConfig(opts, config) + runner, config, err := newRunner(filename) if err != nil { return errors.Wrap(err, "creating runner") } diff --git a/cmd/skaffold/app/cmd/delete.go b/cmd/skaffold/app/cmd/delete.go index f9b25b41f1d..81a4a385ed5 100644 --- a/cmd/skaffold/app/cmd/delete.go +++ b/cmd/skaffold/app/cmd/delete.go @@ -20,7 +20,6 @@ import ( "context" "io" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/runner" "github.com/pkg/errors" "github.com/spf13/cobra" ) @@ -42,12 +41,7 @@ func NewCmdDelete(out io.Writer) *cobra.Command { func delete(out io.Writer, filename string) error { ctx := context.Background() - config, err := readConfiguration(filename) - if err != nil { - return errors.Wrap(err, "reading configuration") - } - - runner, err := runner.NewForConfig(opts, config) + runner, _, err := newRunner(filename) if err != nil { return errors.Wrap(err, "creating runner") } diff --git a/cmd/skaffold/app/cmd/deploy.go b/cmd/skaffold/app/cmd/deploy.go index ede7dea9993..32d1536b30d 100644 --- a/cmd/skaffold/app/cmd/deploy.go +++ b/cmd/skaffold/app/cmd/deploy.go @@ -23,7 +23,6 @@ import ( "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/runner" "github.com/pkg/errors" "github.com/spf13/cobra" ) @@ -51,12 +50,7 @@ func NewCmdDeploy(out io.Writer) *cobra.Command { func runDeploy(out io.Writer, filename string) error { ctx := context.Background() - config, err := readConfiguration(filename) - if err != nil { - return errors.Wrap(err, "reading configuration") - } - - runner, err := runner.NewForConfig(opts, config) + runner, _, err := newRunner(filename) if err != nil { return errors.Wrap(err, "creating runner") } diff --git a/cmd/skaffold/app/cmd/dev.go b/cmd/skaffold/app/cmd/dev.go index cc720a4f33e..87ee2bd1865 100644 --- a/cmd/skaffold/app/cmd/dev.go +++ b/cmd/skaffold/app/cmd/dev.go @@ -20,7 +20,6 @@ import ( "context" "io" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/runner" "github.com/pkg/errors" "github.com/spf13/cobra" ) @@ -43,12 +42,7 @@ func NewCmdDev(out io.Writer) *cobra.Command { func dev(out io.Writer, filename string) error { ctx := context.Background() - config, err := readConfiguration(filename) - if err != nil { - return errors.Wrap(err, "reading configuration") - } - - runner, err := runner.NewForConfig(opts, config) + runner, config, err := newRunner(filename) if err != nil { return errors.Wrap(err, "creating runner") } diff --git a/cmd/skaffold/app/cmd/run.go b/cmd/skaffold/app/cmd/run.go index ab09cc0fc68..6f03a45185e 100644 --- a/cmd/skaffold/app/cmd/run.go +++ b/cmd/skaffold/app/cmd/run.go @@ -20,7 +20,6 @@ import ( "context" "io" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/runner" "github.com/pkg/errors" "github.com/spf13/cobra" ) @@ -44,12 +43,7 @@ func NewCmdRun(out io.Writer) *cobra.Command { func run(out io.Writer, filename string) error { ctx := context.Background() - config, err := readConfiguration(filename) - if err != nil { - return errors.Wrap(err, "reading configuration") - } - - runner, err := runner.NewForConfig(opts, config) + runner, config, err := newRunner(filename) if err != nil { return errors.Wrap(err, "creating runner") } diff --git a/cmd/skaffold/app/cmd/runner.go b/cmd/skaffold/app/cmd/runner.go new file mode 100644 index 00000000000..72fd4b75b95 --- /dev/null +++ b/cmd/skaffold/app/cmd/runner.go @@ -0,0 +1,38 @@ +/* +Copyright 2018 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 cmd + +import ( + "github.com/GoogleContainerTools/skaffold/pkg/skaffold/config" + "github.com/GoogleContainerTools/skaffold/pkg/skaffold/runner" + "github.com/pkg/errors" +) + +// newRunner creates a SkaffoldRunner and returns the SkaffoldConfig associated with it. +func newRunner(filename string) (*runner.SkaffoldRunner, *config.SkaffoldConfig, error) { + config, err := readConfiguration(filename) + if err != nil { + return nil, nil, errors.Wrap(err, "reading configuration") + } + + runner, err := runner.NewForConfig(opts, config) + if err != nil { + return nil, nil, errors.Wrap(err, "creating runner") + } + + return runner, config, nil +} From 789625f4048c1891a6ae906a93d9521f0acbad26 Mon Sep 17 00:00:00 2001 From: David Gageot Date: Thu, 7 Jun 2018 09:14:05 +0200 Subject: [PATCH 2/2] Extract signal handling code from runner Signed-off-by: David Gageot --- cmd/skaffold/app/cmd/dev.go | 36 +++++++++++++++++++-- pkg/skaffold/runner/runner.go | 52 +++++------------------------- pkg/skaffold/runner/runner_test.go | 9 ++---- 3 files changed, 45 insertions(+), 52 deletions(-) diff --git a/cmd/skaffold/app/cmd/dev.go b/cmd/skaffold/app/cmd/dev.go index 87ee2bd1865..99cdbd2906a 100644 --- a/cmd/skaffold/app/cmd/dev.go +++ b/cmd/skaffold/app/cmd/dev.go @@ -19,8 +19,12 @@ package cmd import ( "context" "io" + "os" + "os/signal" + "syscall" "github.com/pkg/errors" + "github.com/sirupsen/logrus" "github.com/spf13/cobra" ) @@ -40,12 +44,40 @@ func NewCmdDev(out io.Writer) *cobra.Command { } func dev(out io.Writer, filename string) error { - ctx := context.Background() + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + if opts.Cleanup { + catchCtrlC(cancel) + } runner, config, err := newRunner(filename) if err != nil { return errors.Wrap(err, "creating runner") } - return runner.Dev(ctx, out, config.Build.Artifacts) + built, err := runner.Dev(ctx, out, config.Build.Artifacts) + + if opts.Cleanup && built != nil { + // Cleanup only if something was built + if err := runner.Cleanup(ctx, out); err != nil { + logrus.Warnln("cleanup:", err) + } + } + + return err +} + +func catchCtrlC(cancel context.CancelFunc) { + signals := make(chan os.Signal, 1) + signal.Notify(signals, os.Interrupt, + syscall.SIGTERM, + syscall.SIGINT, + syscall.SIGPIPE, + ) + + go func() { + <-signals + cancel() + }() } diff --git a/pkg/skaffold/runner/runner.go b/pkg/skaffold/runner/runner.go index c716049dad6..c514cd8879d 100644 --- a/pkg/skaffold/runner/runner.go +++ b/pkg/skaffold/runner/runner.go @@ -22,8 +22,6 @@ import ( "io" "io/ioutil" "os" - "os/signal" - "syscall" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/tag" @@ -45,7 +43,6 @@ type SkaffoldRunner struct { watch.WatcherFactory build.DependencyMapFactory - opts *config.SkaffoldOptions builds []build.Build } @@ -83,7 +80,6 @@ func NewForConfig(opts *config.SkaffoldOptions, cfg *config.SkaffoldConfig) (*Sk Tagger: tagger, WatcherFactory: watch.NewWatcher, DependencyMapFactory: build.NewDependencyMap, - opts: opts, }, nil } @@ -167,33 +163,26 @@ func (r *SkaffoldRunner) Run(ctx context.Context, out io.Writer, artifacts []*v1 // Dev watches for changes and runs the skaffold build and deploy // pipeline until interrrupted by the user. -func (r *SkaffoldRunner) Dev(ctx context.Context, out io.Writer, artifacts []*v1alpha2.Artifact) error { - if r.opts.Cleanup { - return r.cleanUpOnCtrlC(ctx, out, artifacts) - } - return r.watchBuildDeploy(ctx, out, artifacts) -} - -func (r *SkaffoldRunner) watchBuildDeploy(ctx context.Context, out io.Writer, artifacts []*v1alpha2.Artifact) error { +func (r *SkaffoldRunner) Dev(ctx context.Context, out io.Writer, artifacts []*v1alpha2.Artifact) ([]build.Build, error) { depMap, err := r.DependencyMapFactory(artifacts) if err != nil { - return errors.Wrap(err, "getting path to dependency map") + return nil, errors.Wrap(err, "getting path to dependency map") } watcher, err := r.WatcherFactory(depMap.Paths()) if err != nil { - return errors.Wrap(err, "creating watcher") + return nil, errors.Wrap(err, "creating watcher") } deployDeps, err := r.Dependencies() if err != nil { - return errors.Wrap(err, "getting deploy dependencies") + return nil, errors.Wrap(err, "getting deploy dependencies") } logrus.Infof("Deployer dependencies: %s", deployDeps) deployWatcher, err := r.WatcherFactory(deployDeps) if err != nil { - return errors.Wrap(err, "creating deploy watcher") + return nil, errors.Wrap(err, "creating deploy watcher") } imageList := kubernetes.NewImageList() @@ -235,12 +224,12 @@ func (r *SkaffoldRunner) watchBuildDeploy(ctx context.Context, out io.Writer, ar } if err := onChange(depMap.Paths()); err != nil { - return errors.Wrap(err, "first build") + return nil, errors.Wrap(err, "first build") } // Start logs if err = logger.Start(ctx); err != nil { - return errors.Wrap(err, "starting logger") + return r.builds, errors.Wrap(err, "starting logger") } // Watch files and rebuild @@ -252,32 +241,7 @@ func (r *SkaffoldRunner) watchBuildDeploy(ctx context.Context, out io.Writer, ar return deployWatcher.Start(watchCtx, ioutil.Discard, onDeployChange) }) - return g.Wait() -} - -func (r *SkaffoldRunner) cleanUpOnCtrlC(ctx context.Context, out io.Writer, artifacts []*v1alpha2.Artifact) error { - ctx, cancel := context.WithCancel(ctx) - - signals := make(chan os.Signal, 1) - signal.Notify(signals, os.Interrupt, - syscall.SIGTERM, - syscall.SIGINT, - syscall.SIGPIPE, - ) - - go func() { - <-signals - cancel() - }() - - errRun := r.watchBuildDeploy(ctx, out, artifacts) - // Cleanup only if something was built - if r.builds != nil { - if err := r.Cleanup(ctx, out); err != nil { - logrus.Warnln("cleanup:", err) - } - } - return errRun + return r.builds, g.Wait() } func mergeWithPreviousBuilds(builds, previous []build.Build) []build.Build { diff --git a/pkg/skaffold/runner/runner_test.go b/pkg/skaffold/runner/runner_test.go index dc0dfd6491f..5ea54d4cf62 100644 --- a/pkg/skaffold/runner/runner_test.go +++ b/pkg/skaffold/runner/runner_test.go @@ -242,7 +242,6 @@ func TestRun(t *testing.T) { runner := &SkaffoldRunner{ Builder: test.builder, Deployer: test.deployer, - opts: &config.SkaffoldOptions{}, Tagger: &tag.ChecksumTagger{}, } err := runner.Run(context.Background(), ioutil.Discard, test.config.Build.Artifacts) @@ -290,12 +289,11 @@ func TestDev(t *testing.T) { runner := &SkaffoldRunner{ Builder: test.builder, Deployer: &TestDeployer{}, - opts: &config.SkaffoldOptions{}, Tagger: &tag.ChecksumTagger{}, DependencyMapFactory: build.NewDependencyMap, WatcherFactory: test.watcherFactory, } - err := runner.Dev(context.Background(), ioutil.Discard, nil) + _, err := runner.Dev(context.Background(), ioutil.Discard, nil) testutil.CheckError(t, test.shouldErr, err) }) @@ -318,7 +316,6 @@ func TestBuildAndDeployAllArtifacts(t *testing.T) { } runner := &SkaffoldRunner{ - opts: &config.SkaffoldOptions{}, Builder: builder, Deployer: deployer, DependencyMapFactory: func(artifacts []*v1alpha2.Artifact) (*build.DependencyMap, error) { @@ -330,7 +327,7 @@ func TestBuildAndDeployAllArtifacts(t *testing.T) { // All artifacts are changed runner.WatcherFactory = NewWatcherFactory(nil, []string{"path1", "path2"}) - err := runner.Dev(ctx, ioutil.Discard, artifacts) + _, err := runner.Dev(ctx, ioutil.Discard, artifacts) if err != nil { t.Errorf("Didn't expect an error. Got %s", err) @@ -344,7 +341,7 @@ func TestBuildAndDeployAllArtifacts(t *testing.T) { // Only one is changed runner.WatcherFactory = NewWatcherFactory(nil, []string{"path2"}) - err = runner.Dev(ctx, ioutil.Discard, artifacts) + _, err = runner.Dev(ctx, ioutil.Discard, artifacts) if err != nil { t.Errorf("Didn't expect an error. Got %s", err)