Skip to content

Commit

Permalink
Make the dev loop interruptible.
Browse files Browse the repository at this point in the history 
Wether it’s during the build, the deploy or the
status check, the dev loop can now be interrupted
on a file change.

Fix GoogleContainerTools#3908
Fix GoogleContainerTools#916

If it’s the skaffold.yaml that’s modified, instead
Of restarting the dev loop right away, we first
validate that the config is valid.

Fix GoogleContainerTools#4225

Signed-off-by: David Gageot <david@gageot.net>
  • Loading branch information
@dgageot
dgageot committed Jul 31, 2020
1 parent 750455e commit 3598485
Show file tree
Hide file tree
Showing 4 changed files with 66 additions and 26 deletions.
21 changes: 16 additions & 5 deletions pkg/skaffold/runner/dev.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import (
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/filemon"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/kubernetes"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/kubernetes/portforward"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/sync"
"github.com/GoogleContainerTools/skaffold/proto"
Expand All @@ -49,6 +50,12 @@ var (

func (r *SkaffoldRunner) doDev(ctx context.Context, out io.Writer, logger *kubernetes.LogAggregator, forwarderManager portforward.Forwarder) error {
if r.changeSet.needsReload {
// If the config can't be parsed, do not exit the dev loop.
if _, err := schema.ParseConfigAndUpgrade(r.runCtx.Opts.ConfigurationFile, latest.Version); err != nil {
return err
}

// Bubble up the error so that a new dev loop is started.
return ErrorConfigurationChanged
}

Expand Down Expand Up @@ -96,8 +103,10 @@ func (r *SkaffoldRunner) doDev(ctx context.Context, out io.Writer, logger *kuber
}()

if _, err := r.BuildAndTest(ctx, out, r.changeSet.needsRebuild); err != nil {
logrus.Warnln("Skipping deploy due to error:", err)
event.DevLoopFailedInPhase(r.devIteration, sErrors.Build, err)
if !errors.Is(err, context.Canceled) {
logrus.Warnln("Skipping deploy due to error:", err)
event.DevLoopFailedInPhase(r.devIteration, sErrors.Build, err)
}
return nil
}
}
Expand All @@ -111,8 +120,10 @@ func (r *SkaffoldRunner) doDev(ctx context.Context, out io.Writer, logger *kuber

forwarderManager.Stop()
if err := r.Deploy(ctx, out, r.builds); err != nil {
logrus.Warnln("Skipping deploy due to error:", err)
event.DevLoopFailedInPhase(r.devIteration, sErrors.Deploy, err)
if !errors.Is(err, context.Canceled) {
logrus.Warnln("Skipping deploy due to error:", err)
event.DevLoopFailedInPhase(r.devIteration, sErrors.Deploy, err)
}
return nil
}
if err := forwarderManager.Start(ctx); err != nil {
Expand Down Expand Up @@ -242,7 +253,7 @@ func (r *SkaffoldRunner) Dev(ctx context.Context, out io.Writer, artifacts []*la
color.Yellow.Fprintln(out, "Press Ctrl+C to exit")

event.DevLoopComplete(0)
return r.listener.WatchForChanges(ctx, out, func() error {
return r.listener.WatchForChanges(ctx, out, func(ctx context.Context) error {
return r.doDev(ctx, out, logger, forwarderManager)
})
}
56 changes: 42 additions & 14 deletions pkg/skaffold/runner/listen.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,14 @@ import (
"io"

"github.com/sirupsen/logrus"
"golang.org/x/sync/errgroup"

"github.com/GoogleContainerTools/skaffold/pkg/skaffold/filemon"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/trigger"
)

type Listener interface {
WatchForChanges(context.Context, io.Writer, func() error) error
WatchForChanges(context.Context, io.Writer, func(ctx context.Context) error) error
LogWatchToUser(io.Writer)
}

Expand All @@ -45,10 +46,11 @@ func (l *SkaffoldListener) LogWatchToUser(out io.Writer) {

// WatchForChanges listens to a trigger, and when one is received, computes file changes and
// conditionally runs the dev loop.
func (l *SkaffoldListener) WatchForChanges(ctx context.Context, out io.Writer, devLoop func() error) error {
ctxTrigger, cancelTrigger := context.WithCancel(ctx)
defer cancelTrigger()
trigger, err := trigger.StartTrigger(ctxTrigger, l.Trigger)
func (l *SkaffoldListener) WatchForChanges(ctx context.Context, out io.Writer, devLoop func(ctx context.Context) error) error {
ctx, cancel := context.WithCancel(ctx)
defer cancel()

trigger, err := trigger.StartTrigger(ctx, l.Trigger)
if err != nil {
return fmt.Errorf("unable to start trigger: %w", err)
}
Expand All @@ -60,29 +62,55 @@ func (l *SkaffoldListener) WatchForChanges(ctx context.Context, out io.Writer, d

l.LogWatchToUser(out)

change := merge(l.intentChan, trigger)
eg, egCtx := errgroup.WithContext(ctx)
devCtx, cancelDev := context.WithCancel(egCtx)
for {
select {
case <-ctx.Done():
case <-egCtx.Done():
cancelDev()
return nil
case <-l.intentChan:
if err := l.do(devLoop); err != nil {
return err
}
case <-trigger:
if err := l.do(devLoop); err != nil {
case <-change:
// Cancel and wait for the previous dev cycle.
cancelDev()
err := eg.Wait()
if err != nil {
return err
}

// Start a new dev cycle.
eg, egCtx = errgroup.WithContext(ctx)
devCtx, cancelDev = context.WithCancel(egCtx)

eg.Go(func() error {
return l.do(devCtx, devLoop)
})
}
}
}

func (l *SkaffoldListener) do(devLoop func() error) error {
func merge(list ...<-chan bool) <-chan bool {
merged := make(chan bool)

for i := range list {
c := list[i]
go func() {
for range c {
merged <- true
}
}()
}

return merged
}

func (l *SkaffoldListener) do(ctx context.Context, devLoop func(ctx context.Context) error) error {
if err := l.Monitor.Run(l.Trigger.Debounce()); err != nil {
logrus.Warnf("Ignoring changes: %s", err.Error())
return nil
}

if err := devLoop(); err != nil {
if err := devLoop(ctx); err != nil {
// propagating this error up causes a new runner to be created
// and a new dev loop to start
if errors.Is(err, ErrorConfigurationChanged) {
Expand Down
7 changes: 4 additions & 3 deletions pkg/skaffold/runner/listen_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package runner

import (
"context"
"errors"
"testing"

Expand Down Expand Up @@ -58,7 +59,7 @@ func TestSkipDevLoopOnMonitorError(t *testing.T) {
}

var devLoopWasCalled bool
err := listener.do(func() error {
err := listener.do(context.Background(), func(context.Context) error {
devLoopWasCalled = true
return nil
})
Expand All @@ -72,7 +73,7 @@ func TestContinueOnDevLoopError(t *testing.T) {
Trigger: &fakeTriggger{},
}

err := listener.do(func() error {
err := listener.do(context.Background(), func(context.Context) error {
return errors.New("devloop error")
})

Expand All @@ -85,7 +86,7 @@ func TestReportDevLoopError(t *testing.T) {
Trigger: &fakeTriggger{},
}

err := listener.do(func() error {
err := listener.do(context.Background(), func(context.Context) error {
return ErrorConfigurationChanged
})

Expand Down
8 changes: 4 additions & 4 deletions pkg/skaffold/runner/runner_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ type TestBench struct {
deployErrors []error
namespaces []string

devLoop func(context.Context, io.Writer, func() error) error
devLoop func(context.Context, io.Writer, func(context.Context) error) error
firstMonitor func(bool) error
cycles int
currentCycle int
Expand Down Expand Up @@ -170,7 +170,7 @@ func (t *TestBench) Actions() []Actions {
return append(t.actions, t.currentActions)
}

func (t *TestBench) WatchForChanges(ctx context.Context, out io.Writer, doDev func() error) error {
func (t *TestBench) WatchForChanges(ctx context.Context, out io.Writer, doDev func(context.Context) error) error {
// don't actually call the monitor here, because extra actions would be added
if err := t.firstMonitor(true); err != nil {
return err
Expand Down Expand Up @@ -234,11 +234,11 @@ func createRunner(t *testutil.T, testBench *TestBench, monitor filemon.Monitor)
runner.listener = testBench
runner.monitor = monitor

testBench.devLoop = func(ctx context.Context, out io.Writer, doDev func() error) error {
testBench.devLoop = func(ctx context.Context, out io.Writer, doDev func(ctx context.Context) error) error {
if err := monitor.Run(true); err != nil {
return err
}
return doDev()
return doDev(ctx)
}

testBench.firstMonitor = func(bool) error {
Expand Down

0 comments on commit 3598485

Please sign in to comment.