Skip to content

Commit

Permalink
fix: safer re-merging with updated upstream (runatlantis#3499)
Browse files Browse the repository at this point in the history
* Safer handling of merging with an updated upstream.

We used to call forceClone() to update with upstream, but this deletes the
checked out directory. This is inefficient, can delete existing plan files,
and is very surprising if you are working manually in the working directory.

We now fetch an updated upstream, and re-do the merge operation. This
leaves any working files intact.

* Rename SafeToReClone -> CheckForUpstreamChanges

It's never safe to clone again. But sometimes we need to check
for upstream changes to avoid reverting changes.

The flag is now used to know when we need to merge
again non-destructively with new changes.

* Update fixtures.go

* Add test to make sure plans are not wiped out

As long as the branch itself has not been updated, plans
should be kept. Even if upstream has changed.

* renamed HasDiverged to MergedAgain in PlanResult and from Clone()

This flag was only set to true in case a call to Clone()
ended up merging with an updated upstream, so the
new name better represents what it means.

* Test that Clone on branch update wipes old plans

This complements the test that Clone with unmodified
branch but modified upstream does _not_ wipe plans.

* runGit now runs git instead of returning a function that runs git

* Updated template to merged again instead of diverged

This is no longer a warning, but expected behavior in merge chekout mode

* Rename git wrapper to wrappedGit, add a type for static config

Every call to wrappedGit for the same PR uses identical setup
for directory, head repo and PR, so passing the

---------

Co-authored-by: nitrocode <7775707+nitrocode@users.noreply.github.com>
Co-authored-by: PePe Amengual <jose.amengual@gmail.com>
  • Loading branch information
3 people authored and ijames-gc committed Feb 13, 2024
1 parent b54ec48 commit c1ee37e
Show file tree
Hide file tree
Showing 11 changed files with 173 additions and 120 deletions.
8 changes: 4 additions & 4 deletions server/events/markdown_renderer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ $$$
LockURL: "lock-url",
RePlanCmd: "atlantis plan -d path -w workspace",
ApplyCmd: "atlantis apply -d path -w workspace",
HasDiverged: true,
MergedAgain: true,
},
Workspace: "workspace",
RepoRelDir: "path",
Expand All @@ -210,7 +210,7 @@ $$$
* :repeat: To **plan** this project again, comment:
* $atlantis plan -d path -w workspace$
:warning: The branch we're merging into is ahead, it is recommended to pull new commits first.
:twisted_rightwards_arrows: Upstream was modified, a new merge was performed.
---
* :fast_forward: To **apply** all unapplied plans from this pull request, comment:
Expand Down Expand Up @@ -1974,7 +1974,7 @@ $$$
LockURL: "lock-url",
RePlanCmd: "atlantis plan -d path -w workspace",
ApplyCmd: "atlantis apply -d path -w workspace",
HasDiverged: true,
MergedAgain: true,
},
Workspace: "workspace",
RepoRelDir: "path",
Expand All @@ -1992,7 +1992,7 @@ $$$
* :repeat: To **plan** this project again, comment:
* $atlantis plan -d path -w workspace$
:warning: The branch we're merging into is ahead, it is recommended to pull new commits first.
:twisted_rightwards_arrows: Upstream was modified, a new merge was performed.
---
* :fast_forward: To **apply** all unapplied plans from this pull request, comment:
Expand Down
16 changes: 8 additions & 8 deletions server/events/mock_workingdir_test.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

16 changes: 8 additions & 8 deletions server/events/mocks/mock_working_dir.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 4 additions & 4 deletions server/events/models/models.go
Original file line number Diff line number Diff line change
Expand Up @@ -361,10 +361,10 @@ type PlanSuccess struct {
RePlanCmd string
// ApplyCmd is the command that users should run to apply this plan.
ApplyCmd string
// HasDiverged is true if we're using the checkout merge strategy and the
// branch we're merging into has been updated since we cloned and merged
// it.
HasDiverged bool
// MergedAgain is true if we're using the checkout merge strategy and the
// branch we're merging into had been updated, and we had to merge again
// before planning
MergedAgain bool
}

type PolicySetResult struct {
Expand Down
6 changes: 3 additions & 3 deletions server/events/project_command_runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -543,9 +543,9 @@ func (p *DefaultProjectCommandRunner) doPlan(ctx command.ProjectContext) (*model
}
defer unlockFn()

p.WorkingDir.SetSafeToReClone()
p.WorkingDir.SetCheckForUpstreamChanges()
// Clone is idempotent so okay to run even if the repo was already cloned.
repoDir, hasDiverged, cloneErr := p.WorkingDir.Clone(ctx.HeadRepo, ctx.Pull, ctx.Workspace)
repoDir, mergedAgain, cloneErr := p.WorkingDir.Clone(ctx.HeadRepo, ctx.Pull, ctx.Workspace)
if cloneErr != nil {
if unlockErr := lockAttempt.UnlockFn(); unlockErr != nil {
ctx.Log.Err("error unlocking state after plan error: %v", unlockErr)
Expand Down Expand Up @@ -576,7 +576,7 @@ func (p *DefaultProjectCommandRunner) doPlan(ctx command.ProjectContext) (*model
TerraformOutput: strings.Join(outputs, "\n"),
RePlanCmd: ctx.RePlanCmd,
ApplyCmd: ctx.ApplyCmd,
HasDiverged: hasDiverged,
MergedAgain: mergedAgain,
}, "", nil
}

Expand Down
5 changes: 0 additions & 5 deletions server/events/templates/diverged.tmpl

This file was deleted.

5 changes: 5 additions & 0 deletions server/events/templates/merged_again.tmpl
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{{ define "mergedAgain" -}}
{{ if .MergedAgain }}
:twisted_rightwards_arrows: Upstream was modified, a new merge was performed.
{{ end -}}
{{ end -}}
2 changes: 1 addition & 1 deletion server/events/templates/plan_success_unwrapped.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -16,5 +16,5 @@ This plan was not saved because one or more projects failed and automerge requir
* :repeat: To **plan** this project again, comment:
* `{{ .RePlanCmd }}`
{{ end -}}
{{ template "diverged" . }}
{{ template "mergedAgain" . }}
{{ end -}}
2 changes: 1 addition & 1 deletion server/events/templates/plan_success_wrapped.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -20,5 +20,5 @@ This plan was not saved because one or more projects failed and automerge requir
{{ end -}}
</details>
{{ .PlanSummary -}}
{{ template "diverged" . -}}
{{ template "mergedAgain" . -}}
{{ end -}}
Loading

0 comments on commit c1ee37e

Please sign in to comment.