Skip to content

Cleanup only if something was actually deployed #1343

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Nov 30, 2018
Merged

Cleanup only if something was actually deployed #1343

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b95e2f9
Returned value is always nil
dgageot Nov 30, 2018
c6bcd75
Cleanup only if something was actually deployed
dgageot Nov 30, 2018
1c7c212
Fix typo
dgageot Nov 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions cmd/skaffold/app/cmd/dev.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,11 +52,10 @@ func dev(out io.Writer) error {
defer cancel()
catchCtrlC(cancel)

cleanup := func() {}
if opts.Cleanup {
defer func() {
if err := delete(out); err != nil {
logrus.Warnln("cleanup:", err)
}
cleanup()
}()
}

Expand All @@ -70,7 +69,15 @@ func dev(out io.Writer) error {
return errors.Wrap(err, "creating runner")
}

if _, err := r.Dev(ctx, out, config.Build.Artifacts); err != nil {
err = r.Dev(ctx, out, config.Build.Artifacts)
if r.HasDeployed() {
cleanup = func() {
if err := r.Cleanup(context.Background(), out); err != nil {
logrus.Warnln("cleanup:", err)
}
}
}
if err != nil {
if errors.Cause(err) != runner.ErrorConfigurationChanged {
return err
}
Expand Down
31 changes: 22 additions & 9 deletions pkg/skaffold/runner/runner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ type SkaffoldRunner struct {
opts *config.SkaffoldOptions
watchFactory watch.Factory
builds []build.Artifact
hasDeployed bool
imageList *kubernetes.ImageList
}

Expand Down Expand Up @@ -207,6 +208,11 @@ func (r *SkaffoldRunner) newLogger(out io.Writer, artifacts []*latest.Artifact)
return kubernetes.NewLogAggregator(out, imageNames, r.imageList)
}

// HasDeployed returns true if this runner has deployed something.
func (r *SkaffoldRunner) HasDeployed() bool {
return r.hasDeployed
}

func (r *SkaffoldRunner) buildTestDeploy(ctx context.Context, out io.Writer, artifacts []*latest.Artifact) error {
bRes, err := r.BuildAndTest(ctx, out, artifacts)
if err != nil {
Expand Down Expand Up @@ -260,6 +266,13 @@ func (r *SkaffoldRunner) BuildAndTest(ctx context.Context, out io.Writer, artifa
return bRes, err
}

// Deploy deploys the given artifacts
func (r *SkaffoldRunner) Deploy(ctx context.Context, out io.Writer, artifacts []build.Artifact) ([]deploy.Artifact, error) {
dRes, err := r.Deployer.Deploy(ctx, out, artifacts)
r.hasDeployed = true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it's not different than what we had before, but if Deploy() errors out do we want to set this flag?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it could fail and have deployed a few things. We want to clean this up

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, not really a good way to know whether or not that's the case so let's just be safe.

return dRes, err
}

// TailLogs prints the logs for deployed artifacts.
func (r *SkaffoldRunner) TailLogs(ctx context.Context, out io.Writer, artifacts []*latest.Artifact, bRes []build.Artifact) error {
if !r.opts.Tail {
Expand All @@ -281,7 +294,7 @@ func (r *SkaffoldRunner) TailLogs(ctx context.Context, out io.Writer, artifacts

// 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 []*latest.Artifact) ([]build.Artifact, error) {
func (r *SkaffoldRunner) Dev(ctx context.Context, out io.Writer, artifacts []*latest.Artifact) error {
logger := r.newLogger(out, artifacts)

// Create watcher and register artifacts to build current state of files.
Expand Down Expand Up @@ -349,7 +362,7 @@ func (r *SkaffoldRunner) Dev(ctx context.Context, out io.Writer, artifacts []*la
func() ([]string, error) { return DependenciesForArtifact(ctx, artifact) },
func(e watch.Events) { changed.AddDirtyArtifact(artifact, e) },
); err != nil {
return nil, errors.Wrapf(err, "watching files for artifact %s", artifact.ImageName)
return errors.Wrapf(err, "watching files for artifact %s", artifact.ImageName)
}
}

Expand All @@ -358,47 +371,47 @@ func (r *SkaffoldRunner) Dev(ctx context.Context, out io.Writer, artifacts []*la
func() ([]string, error) { return r.TestDependencies() },
func(watch.Events) { changed.needsRedeploy = true },
); err != nil {
return nil, errors.Wrap(err, "watching test files")
return errors.Wrap(err, "watching test files")
}

// Watch deployment configuration
if err := watcher.Register(
func() ([]string, error) { return r.Dependencies() },
func(watch.Events) { changed.needsRedeploy = true },
); err != nil {
return nil, errors.Wrap(err, "watching files for deployer")
return errors.Wrap(err, "watching files for deployer")
}

// Watch Skaffold configuration
if err := watcher.Register(
func() ([]string, error) { return []string{r.opts.ConfigurationFile}, nil },
func(watch.Events) { changed.needsReload = true },
); err != nil {
return nil, errors.Wrapf(err, "watching skaffold configuration %s", r.opts.ConfigurationFile)
return errors.Wrapf(err, "watching skaffold configuration %s", r.opts.ConfigurationFile)
}

// First run
if err := r.buildTestDeploy(ctx, out, artifacts); err != nil {
return nil, errors.Wrap(err, "exiting dev mode because first run failed")
return errors.Wrap(err, "exiting dev mode because first run failed")
}

// Start logs
if r.opts.TailDev {
if err := logger.Start(ctx); err != nil {
return nil, errors.Wrap(err, "starting logger")
return errors.Wrap(err, "starting logger")
}
}

if r.opts.PortForward {
portForwarder := kubernetes.NewPortForwarder(out, r.imageList)

if err := portForwarder.Start(ctx); err != nil {
return nil, errors.Wrap(err, "starting port-forwarder")
return errors.Wrap(err, "starting port-forwarder")
}
}

r.Trigger.WatchForChanges(out)
return nil, watcher.Run(ctx, r.Trigger, onChange)
return watcher.Run(ctx, r.Trigger, onChange)
}

func (r *SkaffoldRunner) shouldWatch(artifact *latest.Artifact) bool {
Expand Down
6 changes: 3 additions & 3 deletions pkg/skaffold/runner/runner_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -394,7 +394,7 @@ func TestDev(t *testing.T) {
runner.Deployer = test.deployer
runner.watchFactory = test.watcherFactory

_, err := runner.Dev(context.Background(), ioutil.Discard, nil)
err := runner.Dev(context.Background(), ioutil.Discard, nil)

testutil.CheckError(t, test.shouldErr, err)
})
Expand All @@ -417,7 +417,7 @@ func TestBuildAndDeployAllArtifacts(t *testing.T) {

// Both artifacts are changed
runner.watchFactory = NewWatcherFactory(nil, nil, []int{0, 1})
_, 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)
Expand All @@ -431,7 +431,7 @@ func TestBuildAndDeployAllArtifacts(t *testing.T) {

// Only one is changed
runner.watchFactory = NewWatcherFactory(nil, nil, []int{1})
_, 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)
Expand Down