Skip to content

Commit

Permalink
Only apply cache result progress if resulted in diff (#778)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
eseliger authored Jun 14, 2022
1 parent 75fa7f1 commit 181e3de
Show file tree
Hide file tree
Showing 4 changed files with 17 additions and 7 deletions.
8 changes: 6 additions & 2 deletions internal/batches/executor/run_steps.go
Original file line number Diff line number Diff line change
Expand Up @@ -121,8 +121,12 @@ func runSteps(ctx context.Context, opts *executionOpts) (result execution.Result
stepContext.Steps.Changes = previousStepResult.Files
stepContext.Outputs = opts.task.CachedResult.Outputs

if err := ws.ApplyDiff(ctx, []byte(opts.task.CachedResult.Diff)); err != nil {
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 != "" {
if err := ws.ApplyDiff(ctx, []byte(opts.task.CachedResult.Diff)); err != nil {
return execResult, nil, errors.Wrap(err, "getting changed files in step")
}
}
}

Expand Down
4 changes: 2 additions & 2 deletions internal/batches/workspace/bind_workspace.go
Original file line number Diff line number Diff line change
Expand Up @@ -172,8 +172,8 @@ func (w *dockerBindWorkspace) ApplyDiff(ctx context.Context, diff []byte) error
}

// Apply diff
if _, err = runGitCmd(ctx, w.dir, "apply", "-p0", tmp.Name()); err != nil {
return errors.Wrap(err, "applying cached diff")
if out, err := runGitCmd(ctx, w.dir, "apply", "-p0", tmp.Name()); err != nil {
return errors.Wrapf(err, "applying cached diff: %s", string(out))
}

// Add all files to index
Expand Down
2 changes: 1 addition & 1 deletion internal/batches/workspace/git.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ func runGitCmd(ctx context.Context, dir string, args ...string) ([]byte, error)
cmd.Dir = dir
out, err := cmd.CombinedOutput()
if err != nil {
return nil, errors.Wrapf(err, "'git %s' failed: %s", strings.Join(args, " "), out)
return out, errors.Wrapf(err, "'git %s' failed: %s", strings.Join(args, " "), out)
}
return out, nil
}
10 changes: 8 additions & 2 deletions internal/batches/workspace/volume_workspace.go
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,8 @@ exec git diff --cached --no-prefix --binary
func (w *dockerVolumeWorkspace) ApplyDiff(ctx context.Context, diff []byte) error {
script := fmt.Sprintf(`#!/bin/sh
set -e
cat <<'EOF' | exec git apply -p0 -
%s
EOF
Expand Down Expand Up @@ -322,11 +324,15 @@ func (w *dockerVolumeWorkspace) runScript(ctx context.Context, target, script st
if _, err := f.WriteString(script); err != nil {
return nil, errors.Wrap(err, "writing run script")
}
f.Close()
if err := f.Close(); err != nil {
return nil, errors.Wrap(err, "closing run script")
}

// Sidestep any umask issues on the temporary file by always making it
// executable by everyone.
os.Chmod(name, 0755)
if err := os.Chmod(name, 0755); err != nil {
return nil, errors.Wrap(err, "chmodding run script")
}

common, err := w.DockerRunOpts(ctx, target)
if err != nil {
Expand Down

0 comments on commit 181e3de

Please sign in to comment.