Skip to content

Fix step caching #781

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
eseliger merged 2 commits into from
Jun 14, 2022
+96 −23
Merged

Fix step caching #781

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bcc60ba
Fix step caching
eseliger Jun 14, 2022
df72f92
Bump lib
eseliger Jun 14, 2022
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
6 changes: 3 additions & 3 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ require (
github.com/sourcegraph/go-diff v0.6.1
github.com/sourcegraph/jsonx v0.0.0-20200629203448-1a936bd500cf
github.com/sourcegraph/scip v0.1.0
github.com/sourcegraph/sourcegraph/lib v0.0.0-20220613202038-1231f0d77142
github.com/sourcegraph/sourcegraph/lib v0.0.0-20220614231716-f73c9fa26c46
github.com/stretchr/testify v1.7.2
golang.org/x/net v0.0.0-20220526153639-5463443f8c37
golang.org/x/sync v0.0.0-20210220032951-036812b2e83c
Expand Down Expand Up @@ -83,7 +83,7 @@ require (
github.com/rivo/uniseg v0.2.0 // indirect
github.com/rogpeppe/go-internal v1.8.1 // indirect
github.com/russross/blackfriday/v2 v2.1.0 // indirect
github.com/sourcegraph/log v0.0.0-20220608183241-51d325291e9a // indirect
github.com/sourcegraph/log v0.0.0-20220613150728-bb50c87ba841 // indirect
github.com/spf13/cobra v1.4.0 // indirect
github.com/spf13/pflag v1.0.5 // indirect
github.com/ssor/bom v0.0.0-20170718123548-6386211fdfcf // indirect
Expand All @@ -98,7 +98,7 @@ require (
go.uber.org/zap v1.21.0 // indirect
golang.org/x/crypto v0.0.0-20210921155107-089bfa567519 // indirect
golang.org/x/mod v0.6.0-dev.0.20220419223038-86c51ed26bb4 // indirect
golang.org/x/sys v0.0.0-20220610221304-9f5ed59c137d // indirect
golang.org/x/sys v0.0.0-20220614162138-6c1b26c55098 // indirect
golang.org/x/term v0.0.0-20220411215600-e5f449aeb171 // indirect
golang.org/x/text v0.3.7 // indirect
golang.org/x/tools v0.1.11 // indirect
Expand Down
12 changes: 6 additions & 6 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,12 +347,12 @@ github.com/sourcegraph/go-diff v0.6.1 h1:hmA1LzxW0n1c3Q4YbrFgg4P99GSnebYa3x8gr0H
github.com/sourcegraph/go-diff v0.6.1/go.mod h1:iBszgVvyxdc8SFZ7gm69go2KDdt3ag071iBaWPF6cjs=
github.com/sourcegraph/jsonx v0.0.0-20200629203448-1a936bd500cf h1:oAdWFqhStsWiiMP/vkkHiMXqFXzl1XfUNOdxKJbd6bI=
github.com/sourcegraph/jsonx v0.0.0-20200629203448-1a936bd500cf/go.mod h1:ppFaPm6kpcHnZGqQTFhUIAQRIEhdQDWP1PCv4/ON354=
github.com/sourcegraph/log v0.0.0-20220608183241-51d325291e9a h1:poxCcQH13nS9l4tkXN7+qRwmyxdO1lYeOUEY9ycwH84=
github.com/sourcegraph/log v0.0.0-20220608183241-51d325291e9a/go.mod h1:A+9F6IicYvBbl2aT0R81lMraKcXjVfdfw352yPe2yJI=
github.com/sourcegraph/log v0.0.0-20220613150728-bb50c87ba841 h1:kiYxuyQ1zSNA4YPtVVJQAFw5ZRNaFRkFqgKyEJQTzhg=
github.com/sourcegraph/log v0.0.0-20220613150728-bb50c87ba841/go.mod h1:A+9F6IicYvBbl2aT0R81lMraKcXjVfdfw352yPe2yJI=
github.com/sourcegraph/scip v0.1.0 h1:kTs0CJaLQvcRZjg+HpGrcJPNX2Tx31+d6szWio3ZOkQ=
github.com/sourcegraph/scip v0.1.0/go.mod h1:/AZ8RvsnRfeCZy232PJuVZqcl9f82fJnYwdWZeU2JCo=
github.com/sourcegraph/sourcegraph/lib v0.0.0-20220613202038-1231f0d77142 h1:wnp9JgzLmppL9SAUkkjFp8V2Q8Q/oLVJoqXRxHWx2zU=
github.com/sourcegraph/sourcegraph/lib v0.0.0-20220613202038-1231f0d77142/go.mod h1:RaBtTwfNRQvoRTEcbrKLesaEbWex/KblcPDd979zzww=
github.com/sourcegraph/sourcegraph/lib v0.0.0-20220614231716-f73c9fa26c46 h1:ehDPNEMxvaCKWlV6oGh2jvkcs4bJzL020aSHjxNLSZs=
github.com/sourcegraph/sourcegraph/lib v0.0.0-20220614231716-f73c9fa26c46/go.mod h1:Orrt+5wdseAvxsVxgdswYPzJgVkTk0rLlmFHZW61epo=
github.com/sourcegraph/yaml v1.0.1-0.20200714132230-56936252f152 h1:z/MpntplPaW6QW95pzcAR/72Z5TWDyDnSo0EOcyij9o=
github.com/sourcegraph/yaml v1.0.1-0.20200714132230-56936252f152/go.mod h1:GIjDIg/heH5DOkXY3YJ/wNhfHsQHoXGjl8G8amsYQ1I=
github.com/spf13/afero v1.1.2/go.mod h1:j4pytiNVoe2o6bmDsKpLACNPDBIoEAkihy7loJ1B0CQ=
Expand Down Expand Up @@ -503,8 +503,8 @@ golang.org/x/sys v0.0.0-20210630005230-0f9fa26af87c/go.mod h1:oPkhp1MJrh7nUepCBc
golang.org/x/sys v0.0.0-20210927094055-39ccf1dd6fa6/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.0.0-20211007075335-d3039528d8ac/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.0.0-20220209214540-3681064d5158/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.0.0-20220610221304-9f5ed59c137d h1:Zu/JngovGLVi6t2J3nmAf3AoTDwuzw85YZ3b9o4yU7s=
golang.org/x/sys v0.0.0-20220610221304-9f5ed59c137d/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.0.0-20220614162138-6c1b26c55098 h1:PgOr27OhUx2IRqGJ2RxAWI4dJQ7bi9cSrB82uzFzfUA=
golang.org/x/sys v0.0.0-20220614162138-6c1b26c55098/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo=
golang.org/x/term v0.0.0-20220411215600-e5f449aeb171 h1:EH1Deb8WZJ0xc0WK//leUHXcX9aLE5SymusoTmMZye8=
golang.org/x/term v0.0.0-20220411215600-e5f449aeb171/go.mod h1:jbD1KX2456YbFQfuXm/mYQcufACuNUgVhRMnK/tPxf8=
Expand Down
87 changes: 80 additions & 7 deletions internal/batches/executor/executor_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -545,10 +545,10 @@ index 02a19af..c9644dd 100644
},
CachedResultFound: true,
CachedResult: execution.AfterStepResult{
StepIndex: 0,
Diff: cachedDiff,
Outputs: map[string]interface{}{},
PreviousStepResult: execution.StepResult{},
StepIndex: 0,
Diff: cachedDiff,
Outputs: map[string]interface{}{},
StepResult: execution.StepResult{},
},
Repository: testRepo1,
}
Expand Down Expand Up @@ -666,13 +666,13 @@ echo "previous_step.modified_files=${{ previous_step.modified_files }}" >> READM
Outputs: map[string]interface{}{
"myOutput": "my-output.txt",
},
PreviousStepResult: execution.StepResult{
StepResult: execution.StepResult{
Files: &git.Changes{
Modified: []string{"README.md"},
Added: []string{"README.txt"},
},
Stdout: nil,
Stderr: nil,
Stdout: "",
Stderr: "",
},
},
}
Expand Down Expand Up @@ -709,6 +709,79 @@ echo "previous_step.modified_files=${{ previous_step.modified_files }}" >> READM
t.Fatalf("wrong step result outputs: %s", diff)
}
})

