Skip to content

Commit

Permalink
Set redeploy intent only when there are rebuilt artifacts (#5553)
Browse files Browse the repository at this point in the history 
* Set `redeploy` intent only when there are rebuilt artifacts

* Update pkg/skaffold/runner/dev.go

Co-authored-by: Brian de Alwis <[email protected]>

* add UT

Co-authored-by: Brian de Alwis <[email protected]>
  • Loading branch information
@gsquared94 @briandealwis
gsquared94 and briandealwis authored Mar 24, 2021
1 parent 3a6c3e0 commit 8b4cfda
Show file tree
Hide file tree
Showing 6 changed files with 143 additions and 16 deletions.
8 changes: 4 additions & 4 deletions pkg/skaffold/runner/build_deploy_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand Down
1 change: 0 additions & 1 deletion pkg/skaffold/runner/changeset.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
4 changes: 2 additions & 2 deletions pkg/skaffold/runner/deploy_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down Expand Up @@ -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{
Expand Down
2 changes: 2 additions & 0 deletions pkg/skaffold/runner/dev.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
127 changes: 122 additions & 5 deletions pkg/skaffold/runner/dev_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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)

Expand All @@ -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
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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)

Expand Down
17 changes: 13 additions & 4 deletions pkg/skaffold/runner/runner_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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)
Expand Down

0 comments on commit 8b4cfda

Please sign in to comment.