From 8b4cfdae3ce07862279e1341e430341b78794b25 Mon Sep 17 00:00:00 2001 From: Gaurav <39389231+gsquared94@users.noreply.github.com> Date: Wed, 24 Mar 2021 14:15:35 -0700 Subject: [PATCH] Set `redeploy` intent only when there are rebuilt artifacts (#5553) * Set `redeploy` intent only when there are rebuilt artifacts * Update pkg/skaffold/runner/dev.go Co-authored-by: Brian de Alwis * add UT Co-authored-by: Brian de Alwis --- pkg/skaffold/runner/build_deploy_test.go | 8 +- pkg/skaffold/runner/changeset.go | 1 - pkg/skaffold/runner/deploy_test.go | 4 +- pkg/skaffold/runner/dev.go | 2 + pkg/skaffold/runner/dev_test.go | 127 ++++++++++++++++++++++- pkg/skaffold/runner/runner_test.go | 17 ++- 6 files changed, 143 insertions(+), 16 deletions(-) diff --git a/pkg/skaffold/runner/build_deploy_test.go b/pkg/skaffold/runner/build_deploy_test.go index 242cc2ee1c2..b33d0976885 100644 --- a/pkg/skaffold/runner/build_deploy_test.go +++ b/pkg/skaffold/runner/build_deploy_test.go @@ -75,7 +75,7 @@ func TestTest(t *testing.T) { } for _, test := range tests { testutil.Run(t, test.description, func(t *testutil.T) { - runner := createRunner(t, test.testBench, nil, test.cfg) + runner := createRunner(t, test.testBench, nil, test.cfg, nil) err := runner.Test(context.Background(), ioutil.Discard, test.artifacts) @@ -136,7 +136,7 @@ func TestBuildTestDeploy(t *testing.T) { ImageName: "img", }} - runner := createRunner(t, test.testBench, nil, artifacts) + runner := createRunner(t, test.testBench, nil, artifacts, nil) bRes, err := runner.Build(ctx, ioutil.Discard, artifacts) if err == nil { err = runner.Test(ctx, ioutil.Discard, bRes) @@ -157,7 +157,7 @@ func TestBuildDryRun(t *testing.T) { {ImageName: "img1"}, {ImageName: "img2"}, } - runner := createRunner(t, testBench, nil, artifacts) + runner := createRunner(t, testBench, nil, artifacts, nil) runner.runCtx.Opts.DryRun = true bRes, err := runner.Build(context.Background(), ioutil.Discard, artifacts) @@ -178,7 +178,7 @@ func TestBuildSkipBuild(t *testing.T) { {ImageName: "img1"}, {ImageName: "img2"}, } - runner := createRunner(t, testBench, nil, artifacts) + runner := createRunner(t, testBench, nil, artifacts, nil) runner.runCtx.Opts.DigestSource = "none" bRes, err := runner.Build(context.Background(), ioutil.Discard, artifacts) diff --git a/pkg/skaffold/runner/changeset.go b/pkg/skaffold/runner/changeset.go index 01335d92e52..ca35910a107 100644 --- a/pkg/skaffold/runner/changeset.go +++ b/pkg/skaffold/runner/changeset.go @@ -41,7 +41,6 @@ func (c *changeSet) AddRebuild(a *latest.Artifact) { } c.rebuildTracker[a.ImageName] = a c.needsRebuild = append(c.needsRebuild, a) - c.needsRedeploy = true } func (c *changeSet) AddResync(s *sync.Item) { diff --git a/pkg/skaffold/runner/deploy_test.go b/pkg/skaffold/runner/deploy_test.go index 91148ab43bf..8612869586d 100644 --- a/pkg/skaffold/runner/deploy_test.go +++ b/pkg/skaffold/runner/deploy_test.go @@ -73,7 +73,7 @@ func TestDeploy(t *testing.T) { return dummyStatusChecker{} }) - runner := createRunner(t, test.testBench, nil, []*latest.Artifact{{ImageName: "img1"}, {ImageName: "img2"}}) + runner := createRunner(t, test.testBench, nil, []*latest.Artifact{{ImageName: "img1"}, {ImageName: "img2"}}, nil) runner.runCtx.Opts.StatusCheck = test.statusCheck out := new(bytes.Buffer) @@ -123,7 +123,7 @@ func TestDeployNamespace(t *testing.T) { return dummyStatusChecker{} }) - runner := createRunner(t, test.testBench, nil, []*latest.Artifact{{ImageName: "img1"}, {ImageName: "img2"}}) + runner := createRunner(t, test.testBench, nil, []*latest.Artifact{{ImageName: "img1"}, {ImageName: "img2"}}, nil) runner.runCtx.Namespaces = test.Namespaces runner.Deploy(context.Background(), ioutil.Discard, []build.Artifact{ diff --git a/pkg/skaffold/runner/dev.go b/pkg/skaffold/runner/dev.go index d0b828d831f..82d479a306f 100644 --- a/pkg/skaffold/runner/dev.go +++ b/pkg/skaffold/runner/dev.go @@ -114,6 +114,8 @@ func (r *SkaffoldRunner) doDev(ctx context.Context, out io.Writer, logger *kuber return nil } needsTest = true + r.changeSet.needsRedeploy = true + needsDeploy = deployIntent } if needsTest && !r.runCtx.SkipTests() { diff --git a/pkg/skaffold/runner/dev_test.go b/pkg/skaffold/runner/dev_test.go index 7b1f9d426c3..ffbc198ccf3 100644 --- a/pkg/skaffold/runner/dev_test.go +++ b/pkg/skaffold/runner/dev_test.go @@ -22,6 +22,7 @@ import ( "io/ioutil" "testing" + "github.com/google/go-cmp/cmp" k8s "k8s.io/client-go/kubernetes" fakekubeclientset "k8s.io/client-go/kubernetes/fake" "k8s.io/client-go/tools/clientcmd/api" @@ -140,7 +141,7 @@ func TestDevFailFirstCycle(t *testing.T) { artifacts := []*latest.Artifact{{ ImageName: "img", }} - runner := createRunner(t, test.testBench, test.monitor, artifacts) + runner := createRunner(t, test.testBench, test.monitor, artifacts, nil) test.testBench.firstMonitor = test.monitor.Run err := runner.Dev(context.Background(), ioutil.Discard, artifacts) @@ -275,7 +276,7 @@ func TestDev(t *testing.T) { runner := createRunner(t, test.testBench, &TestMonitor{ events: test.watchEvents, testBench: test.testBench, - }, artifacts) + }, artifacts, nil) err := runner.Dev(context.Background(), ioutil.Discard, artifacts) @@ -410,7 +411,7 @@ func TestDev_WithDependencies(t *testing.T) { runner := createRunner(t, test.testBench, &TestMonitor{ events: test.watchEvents, testBench: test.testBench, - }, artifacts) + }, artifacts, nil) err := runner.Dev(context.Background(), ioutil.Discard, artifacts) @@ -420,6 +421,122 @@ func TestDev_WithDependencies(t *testing.T) { } } +func TestDevAutoTriggers(t *testing.T) { + tests := []struct { + description string + watchEvents []filemon.Events + expectedActions []Actions + autoTriggers triggerState // the state of auto triggers + singleTriggers triggerState // the state of single intent triggers at the end of dev loop + }{ + { + description: "build on; sync on; deploy on", + watchEvents: []filemon.Events{ + {Modified: []string{"file1"}}, + {Modified: []string{"file2"}}, + }, + autoTriggers: triggerState{true, true, true}, + singleTriggers: triggerState{true, true, true}, + expectedActions: []Actions{ + { + Synced: []string{"img1:1"}, + }, + { + Built: []string{"img2:2"}, + Tested: []string{"img2:2"}, + Deployed: []string{"img1:1", "img2:2"}, + }, + }, + }, + { + description: "build off; sync off; deploy off", + watchEvents: []filemon.Events{ + {Modified: []string{"file1"}}, + {Modified: []string{"file2"}}, + }, + expectedActions: []Actions{{}, {}}, + }, + { + description: "build on; sync off; deploy off", + watchEvents: []filemon.Events{ + {Modified: []string{"file1"}}, + {Modified: []string{"file2"}}, + }, + autoTriggers: triggerState{true, false, false}, + singleTriggers: triggerState{true, false, false}, + expectedActions: []Actions{{}, { + Built: []string{"img2:2"}, + Tested: []string{"img2:2"}, + }}, + }, + { + description: "build off; sync on; deploy off", + watchEvents: []filemon.Events{ + {Modified: []string{"file1"}}, + {Modified: []string{"file2"}}, + }, + autoTriggers: triggerState{false, true, false}, + singleTriggers: triggerState{false, true, false}, + expectedActions: []Actions{{ + Synced: []string{"img1:1"}, + }, {}}, + }, + { + description: "build off; sync off; deploy on", + watchEvents: []filemon.Events{ + {Modified: []string{"file1"}}, + {Modified: []string{"file2"}}, + }, + autoTriggers: triggerState{false, false, true}, + singleTriggers: triggerState{false, false, true}, + expectedActions: []Actions{{}, {}}, + }, + } + // first build-test-deploy sequence always happens + firstAction := Actions{ + Built: []string{"img1:1", "img2:1"}, + Tested: []string{"img1:1", "img2:1"}, + Deployed: []string{"img1:1", "img2:1"}, + } + + for _, test := range tests { + testutil.Run(t, test.description, func(t *testutil.T) { + t.SetupFakeKubernetesContext(api.Config{CurrentContext: "cluster1"}) + t.Override(&client.Client, mockK8sClient) + t.Override(&sync.WorkingDir, func(string, docker.Config) (string, error) { return "/", nil }) + testBench := &TestBench{} + testBench.cycles = len(test.watchEvents) + artifacts := []*latest.Artifact{ + { + ImageName: "img1", + Sync: &latest.Sync{ + Manual: []*latest.SyncRule{{Src: "file1", Dest: "file1"}}, + }, + }, + { + ImageName: "img2", + }, + } + runner := createRunner(t, testBench, &TestMonitor{ + events: test.watchEvents, + testBench: testBench, + }, artifacts, &test.autoTriggers) + + err := runner.Dev(context.Background(), ioutil.Discard, artifacts) + + t.CheckNoError(err) + t.CheckDeepEqual(append([]Actions{firstAction}, test.expectedActions...), testBench.Actions()) + + singleTriggers := triggerState{ + build: runner.intents.build, + sync: runner.intents.sync, + deploy: runner.intents.deploy, + } + t.CheckDeepEqual(test.singleTriggers, singleTriggers, cmp.AllowUnexported(triggerState{})) + }) + } +} + func TestDevSync(t *testing.T) { type fileSyncEventCalls struct { InProgress int @@ -507,7 +624,7 @@ func TestDevSync(t *testing.T) { runner := createRunner(t, test.testBench, &TestMonitor{ events: test.watchEvents, testBench: test.testBench, - }, artifacts) + }, artifacts, nil) err := runner.Dev(context.Background(), ioutil.Discard, artifacts) @@ -599,7 +716,7 @@ func TestDevSync_WithDependencies(t *testing.T) { runner := createRunner(t, test.testBench, &TestMonitor{ events: test.watchEvents, testBench: test.testBench, - }, artifacts) + }, artifacts, nil) err := runner.Dev(context.Background(), ioutil.Discard, artifacts) diff --git a/pkg/skaffold/runner/runner_test.go b/pkg/skaffold/runner/runner_test.go index fb3e8909797..f3cc71562ce 100644 --- a/pkg/skaffold/runner/runner_test.go +++ b/pkg/skaffold/runner/runner_test.go @@ -203,7 +203,16 @@ func (r *SkaffoldRunner) WithMonitor(m filemon.Monitor) *SkaffoldRunner { return r } -func createRunner(t *testutil.T, testBench *TestBench, monitor filemon.Monitor, artifacts []*latest.Artifact) *SkaffoldRunner { +type triggerState struct { + build bool + sync bool + deploy bool +} + +func createRunner(t *testutil.T, testBench *TestBench, monitor filemon.Monitor, artifacts []*latest.Artifact, autoTriggers *triggerState) *SkaffoldRunner { + if autoTriggers == nil { + autoTriggers = &triggerState{true, true, true} + } cfg := &latest.SkaffoldConfig{ Pipeline: latest.Pipeline{ Build: latest.BuildConfig{ @@ -223,9 +232,9 @@ func createRunner(t *testutil.T, testBench *TestBench, monitor filemon.Monitor, Opts: config.SkaffoldOptions{ Trigger: "polling", WatchPollInterval: 100, - AutoBuild: true, - AutoSync: true, - AutoDeploy: true, + AutoBuild: autoTriggers.build, + AutoSync: autoTriggers.sync, + AutoDeploy: autoTriggers.deploy, }, } runner, err := NewForConfig(runCtx)