t.Run("step stdout cached", func(t *testing.T) {
archive := mock.RepoArchive{
RepoName: testRepo1.Name, Commit: testRepo1.Rev(),
Files: map[string]string{
"README.md": `# automation-testing
This repository is used to test opening and closing pull request with Automation
`,
},
}

wantFinalDiff := `diff --git README.md README.md
index 3040106..5f2f924 100644
--- README.md
+++ README.md
@@ -1,2 +1,3 @@
# automation-testing
This repository is used to test opening and closing pull request with Automation
+hello world
`

task := &Task{
Repository: testRepo1,
BatchChangeAttributes: &template.BatchChangeAttributes{},
Steps: []batcheslib.Step{
{Run: "echo -n Hello world"},
{Run: `echo ${{ previous_step.stdout }} >> README.md`},
},
CachedResultFound: true,
CachedResult: execution.AfterStepResult{
StepIndex: 0,
Diff: "",
Outputs: map[string]interface{}{},
StepResult: execution.StepResult{
Files: &git.Changes{},
Stdout: "hello world",
Stderr: "",
},
},
}

results, err := testExecuteTasks(t, []*Task{task}, archive)
if err != nil {
t.Fatalf("execution failed: %s", err)
}

if have, want := len(results), 1; have != want {
t.Fatalf("wrong number of execution results. want=%d, have=%d", want, have)
}

executionResult := results[0].result
if diff := cmp.Diff(executionResult.Diff, wantFinalDiff); diff != "" {
t.Fatalf("wrong diff: %s", diff)
}

if diff := cmp.Diff(executionResult.Outputs, task.CachedResult.Outputs); diff != "" {
t.Fatalf("wrong execution result outputs: %s", diff)
}

// Only one step should've been executed
if have, want := len(results[0].stepResults), 1; have != want {
t.Fatalf("wrong length of step results. have=%d, want=%d", have, want)
}

lastStepResult := results[0].stepResults[0]
if have, want := lastStepResult.StepIndex, 1; have != want {
t.Fatalf("wrong stepIndex. have=%d, want=%d", have, want)
}

if diff := cmp.Diff(lastStepResult.Outputs, task.CachedResult.Outputs); diff != "" {
t.Fatalf("wrong step result outputs: %s", diff)
}
})
}

