Skip to content
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

fix(checkout-strategy): On merge strategy, reclone when necessary on base update #3187

Merged
merged 8 commits into from
Mar 25, 2023
Merged

fix(checkout-strategy): On merge strategy, reclone when necessary on base update #3187

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2bb1442
merge again if base branch has been updated
finnag Mar 3, 2023
66a6771
Remove diverged test that no longer applies
finnag Mar 3, 2023
3bdbf25
Merge branch 'main' into reclone-on-merge-base-update
jamengual Mar 3, 2023
7fad72e
Merge branch 'main' into reclone-on-merge-base-update
jamengual Mar 11, 2023
e40cc54
Merge branch 'main' into reclone-on-merge-base-update
jamengual Mar 20, 2023
2094383
Merge branch 'main' into reclone-on-merge-base-update
jamengual Mar 23, 2023
f4c0f2c
Reinstate TestClone_MasterHasDiverged test and make it pass
finnag Mar 24, 2023
50b3388
Extend TestClone_MasterHasDiverged to test new merging functionality
finnag Mar 24, 2023
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
30 changes: 14 additions & 16 deletions server/events/working_dir.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,26 +117,30 @@ func (w *FileWorkspace) Clone(
// We're prefix matching here because BitBucket doesn't give us the full
// commit, only a 12 character prefix.
if strings.HasPrefix(currCommit, p.HeadCommit) {
log.Debug("repo is at correct commit %q so will not re-clone", p.HeadCommit)
return cloneDir, w.warnDiverged(log, p, headRepo, cloneDir), nil
if w.CheckoutMerge && w.recheckDiverged(log, p, headRepo, cloneDir) {
log.Info("base branch has been updated, using merge strategy and will clone again")
} else {
log.Debug("repo is at correct commit %q so will not re-clone", p.HeadCommit)
return cloneDir, false, nil
finnag marked this conversation as resolved.
Show resolved Hide resolved
}
} else {
log.Debug("repo was already cloned but is not at correct commit, wanted %q got %q", p.HeadCommit, currCommit)
}

log.Debug("repo was already cloned but is not at correct commit, wanted %q got %q", p.HeadCommit, currCommit)
// We'll fall through to re-clone.
}

// Otherwise we clone the repo.
return cloneDir, false, w.forceClone(log, cloneDir, headRepo, p)
}

