Fix empty diff being returned when single *cached* step had to be executed#567
Merged
Conversation
ca99fb1 to
027fe5f
Compare
mrnugget
commented
Jul 19, 2021
| return execResult, stepResults, err | ||
| } | ||
|
|
||
| const workDir = "/work" |
Contributor
Author
There was a problem hiding this comment.
Everything below this line is "just" extracted code from runSteps. There is no new logic, it's the same code, but in separate functions.
mrnugget
commented
Jul 19, 2021
Comment on lines
+98
to
+112
| // If we have cached results and don't need to execute any more steps, | ||
| // we can quit | ||
| if startStep == len(opts.task.Steps) { | ||
| changes, err := git.ChangesInDiff(opts.task.CachedResult.Diff) | ||
| if err != nil { | ||
| return execResult, nil, errors.Wrap(err, "parsing cached step diff") | ||
| } | ||
|
|
||
| execResult.Diff = string(opts.task.CachedResult.Diff) | ||
| execResult.ChangedFiles = &changes | ||
| stepResults = append(stepResults, opts.task.CachedResult) | ||
|
|
||
| return execResult, stepResults, nil | ||
| } | ||
|
|
Contributor
Author
There was a problem hiding this comment.
This is the fix for the problem this PR aims to address. The rest of the changes are either tests or were made to make this fix easy to make (like the removal of the whole results indexing, introducing previousStepResult, etc.)
courier-new
approved these changes
Jul 19, 2021
courier-new
left a comment
Contributor
There was a problem hiding this comment.
Looks good as far as I can tell! Nice cleanup, indeed!
16c5039 to
491487f
Compare
scjohns
pushed a commit
that referenced
this pull request
Apr 24, 2023
…cuted (#567) This PR fixes a bug in the step-wise caching logic that @eseliger and I ran into last week. ### Problem When no cached result for the *complete* task was found, but a cached result for a single step and that step was the only left to execute then an empty diff was returned. ### How to reproduce 1. Execute a batch spec with the following steps ```yaml steps: - run: echo "this is step 1" >> README.txt container: alpine:3 - run: echo "this is step 2" >> README.md container: alpine:3 ``` the complete results and the results for each step are now cached. 2. Update the batch spec and re-execute: ```yaml steps: - run: echo "this is step 1" >> README.txt container: alpine:3 ``` This will produce an empty diff because the `Coordinator` did not find a cached result for the complete task/batch spec, but for the first step. `runSteps` didn't handle this case though, when only a single step had to be executed but that was also cached. ### The fix Take a look at my comments in the diffs here in GitHub to see what the fix is. Spoiler: it's just a condition and an early exit to handle the case of "cached step result == the only step to execute". In order to get to this fix though I wrote a lot of tests, found one other bug (`Files` was not cached), and cleaned up the 300 line long `runSteps` function. `runSteps` is now much easier to understand and doesn't contain _all_ levels of abstraction. The outer `runSteps` concerns itself with which steps need to be executed and how to handle their results and `executeSingleStep` does the lower-level work of rendering templates, starting containers, setting up the logger, etc.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes a bug in the step-wise caching logic that @eseliger and I ran into last week.
Problem
When no cached result for the complete task was found, but a cached result for a single step and that step was the only left to execute then an empty diff was returned.
How to reproduce
Execute a batch spec with the following steps
the complete results and the results for each step are now cached.
Update the batch spec and re-execute:
This will produce an empty diff because the
Coordinatordid not find a cached result for the complete task/batch spec, but for the first step.runStepsdidn't handle this case though, when only a single step had to be executed but that was also cached.The fix
Take a look at my comments in the diffs here in GitHub to see what the fix is. Spoiler: it's just a condition and an early exit to handle the case of "cached step result == the only step to execute".
In order to get to this fix though I wrote a lot of tests, found one other bug (
Fileswas not cached), and cleaned up the 300 line longrunStepsfunction.runStepsis now much easier to understand and doesn't contain all levels of abstraction. The outerrunStepsconcerns itself with which steps need to be executed and how to handle their results andexecuteSingleStepdoes the lower-level work of rendering templates, starting containers, setting up the logger, etc.