Only apply cache result progress if resulted in diff #778
Conversation
This file contains 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
Piszmog
approved these changes
Jun 13, 2022
| return execResult, nil, errors.Wrap(err, "getting changed files in step") | ||
| // If the previous steps made any modifications in the workspace yet, | ||
| // apply them. | ||
| if opts.task.CachedResult.Diff != "" { |
There was a problem hiding this comment.
Are there existing tests to cover when CachedResult.Diff is blank and not blank?
There was a problem hiding this comment.
Another PR just found this bug, and it has a test for it so will merge this and then in the next PR you'll get that test :)
courier-new
approved these changes
Jun 13, 2022
LawnGnome
approved these changes
Jun 14, 2022
This was reported by a user, we store execution step results in the cache also when no diff was generated (rightfully), but then we should only apply the patch when there is actually anything to do. Also, I initially couldn't reproduce this locally because we swallowed this error in volume mode. It would only happen in bind mode.
dedd19b to
cfc2e91
Compare
scjohns
pushed a commit
that referenced
this pull request
Apr 24, 2023
This was reported by a user, we store execution step results in the cache also when no diff was generated (rightfully), but then we should only apply the patch when there is actually anything to do. Also, I initially couldn't reproduce this locally because we swallowed this error in volume mode. It would only happen in bind mode.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
A user reported this, we store execution step results in the cache also when no diff was generated (rightfully), but then we should only apply the patch when there is actually anything to do.
Also, I initially couldn't reproduce this locally because we swallowed this error in volume mode. It would only happen in bind mode.
Closes #755
Test plan
Verified this does properly fail in volume mode now, too and don't fail anymore at all.