// warnDiverged returns true if we should warn the user that the branch we're
// merging into has diverged from what we currently have checked out.
// recheckDiverged returns true if the branch we're merging into has diverged
// from what we currently have checked out.
// This matters in the case of the merge checkout strategy because after
// cloning the repo and doing the merge, it's possible main was updated.
// Then users won't be getting the merge functionality they expected.
// cloning the repo and doing the merge, it's possible main was updated
// and we have to perform a new merge.
// If there are any errors we return false since we prefer things to succeed
// vs. stopping the plan/apply.
func (w *FileWorkspace) warnDiverged(log logging.SimpleLogging, p models.PullRequest, headRepo models.Repo, cloneDir string) bool {
func (w *FileWorkspace) recheckDiverged(log logging.SimpleLogging, p models.PullRequest, headRepo models.Repo, cloneDir string) bool {
if !w.CheckoutMerge {
// It only makes sense to warn that main has diverged if we're using
// the checkout merge strategy. If we're just checking out the branch,
Expand Down Expand Up @@ -174,13 +178,7 @@ func (w *FileWorkspace) warnDiverged(log logging.SimpleLogging, p models.PullReq
}
}

hasDiverged := w.HasDiverged(log, cloneDir)
if hasDiverged {
log.Info("remote main branch is ahead and thereby has new commits, it is recommended to pull new commits")
} else {
log.Debug("remote main branch has no new commits")
}
return hasDiverged
return w.HasDiverged(log, cloneDir)
}

func (w *FileWorkspace) HasDiverged(log logging.SimpleLogging, cloneDir string) bool {
Expand Down
76 changes: 0 additions & 76 deletions server/events/working_dir_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -411,82 +411,6 @@ func TestClone_RecloneWrongCommit(t *testing.T) {
Equals(t, expCommit, actCommit)
}

// Test that if the branch we're merging into has diverged and we're using
// checkout-strategy=merge, we warn the user (see #804).
func TestClone_MasterHasDiverged(t *testing.T) {
// Initialize the git repo.
repoDir := initRepo(t)

// Simulate first PR.
runCmd(t, repoDir, "git", "checkout", "-b", "first-pr")
runCmd(t, repoDir, "touch", "file1")
runCmd(t, repoDir, "git", "add", "file1")
runCmd(t, repoDir, "git", "commit", "-m", "file1")

// Atlantis checkout first PR.
firstPRDir := repoDir + "/first-pr"
runCmd(t, repoDir, "mkdir", "-p", "first-pr")
runCmd(t, firstPRDir, "git", "clone", "--branch", "main", "--single-branch", repoDir, ".")
runCmd(t, firstPRDir, "git", "remote", "add", "head", repoDir)
runCmd(t, firstPRDir, "git", "fetch", "head", "+refs/heads/first-pr")
runCmd(t, firstPRDir, "git", "config", "--local", "user.email", "[email protected]")
runCmd(t, firstPRDir, "git", "config", "--local", "user.name", "atlantisbot")
runCmd(t, firstPRDir, "git", "config", "--local", "commit.gpgsign", "false")
runCmd(t, firstPRDir, "git", "merge", "-q", "--no-ff", "-m", "atlantis-merge", "FETCH_HEAD")

// Simulate second PR.
runCmd(t, repoDir, "git", "checkout", "main")
runCmd(t, repoDir, "git", "checkout", "-b", "second-pr")
runCmd(t, repoDir, "touch", "file2")
runCmd(t, repoDir, "git", "add", "file2")
runCmd(t, repoDir, "git", "commit", "-m", "file2")

// Atlantis checkout second PR.
secondPRDir := repoDir + "/second-pr"
runCmd(t, repoDir, "mkdir", "-p", "second-pr")
runCmd(t, secondPRDir, "git", "clone", "--branch", "main", "--single-branch", repoDir, ".")
runCmd(t, secondPRDir, "git", "remote", "add", "head", repoDir)
runCmd(t, secondPRDir, "git", "fetch", "head", "+refs/heads/second-pr")
runCmd(t, secondPRDir, "git", "config", "--local", "user.email", "[email protected]")
runCmd(t, secondPRDir, "git", "config", "--local", "user.name", "atlantisbot")
runCmd(t, secondPRDir, "git", "config", "--local", "commit.gpgsign", "false")
runCmd(t, secondPRDir, "git", "merge", "-q", "--no-ff", "-m", "atlantis-merge", "FETCH_HEAD")

// Merge first PR
runCmd(t, repoDir, "git", "checkout", "main")
runCmd(t, repoDir, "git", "merge", "first-pr")

// Copy the second-pr repo to our data dir which has diverged remote main
runCmd(t, repoDir, "mkdir", "-p", "repos/0/")
runCmd(t, repoDir, "cp", "-R", secondPRDir, "repos/0/default")

// Run the clone.
wd := &events.FileWorkspace{
DataDir: repoDir,
CheckoutMerge: true,
CheckoutDepth: 50,
GpgNoSigningEnabled: true,
}
_, hasDiverged, err := wd.Clone(logging.NewNoopLogger(t), models.Repo{CloneURL: repoDir}, models.PullRequest{
BaseRepo: models.Repo{CloneURL: repoDir},
HeadBranch: "second-pr",
BaseBranch: "main",
}, "default")
Ok(t, err)
Equals(t, hasDiverged, true)

// Run it again but without the checkout merge strategy. It should return
// false.
wd.CheckoutMerge = false
_, hasDiverged, err = wd.Clone(logging.NewNoopLogger(t), models.Repo{}, models.PullRequest{
BaseRepo: models.Repo{},
HeadBranch: "second-pr",
BaseBranch: "main",
}, "default")
Ok(t, err)
Equals(t, hasDiverged, false)
}

func TestHasDiverged_MasterHasDiverged(t *testing.T) {
// Initialize the git repo.
repoDir := initRepo(t)
Expand Down