func testExecuteTasks(t *testing.T, tasks []*Task, archives ...mock.RepoArchive) ([]taskResult, error) {
Expand Down
14 changes: 7 additions & 7 deletions internal/batches/executor/run_steps.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ func runSteps(ctx context.Context, opts *executionOpts) (result execution.Result
}

if opts.task.CachedResultFound && i == startStep {
previousStepResult = opts.task.CachedResult.PreviousStepResult
previousStepResult = opts.task.CachedResult.StepResult

stepContext.PreviousStep = previousStepResult
stepContext.Steps.Changes = previousStepResult.Files
Expand All @@ -125,7 +125,7 @@ func runSteps(ctx context.Context, opts *executionOpts) (result execution.Result
// apply them.
if opts.task.CachedResult.Diff != "" {
if err := ws.ApplyDiff(ctx, []byte(opts.task.CachedResult.Diff)); err != nil {
return execResult, nil, errors.Wrap(err, "getting changed files in step")
return execResult, nil, errors.Wrap(err, "applying diff of cache result")
}
}
}
Expand Down Expand Up @@ -168,7 +168,7 @@ func runSteps(ctx context.Context, opts *executionOpts) (result execution.Result
return execResult, nil, errors.Wrap(err, "getting changed files in step")
}

result := execution.StepResult{Files: changes, Stdout: &stdoutBuffer, Stderr: &stderrBuffer}
result := execution.StepResult{Files: changes, Stdout: stdoutBuffer.String(), Stderr: stderrBuffer.String()}

// Set stepContext.Step to current step's results before rendering outputs
stepContext.Step = result
Expand All @@ -183,10 +183,10 @@ func runSteps(ctx context.Context, opts *executionOpts) (result execution.Result
return execResult, nil, errors.Wrap(err, "getting diff produced by step")
}
stepResult := execution.AfterStepResult{
StepIndex: i,
Diff: string(stepDiff),
Outputs: make(map[string]interface{}),
PreviousStepResult: stepContext.PreviousStep,
StepIndex: i,
Diff: string(stepDiff),
Outputs: make(map[string]interface{}),
StepResult: result,

@eseliger eseliger Jun 14, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This was just weirdly wrong :D this meant that we got.. the second last result from the cache entry?

@mrnugget mrnugget Jun 15, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmmmmm, I guess this is on me. This part of the code is really tricky - I hope we do have tests covering all the edge cases 😬

}
for k, v := range execResult.Outputs {
stepResult.Outputs[k] = v
Expand Down