Skip to content
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.

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

Simplify doDev() #2815

Merged
merged 1 commit into from
Sep 6, 2019
Merged
Changes from all commits
Commits
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
56 changes: 26 additions & 30 deletions pkg/skaffold/runner/dev.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,30 +34,30 @@ import (
var ErrorConfigurationChanged = errors.New("configuration changed")

func (r *SkaffoldRunner) doDev(ctx context.Context, out io.Writer) error {
actionPerformed := false
if r.changeSet.needsReload {
return ErrorConfigurationChanged
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we want the logs muted during this time right? otherwise we'll have deployment logs interleaved with the build/deploy logs from the new skaffold runner.

also, r.changeSet.resetReload()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In fact, the logs will be stopped when this function returns: here.
Also, a new runner, with a new changeset will be created.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should write tests expressing these expectations.
It seems that there was no log muting in the current version - but I think I agree that interleaving might happen - are you suggesting adding log muting?

Copy link
Contributor

Choose a reason for hiding this comment

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

ahh very nice, thanks for clarifying. this is much cleaner! 👍

}

// acquire the intents
buildIntent, syncIntent, deployIntent := r.intents.GetIntents()

if (r.changeSet.needsRedeploy && deployIntent) ||
(len(r.changeSet.needsRebuild) > 0 && buildIntent) ||
(len(r.changeSet.needsResync) > 0 && syncIntent) {
r.logger.Mute()
// if any action is going to be performed, reset the monitor's changed component tracker for debouncing
defer r.monitor.Reset()
defer r.listener.LogWatchToUser(out)
needsSync := syncIntent && len(r.changeSet.needsResync) > 0
needsBuild := buildIntent && len(r.changeSet.needsRebuild) > 0
needsDeploy := deployIntent && r.changeSet.needsRedeploy
if !needsSync && !needsBuild && !needsDeploy {
return nil
}

r.logger.Mute()
// if any action is going to be performed, reset the monitor's changed component tracker for debouncing
defer r.monitor.Reset()
defer r.listener.LogWatchToUser(out)

switch {
case r.changeSet.needsReload:
r.changeSet.resetReload()
return ErrorConfigurationChanged
case len(r.changeSet.needsResync) > 0 && syncIntent:
case needsSync:
defer func() {
r.changeSet.resetSync()
r.intents.resetSync()
}()
actionPerformed = true

for _, s := range r.changeSet.needsResync {
color.Default.Fprintf(out, "Syncing %d files for %s\n", len(s.Copy)+len(s.Delete), s.Image)

Expand All @@ -67,45 +67,41 @@ func (r *SkaffoldRunner) doDev(ctx context.Context, out io.Writer) error {
return nil
}
}
case len(r.changeSet.needsRebuild) > 0 && buildIntent:

case needsBuild:
defer func() {
r.changeSet.resetBuild()
r.intents.resetBuild()
}()
// this linter apparently doesn't understand fallthroughs
//nolint:ineffassign
actionPerformed = true

if _, err := r.BuildAndTest(ctx, out, r.changeSet.needsRebuild); err != nil {
r.changeSet.reset()
logrus.Warnln("Skipping deploy due to error:", err)
return nil
}
r.changeSet.needsRedeploy = true
fallthrough // always try a redeploy after a successful build
case r.changeSet.needsRedeploy && deployIntent:
if !deployIntent {
// in case we fell through, but haven't received the go ahead to deploy
r.logger.Unmute()
return nil
break
}
actionPerformed = true
r.forwarderManager.Stop()
fallthrough // redeploy after a successful build

case needsDeploy:
defer func() {
r.changeSet.reset()
r.intents.resetDeploy()
}()

r.forwarderManager.Stop()
if err := r.Deploy(ctx, out, r.builds); err != nil {
logrus.Warnln("Skipping deploy due to error:", err)
return nil
}
if err := r.forwarderManager.Start(ctx); err != nil {
logrus.Warnln("Port forwarding failed due to error:", err)
logrus.Warnln("Port forwarding failed:", err)
}
}

if actionPerformed {
r.logger.Unmute()
}
r.logger.Unmute()
return nil
}

Expand Down