From 22bd2b785894aa6f390a267d667f5c6035b5ff3a Mon Sep 17 00:00:00 2001 From: Finn Arne Gangstad Date: Mon, 14 Aug 2023 14:07:18 +0200 Subject: [PATCH 1/4] Remove TryLockPull, use TryLock per directory instead We generate a list of all interesting directories, so we can target the locks to the affected directories instead of using a (too) global lock --- .../events/mocks/mock_working_dir_locker.go | 50 ------------- server/events/project_command_builder.go | 14 ++-- server/events/working_dir_locker.go | 73 +++---------------- server/events/working_dir_locker_test.go | 44 ----------- 4 files changed, 16 insertions(+), 165 deletions(-) diff --git a/server/events/mocks/mock_working_dir_locker.go b/server/events/mocks/mock_working_dir_locker.go index 6bb1cde91f..9c3c4302dd 100644 --- a/server/events/mocks/mock_working_dir_locker.go +++ b/server/events/mocks/mock_working_dir_locker.go @@ -43,25 +43,6 @@ func (mock *MockWorkingDirLocker) TryLock(repoFullName string, pullNum int, work return ret0, ret1 } -func (mock *MockWorkingDirLocker) TryLockPull(repoFullName string, pullNum int) (func(), error) { - if mock == nil { - panic("mock must not be nil. Use myMock := NewMockWorkingDirLocker().") - } - params := []pegomock.Param{repoFullName, pullNum} - result := pegomock.GetGenericMockFrom(mock).Invoke("TryLockPull", params, []reflect.Type{reflect.TypeOf((*func())(nil)).Elem(), reflect.TypeOf((*error)(nil)).Elem()}) - var ret0 func() - var ret1 error - if len(result) != 0 { - if result[0] != nil { - ret0 = result[0].(func()) - } - if result[1] != nil { - ret1 = result[1].(error) - } - } - return ret0, ret1 -} - func (mock *MockWorkingDirLocker) VerifyWasCalledOnce() *VerifierMockWorkingDirLocker { return &VerifierMockWorkingDirLocker{ mock: mock, @@ -137,34 +118,3 @@ func (c *MockWorkingDirLocker_TryLock_OngoingVerification) GetAllCapturedArgumen } return } - -func (verifier *VerifierMockWorkingDirLocker) TryLockPull(repoFullName string, pullNum int) *MockWorkingDirLocker_TryLockPull_OngoingVerification { - params := []pegomock.Param{repoFullName, pullNum} - methodInvocations := pegomock.GetGenericMockFrom(verifier.mock).Verify(verifier.inOrderContext, verifier.invocationCountMatcher, "TryLockPull", params, verifier.timeout) - return &MockWorkingDirLocker_TryLockPull_OngoingVerification{mock: verifier.mock, methodInvocations: methodInvocations} -} - -type MockWorkingDirLocker_TryLockPull_OngoingVerification struct { - mock *MockWorkingDirLocker - methodInvocations []pegomock.MethodInvocation -} - -func (c *MockWorkingDirLocker_TryLockPull_OngoingVerification) GetCapturedArguments() (string, int) { - repoFullName, pullNum := c.GetAllCapturedArguments() - return repoFullName[len(repoFullName)-1], pullNum[len(pullNum)-1] -} - -func (c *MockWorkingDirLocker_TryLockPull_OngoingVerification) GetAllCapturedArguments() (_param0 []string, _param1 []int) { - params := pegomock.GetGenericMockFrom(c.mock).GetInvocationParams(c.methodInvocations) - if len(params) > 0 { - _param0 = make([]string, len(c.methodInvocations)) - for u, param := range params[0] { - _param0[u] = param.(string) - } - _param1 = make([]int, len(c.methodInvocations)) - for u, param := range params[1] { - _param1[u] = param.(int) - } - } - return -} diff --git a/server/events/project_command_builder.go b/server/events/project_command_builder.go index 35e540a858..7d587e4ce3 100644 --- a/server/events/project_command_builder.go +++ b/server/events/project_command_builder.go @@ -756,14 +756,6 @@ func (p *DefaultProjectCommandBuilder) getCfg(ctx *command.Context, projectName // buildAllProjectCommandsByPlan builds contexts for a command for every project that has // pending plans in this ctx. func (p *DefaultProjectCommandBuilder) buildAllProjectCommandsByPlan(ctx *command.Context, commentCmd *CommentCommand) ([]command.ProjectContext, error) { - // Lock all dirs in this pull request (instead of a single dir) because we - // don't know how many dirs we'll need to run the command in. - unlockFn, err := p.WorkingDirLocker.TryLockPull(ctx.Pull.BaseRepo.FullName, ctx.Pull.Num) - if err != nil { - return nil, err - } - defer unlockFn() - pullDir, err := p.WorkingDir.GetPullDir(ctx.Pull.BaseRepo, ctx.Pull) if err != nil { return nil, err @@ -783,6 +775,12 @@ func (p *DefaultProjectCommandBuilder) buildAllProjectCommandsByPlan(ctx *comman var cmds []command.ProjectContext for _, plan := range plans { + // Lock all the directories we need to run the command in + unlockFn, err := p.WorkingDirLocker.TryLock(ctx.Pull.BaseRepo.FullName, ctx.Pull.Num, plan.Workspace, plan.RepoRelDir) + if err != nil { + return nil, err + } + defer unlockFn() commentCmds, err := p.buildProjectCommandCtx(ctx, commentCmd.CommandName(), commentCmd.SubName, plan.ProjectName, commentCmd.Flags, defaultRepoDir, plan.RepoRelDir, plan.Workspace, commentCmd.Verbose) if err != nil { return nil, errors.Wrapf(err, "building command for dir '%s'", plan.RepoRelDir) diff --git a/server/events/working_dir_locker.go b/server/events/working_dir_locker.go index 06af44ec5a..901c8fabc1 100644 --- a/server/events/working_dir_locker.go +++ b/server/events/working_dir_locker.go @@ -15,7 +15,6 @@ package events import ( "fmt" - "strings" "sync" ) @@ -32,12 +31,6 @@ type WorkingDirLocker interface { // an error if the workspace is already locked. The error is expected to // be printed to the pull request. TryLock(repoFullName string, pullNum int, workspace string, path string) (func(), error) - // TryLockPull tries to acquire a lock for all the workspaces in this repo - // and pull. - // It returns a function that should be used to unlock the workspace and - // an error if the workspace is already locked. The error is expected to - // be printed to the pull request. - TryLockPull(repoFullName string, pullNum int) (func(), error) } // DefaultWorkingDirLocker implements WorkingDirLocker. @@ -45,49 +38,26 @@ type DefaultWorkingDirLocker struct { // mutex prevents against multiple threads calling functions on this struct // concurrently. It's only used for entry/exit to each function. mutex sync.Mutex - // locks is a list of the keys that are locked. We then use prefix - // matching to determine if something is locked. It's naive but that's okay - // because there won't be many locks at one time. - locks []string + // locks is a set of the keys that are locked. + locks map[string]struct{} } // NewDefaultWorkingDirLocker is a constructor. func NewDefaultWorkingDirLocker() *DefaultWorkingDirLocker { - return &DefaultWorkingDirLocker{} -} - -func (d *DefaultWorkingDirLocker) TryLockPull(repoFullName string, pullNum int) (func(), error) { - d.mutex.Lock() - defer d.mutex.Unlock() - - pullKey := d.pullKey(repoFullName, pullNum) - for _, l := range d.locks { - if l == pullKey || strings.HasPrefix(l, pullKey+"/") { - return func() {}, fmt.Errorf("the Atlantis working dir is currently locked by another" + - " command that is running for this pull request.\n" + - "Wait until the previous command is complete and try again") - } - } - d.locks = append(d.locks, pullKey) - return func() { - d.UnlockPull(repoFullName, pullNum) - }, nil + return &DefaultWorkingDirLocker{locks: make(map[string]struct{})} } func (d *DefaultWorkingDirLocker) TryLock(repoFullName string, pullNum int, workspace string, path string) (func(), error) { d.mutex.Lock() defer d.mutex.Unlock() - pullKey := d.pullKey(repoFullName, pullNum) workspaceKey := d.workspaceKey(repoFullName, pullNum, workspace, path) - for _, l := range d.locks { - if l == pullKey || l == workspaceKey { - return func() {}, fmt.Errorf("the %s workspace at path %s is currently locked by another"+ - " command that is running for this pull request.\n"+ - "Wait until the previous command is complete and try again", workspace, path) - } + if _, exists := d.locks[workspaceKey]; exists { + return func() {}, fmt.Errorf("the %s workspace at path %s is currently locked by another"+ + " command that is running for this pull request.\n"+ + "Wait until the previous command is complete and try again", workspace, path) } - d.locks = append(d.locks, workspaceKey) + d.locks[workspaceKey] = struct{}{} return func() { d.unlock(repoFullName, pullNum, workspace, path) }, nil @@ -99,32 +69,9 @@ func (d *DefaultWorkingDirLocker) unlock(repoFullName string, pullNum int, works defer d.mutex.Unlock() workspaceKey := d.workspaceKey(repoFullName, pullNum, workspace, path) - d.removeLock(workspaceKey) -} - -// Unlock unlocks all workspaces for this pull. -func (d *DefaultWorkingDirLocker) UnlockPull(repoFullName string, pullNum int) { - d.mutex.Lock() - defer d.mutex.Unlock() - - pullKey := d.pullKey(repoFullName, pullNum) - d.removeLock(pullKey) -} - -func (d *DefaultWorkingDirLocker) removeLock(key string) { - var newLocks []string - for _, l := range d.locks { - if l != key { - newLocks = append(newLocks, l) - } - } - d.locks = newLocks + delete(d.locks, workspaceKey) } func (d *DefaultWorkingDirLocker) workspaceKey(repo string, pull int, workspace string, path string) string { - return fmt.Sprintf("%s/%s/%s", d.pullKey(repo, pull), workspace, path) -} - -func (d *DefaultWorkingDirLocker) pullKey(repo string, pull int) string { - return fmt.Sprintf("%s/%d", repo, pull) + return fmt.Sprintf("%s/%d/%s/%s", repo, pull, workspace, path) } diff --git a/server/events/working_dir_locker_test.go b/server/events/working_dir_locker_test.go index 11c21ea151..36ce0dc948 100644 --- a/server/events/working_dir_locker_test.go +++ b/server/events/working_dir_locker_test.go @@ -170,47 +170,3 @@ func TestUnlockDifferentPulls(t *testing.T) { _, err = locker.TryLock(repo, newPull, workspace, path) Ok(t, err) } - -func TestLockPull(t *testing.T) { - locker := events.NewDefaultWorkingDirLocker() - unlock, err := locker.TryLockPull("owner/repo", 1) - Ok(t, err) - - // Now a lock for the same pull or for a workspace should fail. - _, err = locker.TryLockPull("owner/repo", 1) - Assert(t, err != nil, "exp err") - _, err = locker.TryLock("owner/repo", 1, "workspace", path) - Assert(t, err != nil, "exp err") - - // Lock for a different pull and workspace should succeed. - _, err = locker.TryLockPull("owner/repo", 2) - Ok(t, err) - _, err = locker.TryLock("owner/repo", 3, "workspace", path) - Ok(t, err) - - // After unlocking, should be able to get a pull lock. - unlock() - unlock, err = locker.TryLockPull("owner/repo", 1) - Ok(t, err) - - // If we unlock that too, should be able to get the workspace lock. - unlock() - _, err = locker.TryLock("owner/repo", 1, "workspace", path) - Ok(t, err) - unlock() -} - -// If the workspace was locked first, we shouldn't be able to get the pull lock. -func TestLockPull_WorkspaceFirst(t *testing.T) { - locker := events.NewDefaultWorkingDirLocker() - unlock, err := locker.TryLock("owner/repo", 1, "workspace", path) - Ok(t, err) - - _, err = locker.TryLockPull("owner/repo", 1) - Assert(t, err != nil, "exp err") - - // After unlocking the workspace, should be able to get the lock. - unlock() - _, err = locker.TryLockPull("owner/repo", 1) - Ok(t, err) -} From 0cb7f493e87d26e98afa98c82b138499c70d7528 Mon Sep 17 00:00:00 2001 From: Finn Arne Gangstad Date: Mon, 14 Aug 2023 15:56:58 +0200 Subject: [PATCH 2/4] Extend clone lock so it covers outside the "are we current" test There is a race condition here where we test if we are current, and only then if we are not current we grab the lock. In the meantime, that information could be stale. Extend the lock to cover all operations, and unconditionally wait for the lock. We can't assume anything can be skipped if we have to wait for the lock. --- server/events/working_dir.go | 25 +++++++------------------ 1 file changed, 7 insertions(+), 18 deletions(-) diff --git a/server/events/working_dir.go b/server/events/working_dir.go index c2e56d8dc7..9937cf308c 100644 --- a/server/events/working_dir.go +++ b/server/events/working_dir.go @@ -97,6 +97,13 @@ type FileWorkspace struct { // multiple dirs of the same repo without deleting existing plans. func (w *FileWorkspace) Clone(logger logging.SimpleLogging, headRepo models.Repo, p models.PullRequest, workspace string) (string, bool, error) { cloneDir := w.cloneDir(p.BaseRepo, p, workspace) + + // Unconditionally wait for the clone lock here, if anyone else is doing any clone + // operation in this directory, we wait for it to finish before we check anything. + value, _ := cloneLocks.LoadOrStore(cloneDir, new(sync.Mutex)) + mutex := value.(*sync.Mutex) + mutex.Lock() + defer mutex.Unlock() defer func() { w.CheckForUpstreamChanges = false }() c := wrappedGitContext{cloneDir, headRepo, p} @@ -217,15 +224,6 @@ func (w *FileWorkspace) HasDiverged(logger logging.SimpleLogging, cloneDir strin } func (w *FileWorkspace) forceClone(logger logging.SimpleLogging, c wrappedGitContext) error { - value, _ := cloneLocks.LoadOrStore(c.dir, new(sync.Mutex)) - mutex := value.(*sync.Mutex) - - defer mutex.Unlock() - if locked := mutex.TryLock(); !locked { - mutex.Lock() - return nil - } - err := os.RemoveAll(c.dir) if err != nil { return errors.Wrapf(err, "deleting dir '%s' before cloning", c.dir) @@ -280,15 +278,6 @@ func (w *FileWorkspace) forceClone(logger logging.SimpleLogging, c wrappedGitCon // There is a new upstream update that we need, and we want to update to it // without deleting any existing plans func (w *FileWorkspace) mergeAgain(logger logging.SimpleLogging, c wrappedGitContext) error { - value, _ := cloneLocks.LoadOrStore(c.dir, new(sync.Mutex)) - mutex := value.(*sync.Mutex) - - defer mutex.Unlock() - if locked := mutex.TryLock(); !locked { - mutex.Lock() - return nil - } - // Reset branch as if it was cloned again if err := w.wrappedGit(logger, c, "reset", "--hard", fmt.Sprintf("refs/remotes/origin/%s", c.pr.BaseBranch)); err != nil { return err From 86994de55b09227859b927e415a6ac0035cb81f6 Mon Sep 17 00:00:00 2001 From: Finn Arne Gangstad Date: Tue, 15 Aug 2023 15:11:38 +0200 Subject: [PATCH 3/4] Reduce the number of git calls in parallel mode when merging All Clone() calls that have signaled an interest in merging before another Clone() checks whether a merge is necessary can skip their own checks. This should reduce the thundering herd problem at the beginning of large paralell runs. --- server/events/working_dir.go | 33 ++++++++++++++++++++++++++++----- 1 file changed, 28 insertions(+), 5 deletions(-) diff --git a/server/events/working_dir.go b/server/events/working_dir.go index 9937cf308c..392e561aa0 100644 --- a/server/events/working_dir.go +++ b/server/events/working_dir.go @@ -32,6 +32,7 @@ import ( const workingDirPrefix = "repos" var cloneLocks sync.Map +var recheckRequiredMap sync.Map //go:generate pegomock generate github.com/runatlantis/atlantis/server/events --package mocks -o mocks/mock_working_dir.go WorkingDir //go:generate pegomock generate github.com/runatlantis/atlantis/server/events --package events WorkingDir @@ -98,13 +99,29 @@ type FileWorkspace struct { func (w *FileWorkspace) Clone(logger logging.SimpleLogging, headRepo models.Repo, p models.PullRequest, workspace string) (string, bool, error) { cloneDir := w.cloneDir(p.BaseRepo, p, workspace) + // Disable the mustRecheckUpstream flag if we are not using the merge checkout strategy + mustRecheckUpstream := w.CheckForUpstreamChanges && w.CheckoutMerge + + if mustRecheckUpstream { + // We atomically set the recheckRequiredMap flag here before grabbing the clone lock. + // If the flag is cleared after we grab the lock, it means some other thread + // did the necessary work late enough that we do not have to do it again. + recheckRequiredMap.Store(cloneDir, struct{}{}) + } + // Unconditionally wait for the clone lock here, if anyone else is doing any clone // operation in this directory, we wait for it to finish before we check anything. value, _ := cloneLocks.LoadOrStore(cloneDir, new(sync.Mutex)) mutex := value.(*sync.Mutex) mutex.Lock() defer mutex.Unlock() - defer func() { w.CheckForUpstreamChanges = false }() + + if mustRecheckUpstream { + if _, exists := recheckRequiredMap.Load(cloneDir); !exists { + mustRecheckUpstream = false + w.Logger.Debug("Skipping upstream check. Some other thread has done this for us") + } + } c := wrappedGitContext{cloneDir, headRepo, p} // If the directory already exists, check if it's at the right commit. @@ -133,11 +150,17 @@ func (w *FileWorkspace) Clone(logger logging.SimpleLogging, headRepo models.Repo // 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) { - if w.CheckForUpstreamChanges && w.CheckoutMerge && w.recheckDiverged(logger, p, headRepo, cloneDir) { - logger.Info("base branch has been updated, using merge strategy and will clone again") - return cloneDir, true, w.mergeAgain(logger, c) + if mustRecheckUpstream { + w.CheckForUpstreamChanges = false + recheckRequiredMap.Delete(cloneDir) + if w.recheckDiverged(p, headRepo, cloneDir) { + w.Logger.Info("base branch has been updated, using merge strategy and will merge again") + return cloneDir, true, w.mergeAgain(logger, c) + } + w.Logger.Debug("repo is at correct commit %q and there are no upstream changes", p.HeadCommit) + } else { + w.Logger.Debug("repo is at correct commit %q so will not re-clone", p.HeadCommit) } - logger.Debug("repo is at correct commit '%s' so will not re-clone", p.HeadCommit) return cloneDir, false, nil } else { logger.Debug("repo was already cloned but is not at correct commit, wanted '%s' got '%s'", p.HeadCommit, currCommit) From 54e44b3d7c073604f0247581eb55c9107650dd97 Mon Sep 17 00:00:00 2001 From: Finn Arne Gangstad Date: Wed, 27 Sep 2023 14:52:24 +0200 Subject: [PATCH 4/4] refactor: split Clone into Clone and MergeAgain Clone is now a NOP if the PR has not changed, and loses its second return value, the MergedAgain flag. MergeAgain must be called explicitly in the only location that cares about this flag, just before planning. This cleans up the code for Clone and re-merging a bit. Also regenerated mocks --- server/events/github_app_working_dir.go | 2 +- server/events/github_app_working_dir_test.go | 8 +- server/events/mock_workingdir_test.go | 67 +++++++++---- server/events/mocks/mock_working_dir.go | 67 +++++++++---- .../post_workflow_hooks_command_runner.go | 2 +- ...post_workflow_hooks_command_runner_test.go | 20 ++-- .../pre_workflow_hooks_command_runner.go | 2 +- .../pre_workflow_hooks_command_runner_test.go | 20 ++-- server/events/project_command_builder.go | 6 +- .../project_command_builder_internal_test.go | 10 +- server/events/project_command_builder_test.go | 20 ++-- server/events/project_command_runner.go | 17 +++- server/events/project_command_runner_test.go | 7 +- server/events/working_dir.go | 96 ++++++++++--------- server/events/working_dir_test.go | 51 +++++----- 15 files changed, 232 insertions(+), 163 deletions(-) diff --git a/server/events/github_app_working_dir.go b/server/events/github_app_working_dir.go index a06599efe0..5852a94e9c 100644 --- a/server/events/github_app_working_dir.go +++ b/server/events/github_app_working_dir.go @@ -20,7 +20,7 @@ type GithubAppWorkingDir struct { } // Clone writes a fresh token for Github App authentication -func (g *GithubAppWorkingDir) Clone(logger logging.SimpleLogging, headRepo models.Repo, p models.PullRequest, workspace string) (string, bool, error) { +func (g *GithubAppWorkingDir) Clone(logger logging.SimpleLogging, headRepo models.Repo, p models.PullRequest, workspace string) (string, error) { baseRepo := &p.BaseRepo // Realistically, this is a super brittle way of supporting clones using gh app installation tokens diff --git a/server/events/github_app_working_dir_test.go b/server/events/github_app_working_dir_test.go index 78e64d4e0b..900b1f6193 100644 --- a/server/events/github_app_working_dir_test.go +++ b/server/events/github_app_working_dir_test.go @@ -45,7 +45,7 @@ func TestClone_GithubAppNoneExisting(t *testing.T) { GithubHostname: testServer, } - cloneDir, _, err := gwd.Clone(logger, models.Repo{}, models.PullRequest{ + cloneDir, err := gwd.Clone(logger, models.Repo{}, models.PullRequest{ BaseRepo: models.Repo{}, HeadBranch: "branch", }, "default") @@ -90,11 +90,11 @@ func TestClone_GithubAppSetsCorrectUrl(t *testing.T) { When(credentials.GetToken()).ThenReturn("token", nil) When(workingDir.Clone(Any[logging.SimpleLogging](), Eq(modifiedBaseRepo), Eq(models.PullRequest{BaseRepo: modifiedBaseRepo}), - Eq("default"))).ThenReturn("", true, nil) + Eq("default"))).ThenReturn("", nil) - _, success, _ := ghAppWorkingDir.Clone(logger, headRepo, models.PullRequest{BaseRepo: baseRepo}, "default") + _, err := ghAppWorkingDir.Clone(logger, headRepo, models.PullRequest{BaseRepo: baseRepo}, "default") workingDir.VerifyWasCalledOnce().Clone(logger, modifiedBaseRepo, models.PullRequest{BaseRepo: modifiedBaseRepo}, "default") - Assert(t, success == true, "clone url mutation error") + Ok(t, err) } diff --git a/server/events/mock_workingdir_test.go b/server/events/mock_workingdir_test.go index d298b2cee7..f5ead7a509 100644 --- a/server/events/mock_workingdir_test.go +++ b/server/events/mock_workingdir_test.go @@ -27,27 +27,23 @@ func NewMockWorkingDir(options ...pegomock.Option) *MockWorkingDir { func (mock *MockWorkingDir) SetFailHandler(fh pegomock.FailHandler) { mock.fail = fh } func (mock *MockWorkingDir) FailHandler() pegomock.FailHandler { return mock.fail } -func (mock *MockWorkingDir) Clone(logger logging.SimpleLogging, headRepo models.Repo, p models.PullRequest, workspace string) (string, bool, error) { +func (mock *MockWorkingDir) Clone(logger logging.SimpleLogging, headRepo models.Repo, p models.PullRequest, workspace string) (string, error) { if mock == nil { panic("mock must not be nil. Use myMock := NewMockWorkingDir().") } params := []pegomock.Param{logger, headRepo, p, workspace} - result := pegomock.GetGenericMockFrom(mock).Invoke("Clone", params, []reflect.Type{reflect.TypeOf((*string)(nil)).Elem(), reflect.TypeOf((*bool)(nil)).Elem(), reflect.TypeOf((*error)(nil)).Elem()}) + result := pegomock.GetGenericMockFrom(mock).Invoke("Clone", params, []reflect.Type{reflect.TypeOf((*string)(nil)).Elem(), reflect.TypeOf((*error)(nil)).Elem()}) var ret0 string - var ret1 bool - var ret2 error + var ret1 error if len(result) != 0 { if result[0] != nil { ret0 = result[0].(string) } if result[1] != nil { - ret1 = result[1].(bool) - } - if result[2] != nil { - ret2 = result[2].(error) + ret1 = result[1].(error) } } - return ret0, ret1, ret2 + return ret0, ret1 } func (mock *MockWorkingDir) Delete(logger logging.SimpleLogging, r models.Repo, p models.PullRequest) error { @@ -167,12 +163,23 @@ func (mock *MockWorkingDir) HasDiverged(logger logging.SimpleLogging, cloneDir s return ret0 } -func (mock *MockWorkingDir) SetCheckForUpstreamChanges() { +func (mock *MockWorkingDir) MergeAgain(logger logging.SimpleLogging, headRepo models.Repo, p models.PullRequest, workspace string) (bool, error) { if mock == nil { panic("mock must not be nil. Use myMock := NewMockWorkingDir().") } - params := []pegomock.Param{} - pegomock.GetGenericMockFrom(mock).Invoke("SetCheckForUpstreamChanges", params, []reflect.Type{}) + params := []pegomock.Param{logger, headRepo, p, workspace} + result := pegomock.GetGenericMockFrom(mock).Invoke("MergeAgain", params, []reflect.Type{reflect.TypeOf((*bool)(nil)).Elem(), reflect.TypeOf((*error)(nil)).Elem()}) + var ret0 bool + var ret1 error + if len(result) != 0 { + if result[0] != nil { + ret0 = result[0].(bool) + } + if result[1] != nil { + ret1 = result[1].(error) + } + } + return ret0, ret1 } func (mock *MockWorkingDir) VerifyWasCalledOnce() *VerifierMockWorkingDir { @@ -508,19 +515,41 @@ func (c *MockWorkingDir_HasDiverged_OngoingVerification) GetAllCapturedArguments return } -func (verifier *VerifierMockWorkingDir) SetCheckForUpstreamChanges() *MockWorkingDir_SetCheckForUpstreamChanges_OngoingVerification { - params := []pegomock.Param{} - methodInvocations := pegomock.GetGenericMockFrom(verifier.mock).Verify(verifier.inOrderContext, verifier.invocationCountMatcher, "SetCheckForUpstreamChanges", params, verifier.timeout) - return &MockWorkingDir_SetCheckForUpstreamChanges_OngoingVerification{mock: verifier.mock, methodInvocations: methodInvocations} +func (verifier *VerifierMockWorkingDir) MergeAgain(logger logging.SimpleLogging, headRepo models.Repo, p models.PullRequest, workspace string) *MockWorkingDir_MergeAgain_OngoingVerification { + params := []pegomock.Param{logger, headRepo, p, workspace} + methodInvocations := pegomock.GetGenericMockFrom(verifier.mock).Verify(verifier.inOrderContext, verifier.invocationCountMatcher, "MergeAgain", params, verifier.timeout) + return &MockWorkingDir_MergeAgain_OngoingVerification{mock: verifier.mock, methodInvocations: methodInvocations} } -type MockWorkingDir_SetCheckForUpstreamChanges_OngoingVerification struct { +type MockWorkingDir_MergeAgain_OngoingVerification struct { mock *MockWorkingDir methodInvocations []pegomock.MethodInvocation } -func (c *MockWorkingDir_SetCheckForUpstreamChanges_OngoingVerification) GetCapturedArguments() { +func (c *MockWorkingDir_MergeAgain_OngoingVerification) GetCapturedArguments() (logging.SimpleLogging, models.Repo, models.PullRequest, string) { + logger, headRepo, p, workspace := c.GetAllCapturedArguments() + return logger[len(logger)-1], headRepo[len(headRepo)-1], p[len(p)-1], workspace[len(workspace)-1] } -func (c *MockWorkingDir_SetCheckForUpstreamChanges_OngoingVerification) GetAllCapturedArguments() { +func (c *MockWorkingDir_MergeAgain_OngoingVerification) GetAllCapturedArguments() (_param0 []logging.SimpleLogging, _param1 []models.Repo, _param2 []models.PullRequest, _param3 []string) { + params := pegomock.GetGenericMockFrom(c.mock).GetInvocationParams(c.methodInvocations) + if len(params) > 0 { + _param0 = make([]logging.SimpleLogging, len(c.methodInvocations)) + for u, param := range params[0] { + _param0[u] = param.(logging.SimpleLogging) + } + _param1 = make([]models.Repo, len(c.methodInvocations)) + for u, param := range params[1] { + _param1[u] = param.(models.Repo) + } + _param2 = make([]models.PullRequest, len(c.methodInvocations)) + for u, param := range params[2] { + _param2[u] = param.(models.PullRequest) + } + _param3 = make([]string, len(c.methodInvocations)) + for u, param := range params[3] { + _param3[u] = param.(string) + } + } + return } diff --git a/server/events/mocks/mock_working_dir.go b/server/events/mocks/mock_working_dir.go index 9c162fc4a2..63b00805ee 100644 --- a/server/events/mocks/mock_working_dir.go +++ b/server/events/mocks/mock_working_dir.go @@ -26,27 +26,23 @@ func NewMockWorkingDir(options ...pegomock.Option) *MockWorkingDir { func (mock *MockWorkingDir) SetFailHandler(fh pegomock.FailHandler) { mock.fail = fh } func (mock *MockWorkingDir) FailHandler() pegomock.FailHandler { return mock.fail } -func (mock *MockWorkingDir) Clone(logger logging.SimpleLogging, headRepo models.Repo, p models.PullRequest, workspace string) (string, bool, error) { +func (mock *MockWorkingDir) Clone(logger logging.SimpleLogging, headRepo models.Repo, p models.PullRequest, workspace string) (string, error) { if mock == nil { panic("mock must not be nil. Use myMock := NewMockWorkingDir().") } params := []pegomock.Param{logger, headRepo, p, workspace} - result := pegomock.GetGenericMockFrom(mock).Invoke("Clone", params, []reflect.Type{reflect.TypeOf((*string)(nil)).Elem(), reflect.TypeOf((*bool)(nil)).Elem(), reflect.TypeOf((*error)(nil)).Elem()}) + result := pegomock.GetGenericMockFrom(mock).Invoke("Clone", params, []reflect.Type{reflect.TypeOf((*string)(nil)).Elem(), reflect.TypeOf((*error)(nil)).Elem()}) var ret0 string - var ret1 bool - var ret2 error + var ret1 error if len(result) != 0 { if result[0] != nil { ret0 = result[0].(string) } if result[1] != nil { - ret1 = result[1].(bool) - } - if result[2] != nil { - ret2 = result[2].(error) + ret1 = result[1].(error) } } - return ret0, ret1, ret2 + return ret0, ret1 } func (mock *MockWorkingDir) Delete(logger logging.SimpleLogging, r models.Repo, p models.PullRequest) error { @@ -166,12 +162,23 @@ func (mock *MockWorkingDir) HasDiverged(logger logging.SimpleLogging, cloneDir s return ret0 } -func (mock *MockWorkingDir) SetCheckForUpstreamChanges() { +func (mock *MockWorkingDir) MergeAgain(logger logging.SimpleLogging, headRepo models.Repo, p models.PullRequest, workspace string) (bool, error) { if mock == nil { panic("mock must not be nil. Use myMock := NewMockWorkingDir().") } - params := []pegomock.Param{} - pegomock.GetGenericMockFrom(mock).Invoke("SetCheckForUpstreamChanges", params, []reflect.Type{}) + params := []pegomock.Param{logger, headRepo, p, workspace} + result := pegomock.GetGenericMockFrom(mock).Invoke("MergeAgain", params, []reflect.Type{reflect.TypeOf((*bool)(nil)).Elem(), reflect.TypeOf((*error)(nil)).Elem()}) + var ret0 bool + var ret1 error + if len(result) != 0 { + if result[0] != nil { + ret0 = result[0].(bool) + } + if result[1] != nil { + ret1 = result[1].(error) + } + } + return ret0, ret1 } func (mock *MockWorkingDir) VerifyWasCalledOnce() *VerifierMockWorkingDir { @@ -507,19 +514,41 @@ func (c *MockWorkingDir_HasDiverged_OngoingVerification) GetAllCapturedArguments return } -func (verifier *VerifierMockWorkingDir) SetCheckForUpstreamChanges() *MockWorkingDir_SetCheckForUpstreamChanges_OngoingVerification { - params := []pegomock.Param{} - methodInvocations := pegomock.GetGenericMockFrom(verifier.mock).Verify(verifier.inOrderContext, verifier.invocationCountMatcher, "SetCheckForUpstreamChanges", params, verifier.timeout) - return &MockWorkingDir_SetCheckForUpstreamChanges_OngoingVerification{mock: verifier.mock, methodInvocations: methodInvocations} +func (verifier *VerifierMockWorkingDir) MergeAgain(logger logging.SimpleLogging, headRepo models.Repo, p models.PullRequest, workspace string) *MockWorkingDir_MergeAgain_OngoingVerification { + params := []pegomock.Param{logger, headRepo, p, workspace} + methodInvocations := pegomock.GetGenericMockFrom(verifier.mock).Verify(verifier.inOrderContext, verifier.invocationCountMatcher, "MergeAgain", params, verifier.timeout) + return &MockWorkingDir_MergeAgain_OngoingVerification{mock: verifier.mock, methodInvocations: methodInvocations} } -type MockWorkingDir_SetCheckForUpstreamChanges_OngoingVerification struct { +type MockWorkingDir_MergeAgain_OngoingVerification struct { mock *MockWorkingDir methodInvocations []pegomock.MethodInvocation } -func (c *MockWorkingDir_SetCheckForUpstreamChanges_OngoingVerification) GetCapturedArguments() { +func (c *MockWorkingDir_MergeAgain_OngoingVerification) GetCapturedArguments() (logging.SimpleLogging, models.Repo, models.PullRequest, string) { + logger, headRepo, p, workspace := c.GetAllCapturedArguments() + return logger[len(logger)-1], headRepo[len(headRepo)-1], p[len(p)-1], workspace[len(workspace)-1] } -func (c *MockWorkingDir_SetCheckForUpstreamChanges_OngoingVerification) GetAllCapturedArguments() { +func (c *MockWorkingDir_MergeAgain_OngoingVerification) GetAllCapturedArguments() (_param0 []logging.SimpleLogging, _param1 []models.Repo, _param2 []models.PullRequest, _param3 []string) { + params := pegomock.GetGenericMockFrom(c.mock).GetInvocationParams(c.methodInvocations) + if len(params) > 0 { + _param0 = make([]logging.SimpleLogging, len(c.methodInvocations)) + for u, param := range params[0] { + _param0[u] = param.(logging.SimpleLogging) + } + _param1 = make([]models.Repo, len(c.methodInvocations)) + for u, param := range params[1] { + _param1[u] = param.(models.Repo) + } + _param2 = make([]models.PullRequest, len(c.methodInvocations)) + for u, param := range params[2] { + _param2[u] = param.(models.PullRequest) + } + _param3 = make([]string, len(c.methodInvocations)) + for u, param := range params[3] { + _param3[u] = param.(string) + } + } + return } diff --git a/server/events/post_workflow_hooks_command_runner.go b/server/events/post_workflow_hooks_command_runner.go index f5fe0c5245..018350f79f 100644 --- a/server/events/post_workflow_hooks_command_runner.go +++ b/server/events/post_workflow_hooks_command_runner.go @@ -59,7 +59,7 @@ func (w *DefaultPostWorkflowHooksCommandRunner) RunPostHooks(ctx *command.Contex ctx.Log.Debug("got workspace lock") defer unlockFn() - repoDir, _, err := w.WorkingDir.Clone(ctx.Log, ctx.HeadRepo, ctx.Pull, DefaultWorkspace) + repoDir, err := w.WorkingDir.Clone(ctx.Log, ctx.HeadRepo, ctx.Pull, DefaultWorkspace) if err != nil { return err } diff --git a/server/events/post_workflow_hooks_command_runner_test.go b/server/events/post_workflow_hooks_command_runner_test.go index 29996d8028..7fdb68c8f9 100644 --- a/server/events/post_workflow_hooks_command_runner_test.go +++ b/server/events/post_workflow_hooks_command_runner_test.go @@ -143,7 +143,7 @@ func TestRunPostHooks_Clone(t *testing.T) { When(postWhWorkingDirLocker.TryLock(testdata.GithubRepo.FullName, newPull.Num, events.DefaultWorkspace, events.DefaultRepoRelDir)).ThenReturn(unlockFn, nil) When(postWhWorkingDir.Clone(Any[logging.SimpleLogging](), Eq(testdata.GithubRepo), Eq(newPull), - Eq(events.DefaultWorkspace))).ThenReturn(repoDir, false, nil) + Eq(events.DefaultWorkspace))).ThenReturn(repoDir, nil) When(whPostWorkflowHookRunner.Run(Any[models.WorkflowHookCommandContext](), Eq(testHook.RunCommand), Any[string](), Any[string](), Eq(repoDir))).ThenReturn(result, runtimeDesc, nil) @@ -237,7 +237,7 @@ func TestRunPostHooks_Clone(t *testing.T) { When(postWhWorkingDirLocker.TryLock(testdata.GithubRepo.FullName, newPull.Num, events.DefaultWorkspace, events.DefaultRepoRelDir)).ThenReturn(unlockFn, nil) When(postWhWorkingDir.Clone(Any[logging.SimpleLogging](), Eq(testdata.GithubRepo), Eq(newPull), - Eq(events.DefaultWorkspace))).ThenReturn(repoDir, false, errors.New("some error")) + Eq(events.DefaultWorkspace))).ThenReturn(repoDir, errors.New("some error")) err := postWh.RunPostHooks(ctx, planCmd) @@ -272,7 +272,7 @@ func TestRunPostHooks_Clone(t *testing.T) { When(postWhWorkingDirLocker.TryLock(testdata.GithubRepo.FullName, newPull.Num, events.DefaultWorkspace, events.DefaultRepoRelDir)).ThenReturn(unlockFn, nil) When(postWhWorkingDir.Clone(Any[logging.SimpleLogging](), Eq(testdata.GithubRepo), Eq(newPull), - Eq(events.DefaultWorkspace))).ThenReturn(repoDir, false, nil) + Eq(events.DefaultWorkspace))).ThenReturn(repoDir, nil) When(whPostWorkflowHookRunner.Run(Any[models.WorkflowHookCommandContext](), Eq(testHook.RunCommand), Any[string](), Any[string](), Eq(repoDir))).ThenReturn(result, runtimeDesc, errors.New("some error")) @@ -314,7 +314,7 @@ func TestRunPostHooks_Clone(t *testing.T) { When(postWhWorkingDirLocker.TryLock(testdata.GithubRepo.FullName, newPull.Num, events.DefaultWorkspace, events.DefaultRepoRelDir)).ThenReturn(unlockFn, nil) When(postWhWorkingDir.Clone(Any[logging.SimpleLogging](), Eq(testdata.GithubRepo), Eq(newPull), - Eq(events.DefaultWorkspace))).ThenReturn(repoDir, false, nil) + Eq(events.DefaultWorkspace))).ThenReturn(repoDir, nil) When(whPostWorkflowHookRunner.Run(Any[models.WorkflowHookCommandContext](), Eq(testHook.RunCommand), Any[string](), Any[string](), Eq(repoDir))).ThenReturn(result, runtimeDesc, nil) @@ -350,7 +350,7 @@ func TestRunPostHooks_Clone(t *testing.T) { When(postWhWorkingDirLocker.TryLock(testdata.GithubRepo.FullName, newPull.Num, events.DefaultWorkspace, events.DefaultRepoRelDir)).ThenReturn(unlockFn, nil) When(postWhWorkingDir.Clone(Any[logging.SimpleLogging](), Eq(testdata.GithubRepo), Eq(newPull), - Eq(events.DefaultWorkspace))).ThenReturn(repoDir, false, nil) + Eq(events.DefaultWorkspace))).ThenReturn(repoDir, nil) When(whPostWorkflowHookRunner.Run(Any[models.WorkflowHookCommandContext](), Eq(testHookWithShell.RunCommand), Any[string](), Any[string](), Eq(repoDir))).ThenReturn(result, runtimeDesc, nil) @@ -386,7 +386,7 @@ func TestRunPostHooks_Clone(t *testing.T) { When(postWhWorkingDirLocker.TryLock(testdata.GithubRepo.FullName, newPull.Num, events.DefaultWorkspace, events.DefaultRepoRelDir)).ThenReturn(unlockFn, nil) When(postWhWorkingDir.Clone(Any[logging.SimpleLogging](), Eq(testdata.GithubRepo), Eq(newPull), - Eq(events.DefaultWorkspace))).ThenReturn(repoDir, false, nil) + Eq(events.DefaultWorkspace))).ThenReturn(repoDir, nil) When(whPostWorkflowHookRunner.Run(Any[models.WorkflowHookCommandContext](), Eq(testHook.RunCommand), Any[string](), Any[string](), Eq(repoDir))).ThenReturn(result, runtimeDesc, nil) @@ -422,7 +422,7 @@ func TestRunPostHooks_Clone(t *testing.T) { When(postWhWorkingDirLocker.TryLock(testdata.GithubRepo.FullName, newPull.Num, events.DefaultWorkspace, events.DefaultRepoRelDir)).ThenReturn(unlockFn, nil) When(postWhWorkingDir.Clone(Any[logging.SimpleLogging](), Eq(testdata.GithubRepo), Eq(newPull), - Eq(events.DefaultWorkspace))).ThenReturn(repoDir, false, nil) + Eq(events.DefaultWorkspace))).ThenReturn(repoDir, nil) When(whPostWorkflowHookRunner.Run(Any[models.WorkflowHookCommandContext](), Eq(testHookWithShellandShellArgs.RunCommand), Any[string](), Any[string](), Eq(repoDir))).ThenReturn(result, runtimeDesc, nil) @@ -459,7 +459,7 @@ func TestRunPostHooks_Clone(t *testing.T) { When(preWhWorkingDirLocker.TryLock(testdata.GithubRepo.FullName, newPull.Num, events.DefaultWorkspace, events.DefaultRepoRelDir)).ThenReturn(unlockFn, nil) When(preWhWorkingDir.Clone(Any[logging.SimpleLogging](), Eq(testdata.GithubRepo), Eq(newPull), - Eq(events.DefaultWorkspace))).ThenReturn(repoDir, false, nil) + Eq(events.DefaultWorkspace))).ThenReturn(repoDir, nil) When(whPreWorkflowHookRunner.Run(Any[models.WorkflowHookCommandContext](), Eq(testHookWithPlanCommand.RunCommand), Any[string](), Any[string](), Eq(repoDir))).ThenReturn(result, runtimeDesc, nil) @@ -495,7 +495,7 @@ func TestRunPostHooks_Clone(t *testing.T) { When(preWhWorkingDirLocker.TryLock(testdata.GithubRepo.FullName, newPull.Num, events.DefaultWorkspace, events.DefaultRepoRelDir)).ThenReturn(unlockFn, nil) When(preWhWorkingDir.Clone(Any[logging.SimpleLogging](), Eq(testdata.GithubRepo), Eq(newPull), - Eq(events.DefaultWorkspace))).ThenReturn(repoDir, false, nil) + Eq(events.DefaultWorkspace))).ThenReturn(repoDir, nil) When(whPreWorkflowHookRunner.Run(Any[models.WorkflowHookCommandContext](), Eq(testHookWithPlanCommand.RunCommand), Any[string](), Any[string](), Eq(repoDir))).ThenReturn(result, runtimeDesc, nil) @@ -531,7 +531,7 @@ func TestRunPostHooks_Clone(t *testing.T) { When(preWhWorkingDirLocker.TryLock(testdata.GithubRepo.FullName, newPull.Num, events.DefaultWorkspace, events.DefaultRepoRelDir)).ThenReturn(unlockFn, nil) When(preWhWorkingDir.Clone(Any[logging.SimpleLogging](), Eq(testdata.GithubRepo), Eq(newPull), - Eq(events.DefaultWorkspace))).ThenReturn(repoDir, false, nil) + Eq(events.DefaultWorkspace))).ThenReturn(repoDir, nil) When(whPreWorkflowHookRunner.Run(Any[models.WorkflowHookCommandContext](), Eq(testHookWithPlanApplyCommands.RunCommand), Any[string](), Any[string](), Eq(repoDir))).ThenReturn(result, runtimeDesc, nil) diff --git a/server/events/pre_workflow_hooks_command_runner.go b/server/events/pre_workflow_hooks_command_runner.go index 70462765a3..8d2442d060 100644 --- a/server/events/pre_workflow_hooks_command_runner.go +++ b/server/events/pre_workflow_hooks_command_runner.go @@ -59,7 +59,7 @@ func (w *DefaultPreWorkflowHooksCommandRunner) RunPreHooks(ctx *command.Context, ctx.Log.Debug("got workspace lock") defer unlockFn() - repoDir, _, err := w.WorkingDir.Clone(ctx.Log, ctx.HeadRepo, ctx.Pull, DefaultWorkspace) + repoDir, err := w.WorkingDir.Clone(ctx.Log, ctx.HeadRepo, ctx.Pull, DefaultWorkspace) if err != nil { return err } diff --git a/server/events/pre_workflow_hooks_command_runner_test.go b/server/events/pre_workflow_hooks_command_runner_test.go index 191a8c27dc..29912576da 100644 --- a/server/events/pre_workflow_hooks_command_runner_test.go +++ b/server/events/pre_workflow_hooks_command_runner_test.go @@ -145,7 +145,7 @@ func TestRunPreHooks_Clone(t *testing.T) { When(preWhWorkingDirLocker.TryLock(testdata.GithubRepo.FullName, newPull.Num, events.DefaultWorkspace, events.DefaultRepoRelDir)).ThenReturn(unlockFn, nil) When(preWhWorkingDir.Clone(Any[logging.SimpleLogging](), Eq(testdata.GithubRepo), Eq(newPull), - Eq(events.DefaultWorkspace))).ThenReturn(repoDir, false, nil) + Eq(events.DefaultWorkspace))).ThenReturn(repoDir, nil) When(whPreWorkflowHookRunner.Run(Any[models.WorkflowHookCommandContext](), Eq(testHook.RunCommand), Any[string](), Any[string](), Eq(repoDir))).ThenReturn(result, runtimeDesc, nil) @@ -241,7 +241,7 @@ func TestRunPreHooks_Clone(t *testing.T) { When(preWhWorkingDirLocker.TryLock(testdata.GithubRepo.FullName, newPull.Num, events.DefaultWorkspace, events.DefaultRepoRelDir)).ThenReturn(unlockFn, nil) When(preWhWorkingDir.Clone(Any[logging.SimpleLogging](), Eq(testdata.GithubRepo), Eq(newPull), - Eq(events.DefaultWorkspace))).ThenReturn(repoDir, false, errors.New("some error")) + Eq(events.DefaultWorkspace))).ThenReturn(repoDir, errors.New("some error")) err := preWh.RunPreHooks(ctx, planCmd) @@ -276,7 +276,7 @@ func TestRunPreHooks_Clone(t *testing.T) { When(preWhWorkingDirLocker.TryLock(testdata.GithubRepo.FullName, newPull.Num, events.DefaultWorkspace, events.DefaultRepoRelDir)).ThenReturn(unlockFn, nil) When(preWhWorkingDir.Clone(Any[logging.SimpleLogging](), Eq(testdata.GithubRepo), Eq(newPull), - Eq(events.DefaultWorkspace))).ThenReturn(repoDir, false, nil) + Eq(events.DefaultWorkspace))).ThenReturn(repoDir, nil) When(whPreWorkflowHookRunner.Run(Any[models.WorkflowHookCommandContext](), Eq(testHook.RunCommand), Any[string](), Any[string](), Eq(repoDir))).ThenReturn(result, runtimeDesc, errors.New("some error")) @@ -318,7 +318,7 @@ func TestRunPreHooks_Clone(t *testing.T) { When(preWhWorkingDirLocker.TryLock(testdata.GithubRepo.FullName, newPull.Num, events.DefaultWorkspace, events.DefaultRepoRelDir)).ThenReturn(unlockFn, nil) When(preWhWorkingDir.Clone(Any[logging.SimpleLogging](), Eq(testdata.GithubRepo), Eq(newPull), - Eq(events.DefaultWorkspace))).ThenReturn(repoDir, false, nil) + Eq(events.DefaultWorkspace))).ThenReturn(repoDir, nil) When(whPreWorkflowHookRunner.Run(Any[models.WorkflowHookCommandContext](), Eq(testHook.RunCommand), Any[string](), Any[string](), Eq(repoDir))).ThenReturn(result, runtimeDesc, nil) @@ -354,7 +354,7 @@ func TestRunPreHooks_Clone(t *testing.T) { When(preWhWorkingDirLocker.TryLock(testdata.GithubRepo.FullName, newPull.Num, events.DefaultWorkspace, events.DefaultRepoRelDir)).ThenReturn(unlockFn, nil) When(preWhWorkingDir.Clone(Any[logging.SimpleLogging](), Eq(testdata.GithubRepo), Eq(newPull), - Eq(events.DefaultWorkspace))).ThenReturn(repoDir, false, nil) + Eq(events.DefaultWorkspace))).ThenReturn(repoDir, nil) When(whPreWorkflowHookRunner.Run(Any[models.WorkflowHookCommandContext](), Eq(testHookWithShell.RunCommand), Any[string](), Any[string](), Eq(repoDir))).ThenReturn(result, runtimeDesc, nil) @@ -390,7 +390,7 @@ func TestRunPreHooks_Clone(t *testing.T) { When(preWhWorkingDirLocker.TryLock(testdata.GithubRepo.FullName, newPull.Num, events.DefaultWorkspace, events.DefaultRepoRelDir)).ThenReturn(unlockFn, nil) When(preWhWorkingDir.Clone(Any[logging.SimpleLogging](), Eq(testdata.GithubRepo), Eq(newPull), - Eq(events.DefaultWorkspace))).ThenReturn(repoDir, false, nil) + Eq(events.DefaultWorkspace))).ThenReturn(repoDir, nil) When(whPreWorkflowHookRunner.Run(Any[models.WorkflowHookCommandContext](), Eq(testHook.RunCommand), Any[string](), Any[string](), Eq(repoDir))).ThenReturn(result, runtimeDesc, nil) @@ -426,7 +426,7 @@ func TestRunPreHooks_Clone(t *testing.T) { When(preWhWorkingDirLocker.TryLock(testdata.GithubRepo.FullName, newPull.Num, events.DefaultWorkspace, events.DefaultRepoRelDir)).ThenReturn(unlockFn, nil) When(preWhWorkingDir.Clone(Any[logging.SimpleLogging](), Eq(testdata.GithubRepo), Eq(newPull), - Eq(events.DefaultWorkspace))).ThenReturn(repoDir, false, nil) + Eq(events.DefaultWorkspace))).ThenReturn(repoDir, nil) When(whPreWorkflowHookRunner.Run(Any[models.WorkflowHookCommandContext](), Eq(testHookWithShellandShellArgs.RunCommand), Any[string](), Any[string](), Eq(repoDir))).ThenReturn(result, runtimeDesc, nil) @@ -463,7 +463,7 @@ func TestRunPreHooks_Clone(t *testing.T) { When(preWhWorkingDirLocker.TryLock(testdata.GithubRepo.FullName, newPull.Num, events.DefaultWorkspace, events.DefaultRepoRelDir)).ThenReturn(unlockFn, nil) When(preWhWorkingDir.Clone(Any[logging.SimpleLogging](), Eq(testdata.GithubRepo), Eq(newPull), - Eq(events.DefaultWorkspace))).ThenReturn(repoDir, false, nil) + Eq(events.DefaultWorkspace))).ThenReturn(repoDir, nil) When(whPreWorkflowHookRunner.Run(Any[models.WorkflowHookCommandContext](), Eq(testHookWithPlanCommand.RunCommand), Any[string](), Any[string](), Eq(repoDir))).ThenReturn(result, runtimeDesc, nil) @@ -499,7 +499,7 @@ func TestRunPreHooks_Clone(t *testing.T) { When(preWhWorkingDirLocker.TryLock(testdata.GithubRepo.FullName, newPull.Num, events.DefaultWorkspace, events.DefaultRepoRelDir)).ThenReturn(unlockFn, nil) When(preWhWorkingDir.Clone(Any[logging.SimpleLogging](), Eq(testdata.GithubRepo), Eq(newPull), - Eq(events.DefaultWorkspace))).ThenReturn(repoDir, false, nil) + Eq(events.DefaultWorkspace))).ThenReturn(repoDir, nil) When(whPreWorkflowHookRunner.Run(Any[models.WorkflowHookCommandContext](), Eq(testHookWithPlanCommand.RunCommand), Any[string](), Any[string](), Eq(repoDir))).ThenReturn(result, runtimeDesc, nil) @@ -535,7 +535,7 @@ func TestRunPreHooks_Clone(t *testing.T) { When(preWhWorkingDirLocker.TryLock(testdata.GithubRepo.FullName, newPull.Num, events.DefaultWorkspace, events.DefaultRepoRelDir)).ThenReturn(unlockFn, nil) When(preWhWorkingDir.Clone(Any[logging.SimpleLogging](), Eq(testdata.GithubRepo), Eq(newPull), - Eq(events.DefaultWorkspace))).ThenReturn(repoDir, false, nil) + Eq(events.DefaultWorkspace))).ThenReturn(repoDir, nil) When(whPreWorkflowHookRunner.Run(Any[models.WorkflowHookCommandContext](), Eq(testHookWithPlanApplyCommands.RunCommand), Any[string](), Any[string](), Eq(repoDir))).ThenReturn(result, runtimeDesc, nil) diff --git a/server/events/project_command_builder.go b/server/events/project_command_builder.go index 7d587e4ce3..2f2f24bc19 100644 --- a/server/events/project_command_builder.go +++ b/server/events/project_command_builder.go @@ -397,7 +397,7 @@ func (p *DefaultProjectCommandBuilder) buildAllCommandsByCfg(ctx *command.Contex ctx.Log.Debug("got workspace lock") defer unlockFn() - repoDir, _, err := p.WorkingDir.Clone(ctx.Log, ctx.HeadRepo, ctx.Pull, workspace) + repoDir, err := p.WorkingDir.Clone(ctx.Log, ctx.HeadRepo, ctx.Pull, workspace) if err != nil { return nil, err } @@ -594,7 +594,7 @@ func (p *DefaultProjectCommandBuilder) buildProjectPlanCommand(ctx *command.Cont defer unlockFn() ctx.Log.Debug("cloning repository") - _, _, err = p.WorkingDir.Clone(ctx.Log, ctx.HeadRepo, ctx.Pull, DefaultWorkspace) + _, err = p.WorkingDir.Clone(ctx.Log, ctx.HeadRepo, ctx.Pull, DefaultWorkspace) if err != nil { return pcc, err } @@ -672,7 +672,7 @@ func (p *DefaultProjectCommandBuilder) buildProjectPlanCommand(ctx *command.Cont if DefaultWorkspace != workspace { ctx.Log.Debug("cloning repository with workspace %s", workspace) - _, _, err = p.WorkingDir.Clone(ctx.Log, ctx.HeadRepo, ctx.Pull, workspace) + _, err = p.WorkingDir.Clone(ctx.Log, ctx.HeadRepo, ctx.Pull, workspace) if err != nil { return pcc, err } diff --git a/server/events/project_command_builder_internal_test.go b/server/events/project_command_builder_internal_test.go index d020871b31..c6e7bc070d 100644 --- a/server/events/project_command_builder_internal_test.go +++ b/server/events/project_command_builder_internal_test.go @@ -631,7 +631,7 @@ projects: workingDir := NewMockWorkingDir() When(workingDir.Clone(Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest](), - Any[string]())).ThenReturn(tmp, false, nil) + Any[string]())).ThenReturn(tmp, nil) vcsClient := vcsmocks.NewMockClient() When(vcsClient.GetModifiedFiles(Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest]())).ThenReturn([]string{"modules/module/main.tf"}, nil) @@ -846,7 +846,7 @@ projects: workingDir := NewMockWorkingDir() When(workingDir.Clone(Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest](), - Any[string]())).ThenReturn(tmp, false, nil) + Any[string]())).ThenReturn(tmp, nil) vcsClient := vcsmocks.NewMockClient() When(vcsClient.GetModifiedFiles(Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest]())).ThenReturn([]string{"modules/module/main.tf"}, nil) @@ -1091,7 +1091,7 @@ workflows: workingDir := NewMockWorkingDir() When(workingDir.Clone(Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest](), - Any[string]())).ThenReturn(tmp, false, nil) + Any[string]())).ThenReturn(tmp, nil) vcsClient := vcsmocks.NewMockClient() When(vcsClient.GetModifiedFiles(Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest]())).ThenReturn([]string{"modules/module/main.tf"}, nil) @@ -1245,7 +1245,7 @@ projects: workingDir := NewMockWorkingDir() When(workingDir.Clone(Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest](), - Any[string]())).ThenReturn(tmp, false, nil) + Any[string]())).ThenReturn(tmp, nil) vcsClient := vcsmocks.NewMockClient() When(vcsClient.GetModifiedFiles(Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest]())).ThenReturn([]string{"modules/module/main.tf"}, nil) @@ -1385,7 +1385,7 @@ projects: workingDir := NewMockWorkingDir() When(workingDir.Clone(Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest](), - Any[string]())).ThenReturn(tmp, false, nil) + Any[string]())).ThenReturn(tmp, nil) vcsClient := vcsmocks.NewMockClient() When(vcsClient.GetModifiedFiles(Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest]())).ThenReturn(c.modifiedFiles, nil) diff --git a/server/events/project_command_builder_test.go b/server/events/project_command_builder_test.go index 7560b5d6de..039612f617 100644 --- a/server/events/project_command_builder_test.go +++ b/server/events/project_command_builder_test.go @@ -241,7 +241,7 @@ terraform { tmpDir := DirStructure(t, c.TestDirStructure) workingDir := mocks.NewMockWorkingDir() When(workingDir.Clone(Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest](), - Any[string]())).ThenReturn(tmpDir, false, nil) + Any[string]())).ThenReturn(tmpDir, nil) vcsClient := vcsmocks.NewMockClient() When(vcsClient.GetModifiedFiles(Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest]())).ThenReturn(ChangedFiles(c.TestDirStructure, ""), nil) @@ -602,7 +602,7 @@ projects: workingDir := mocks.NewMockWorkingDir() When(workingDir.Clone(Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest](), - Any[string]())).ThenReturn(tmpDir, false, nil) + Any[string]())).ThenReturn(tmpDir, nil) When(workingDir.GetWorkingDir(Any[models.Repo](), Any[models.PullRequest](), Any[string]())).ThenReturn(tmpDir, nil) vcsClient := vcsmocks.NewMockClient() When(vcsClient.GetModifiedFiles(Any[logging.SimpleLogging](), Any[models.Repo](), @@ -790,7 +790,7 @@ projects: workingDir := mocks.NewMockWorkingDir() When(workingDir.Clone(Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest](), - Any[string]())).ThenReturn(tmpDir, false, nil) + Any[string]())).ThenReturn(tmpDir, nil) When(workingDir.GetWorkingDir(Any[models.Repo](), Any[models.PullRequest](), Any[string]())).ThenReturn(tmpDir, nil) vcsClient := vcsmocks.NewMockClient() When(vcsClient.GetModifiedFiles(Any[logging.SimpleLogging](), Any[models.Repo](), @@ -1119,7 +1119,7 @@ projects: workingDir := mocks.NewMockWorkingDir() When(workingDir.Clone(Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest](), - Any[string]())).ThenReturn(tmpDir, false, nil) + Any[string]())).ThenReturn(tmpDir, nil) When(workingDir.GetWorkingDir(Any[models.Repo](), Any[models.PullRequest](), Any[string]())).ThenReturn(tmpDir, nil) vcsClient := vcsmocks.NewMockClient() When(vcsClient.GetModifiedFiles(Any[logging.SimpleLogging](), Any[models.Repo](), @@ -1307,7 +1307,7 @@ projects: Ok(t, err) When(workingDir.Clone(Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest](), - Any[string]())).ThenReturn(repoDir, false, nil) + Any[string]())).ThenReturn(repoDir, nil) When(workingDir.GetWorkingDir(Any[models.Repo](), Any[models.PullRequest](), Any[string]())).ThenReturn(repoDir, nil) globalCfgArgs := valid.GlobalCfgArgs{ @@ -1395,7 +1395,7 @@ func TestDefaultProjectCommandBuilder_EscapeArgs(t *testing.T) { workingDir := mocks.NewMockWorkingDir() When(workingDir.Clone(Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest](), - Any[string]())).ThenReturn(tmpDir, false, nil) + Any[string]())).ThenReturn(tmpDir, nil) When(workingDir.GetWorkingDir(Any[models.Repo](), Any[models.PullRequest](), Any[string]())).ThenReturn(tmpDir, nil) vcsClient := vcsmocks.NewMockClient() When(vcsClient.GetModifiedFiles(Any[logging.SimpleLogging](), Any[models.Repo](), @@ -1551,7 +1551,7 @@ projects: Any[models.PullRequest]())).ThenReturn(testCase.ModifiedFiles, nil) workingDir := mocks.NewMockWorkingDir() When(workingDir.Clone(Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest](), - Any[string]())).ThenReturn(tmpDir, false, nil) + Any[string]())).ThenReturn(tmpDir, nil) When(workingDir.GetWorkingDir(Any[models.Repo](), Any[models.PullRequest](), Any[string]())).ThenReturn(tmpDir, nil) globalCfgArgs := valid.GlobalCfgArgs{ @@ -1735,7 +1735,7 @@ func TestDefaultProjectCommandBuilder_WithPolicyCheckEnabled_BuildAutoplanComman workingDir := mocks.NewMockWorkingDir() When(workingDir.Clone(Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest](), - Any[string]())).ThenReturn(tmpDir, false, nil) + Any[string]())).ThenReturn(tmpDir, nil) vcsClient := vcsmocks.NewMockClient() When(vcsClient.GetModifiedFiles(Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest]())).ThenReturn([]string{"main.tf"}, nil) @@ -1952,7 +1952,7 @@ func TestDefaultProjectCommandBuilder_BuildPlanCommands_Single_With_RestrictFile workingDir := mocks.NewMockWorkingDir() When(workingDir.Clone(Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest](), - Any[string]())).ThenReturn(tmpDir, false, nil) + Any[string]())).ThenReturn(tmpDir, nil) When(workingDir.GetWorkingDir(Any[models.Repo](), Any[models.PullRequest](), Any[string]())).ThenReturn(tmpDir, nil) When(workingDir.GetGitUntrackedFiles(Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest](), Any[string]())).ThenReturn(c.UntrackedFiles, nil) @@ -2063,7 +2063,7 @@ func TestDefaultProjectCommandBuilder_BuildPlanCommands_with_IncludeGitUntracked workingDir := mocks.NewMockWorkingDir() When(workingDir.Clone(Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest](), - Any[string]())).ThenReturn(tmpDir, false, nil) + Any[string]())).ThenReturn(tmpDir, nil) When(workingDir.GetWorkingDir(Any[models.Repo](), Any[models.PullRequest](), Any[string]())).ThenReturn(tmpDir, nil) When(workingDir.GetGitUntrackedFiles(Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest](), Any[string]())).ThenReturn(c.UntrackedFiles, nil) diff --git a/server/events/project_command_runner.go b/server/events/project_command_runner.go index 153269c7e2..e2cb01155b 100644 --- a/server/events/project_command_runner.go +++ b/server/events/project_command_runner.go @@ -555,15 +555,22 @@ func (p *DefaultProjectCommandRunner) doPlan(ctx command.ProjectContext) (*model } defer unlockFn() - p.WorkingDir.SetCheckForUpstreamChanges() // Clone is idempotent so okay to run even if the repo was already cloned. - repoDir, mergedAgain, cloneErr := p.WorkingDir.Clone(ctx.Log, ctx.HeadRepo, ctx.Pull, ctx.Workspace) + repoDir, cloneErr := p.WorkingDir.Clone(ctx.Log, 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) } - return nil, "", cloneErr + return nil, "", err } + mergedAgain, err := p.WorkingDir.MergeAgain(ctx.Log, ctx.HeadRepo, ctx.Pull, ctx.Workspace) + if err != nil { + if unlockErr := lockAttempt.UnlockFn(); unlockErr != nil { + ctx.Log.Err("error unlocking state after plan error: %v", unlockErr) + } + return nil, "", err + } + projAbsPath := filepath.Join(repoDir, ctx.RepoRelDir) if _, err = os.Stat(projAbsPath); os.IsNotExist(err) { return nil, "", DirNotExistErr{RepoRelDir: ctx.RepoRelDir} @@ -680,7 +687,7 @@ func (p *DefaultProjectCommandRunner) doVersion(ctx command.ProjectContext) (ver func (p *DefaultProjectCommandRunner) doImport(ctx command.ProjectContext) (out *models.ImportSuccess, failure string, err error) { // Clone is idempotent so okay to run even if the repo was already cloned. - repoDir, _, cloneErr := p.WorkingDir.Clone(ctx.Log, ctx.HeadRepo, ctx.Pull, ctx.Workspace) + repoDir, cloneErr := p.WorkingDir.Clone(ctx.Log, ctx.HeadRepo, ctx.Pull, ctx.Workspace) if cloneErr != nil { return nil, "", cloneErr } @@ -726,7 +733,7 @@ func (p *DefaultProjectCommandRunner) doImport(ctx command.ProjectContext) (out func (p *DefaultProjectCommandRunner) doStateRm(ctx command.ProjectContext) (out *models.StateRmSuccess, failure string, err error) { // Clone is idempotent so okay to run even if the repo was already cloned. - repoDir, _, cloneErr := p.WorkingDir.Clone(ctx.Log, ctx.HeadRepo, ctx.Pull, ctx.Workspace) + repoDir, cloneErr := p.WorkingDir.Clone(ctx.Log, ctx.HeadRepo, ctx.Pull, ctx.Workspace) if cloneErr != nil { return nil, "", cloneErr } diff --git a/server/events/project_command_runner_test.go b/server/events/project_command_runner_test.go index d241d44569..d4436ce13c 100644 --- a/server/events/project_command_runner_test.go +++ b/server/events/project_command_runner_test.go @@ -64,7 +64,7 @@ func TestDefaultProjectCommandRunner_Plan(t *testing.T) { repoDir := t.TempDir() When(mockWorkingDir.Clone(Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest](), - Any[string]())).ThenReturn(repoDir, false, nil) + Any[string]())).ThenReturn(repoDir, nil) When(mockLocker.TryLock(Any[logging.SimpleLogging](), Any[models.PullRequest](), Any[models.User](), Any[string](), Any[models.Project](), AnyBool())).ThenReturn(&events.TryLockResponse{LockAcquired: true, LockKey: "lock-key"}, nil) @@ -104,6 +104,7 @@ func TestDefaultProjectCommandRunner_Plan(t *testing.T) { Assert(t, res.PlanSuccess != nil, "exp plan success") Equals(t, "https://lock-key", res.PlanSuccess.LockURL) + t.Logf("output is %s", res.PlanSuccess.TerraformOutput) Equals(t, "run\napply\nplan\ninit", res.PlanSuccess.TerraformOutput) expSteps := []string{"run", "apply", "plan", "init", "env"} for _, step := range expSteps { @@ -571,7 +572,7 @@ func TestDefaultProjectCommandRunner_RunEnvSteps(t *testing.T) { repoDir := t.TempDir() When(mockWorkingDir.Clone(Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest](), - Any[string]())).ThenReturn(repoDir, false, nil) + Any[string]())).ThenReturn(repoDir, nil) When(mockLocker.TryLock(Any[logging.SimpleLogging](), Any[models.PullRequest](), Any[models.User](), Any[string](), Any[models.Project](), AnyBool())).ThenReturn(&events.TryLockResponse{LockAcquired: true, LockKey: "lock-key"}, nil) @@ -713,7 +714,7 @@ func TestDefaultProjectCommandRunner_Import(t *testing.T) { } repoDir := t.TempDir() When(mockWorkingDir.Clone(Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest](), - Any[string]())).ThenReturn(repoDir, false, nil) + Any[string]())).ThenReturn(repoDir, nil) if c.setup != nil { c.setup(repoDir, ctx, mockLocker, mockInit, mockImport) } diff --git a/server/events/working_dir.go b/server/events/working_dir.go index 392e561aa0..37c087ccf9 100644 --- a/server/events/working_dir.go +++ b/server/events/working_dir.go @@ -40,10 +40,11 @@ var recheckRequiredMap sync.Map // WorkingDir handles the workspace on disk for running commands. type WorkingDir interface { // Clone git clones headRepo, checks out the branch and then returns the - // absolute path to the root of the cloned repo. It also returns - // a boolean indicating if we should warn users that the branch we're - // merging into has been updated since we cloned it. - Clone(logger logging.SimpleLogging, headRepo models.Repo, p models.PullRequest, workspace string) (string, bool, error) + // absolute path to the root of the cloned repo. + Clone(logger logging.SimpleLogging, headRepo models.Repo, p models.PullRequest, workspace string) (string, error) + // MergeAgain merges again with upstream if upstream has been modified, returns + // whether it actually did a new merge + MergeAgain(logger logging.SimpleLogging, headRepo models.Repo, p models.PullRequest, workspace string) (bool, error) // GetWorkingDir returns the path to the workspace for this repo and pull. // If workspace does not exist on disk, error will be of type os.IsNotExist. GetWorkingDir(r models.Repo, p models.PullRequest, workspace string) (string, error) @@ -52,10 +53,6 @@ type WorkingDir interface { // Delete deletes the workspace for this repo and pull. Delete(logger logging.SimpleLogging, r models.Repo, p models.PullRequest) error DeleteForWorkspace(logger logging.SimpleLogging, r models.Repo, p models.PullRequest, workspace string) error - // Set a flag in the workingdir so Clone() can know that it is safe to re-clone the workingdir if - // the upstream branch has been modified. This is only safe after grabbing the project lock - // and before running any plans - SetCheckForUpstreamChanges() // DeletePlan deletes the plan for this repo, pull, workspace path and project name DeletePlan(logger logging.SimpleLogging, r models.Repo, p models.PullRequest, workspace string, path string, projectName string) error // GetGitUntrackedFiles returns a list of Git untracked files in the working dir. @@ -91,24 +88,13 @@ type FileWorkspace struct { } // Clone git clones headRepo, checks out the branch and then returns the absolute -// path to the root of the cloned repo. It also returns -// a boolean indicating whether we had to merge with upstream again. +// path to the root of the cloned repo. // If the repo already exists and is at // the right commit it does nothing. This is to support running commands in // multiple dirs of the same repo without deleting existing plans. -func (w *FileWorkspace) Clone(logger logging.SimpleLogging, headRepo models.Repo, p models.PullRequest, workspace string) (string, bool, error) { +func (w *FileWorkspace) Clone(logger logging.SimpleLogging, headRepo models.Repo, p models.PullRequest, workspace string) (string, error) { cloneDir := w.cloneDir(p.BaseRepo, p, workspace) - // Disable the mustRecheckUpstream flag if we are not using the merge checkout strategy - mustRecheckUpstream := w.CheckForUpstreamChanges && w.CheckoutMerge - - if mustRecheckUpstream { - // We atomically set the recheckRequiredMap flag here before grabbing the clone lock. - // If the flag is cleared after we grab the lock, it means some other thread - // did the necessary work late enough that we do not have to do it again. - recheckRequiredMap.Store(cloneDir, struct{}{}) - } - // Unconditionally wait for the clone lock here, if anyone else is doing any clone // operation in this directory, we wait for it to finish before we check anything. value, _ := cloneLocks.LoadOrStore(cloneDir, new(sync.Mutex)) @@ -116,13 +102,6 @@ func (w *FileWorkspace) Clone(logger logging.SimpleLogging, headRepo models.Repo mutex.Lock() defer mutex.Unlock() - if mustRecheckUpstream { - if _, exists := recheckRequiredMap.Load(cloneDir); !exists { - mustRecheckUpstream = false - w.Logger.Debug("Skipping upstream check. Some other thread has done this for us") - } - } - c := wrappedGitContext{cloneDir, headRepo, p} // If the directory already exists, check if it's at the right commit. // If so, then we do nothing. @@ -143,33 +122,62 @@ func (w *FileWorkspace) Clone(logger logging.SimpleLogging, headRepo models.Repo outputRevParseCmd, err := revParseCmd.CombinedOutput() if err != nil { logger.Warn("will re-clone repo, could not determine if was at correct commit: %s: %s: %s", strings.Join(revParseCmd.Args, " "), err, string(outputRevParseCmd)) - return cloneDir, false, w.forceClone(logger, c) + return cloneDir, w.forceClone(logger, c) } currCommit := strings.Trim(string(outputRevParseCmd), "\n") // 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) { - if mustRecheckUpstream { - w.CheckForUpstreamChanges = false - recheckRequiredMap.Delete(cloneDir) - if w.recheckDiverged(p, headRepo, cloneDir) { - w.Logger.Info("base branch has been updated, using merge strategy and will merge again") - return cloneDir, true, w.mergeAgain(logger, c) - } - w.Logger.Debug("repo is at correct commit %q and there are no upstream changes", p.HeadCommit) - } else { - w.Logger.Debug("repo is at correct commit %q so will not re-clone", p.HeadCommit) - } - return cloneDir, false, nil - } else { - logger.Debug("repo was already cloned but is not at correct commit, wanted '%s' got '%s'", p.HeadCommit, currCommit) + logger.Debug("repo is at correct commit %q so will not re-clone", p.HeadCommit) + return cloneDir, nil } + logger.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(logger, c) + return cloneDir, w.forceClone(logger, c) +} + +// MergeAgain merges again with upstream if we are using the merge checkout strategy, +// and upstream has been modified since we last checked. +// It returns a flag indicating whether we had to merge with upstream again. +func (w *FileWorkspace) MergeAgain( + logger logging.SimpleLogging, + headRepo models.Repo, + p models.PullRequest, + workspace string) (bool, error) { + + if !w.CheckoutMerge { + return false, nil + } + + cloneDir := w.cloneDir(p.BaseRepo, p, workspace) + // We atomically set the recheckRequiredMap flag here before grabbing the clone lock. + // If the flag is cleared after we grab the lock, it means some other thread + // did the necessary work late enough that we do not have to do it again. + recheckRequiredMap.Store(cloneDir, struct{}{}) + + // Unconditionally wait for the clone lock here, if anyone else is doing any clone + // operation in this directory, we wait for it to finish before we check anything. + value, _ := cloneLocks.LoadOrStore(cloneDir, new(sync.Mutex)) + mutex := value.(*sync.Mutex) + mutex.Lock() + defer mutex.Unlock() + + if _, exists := recheckRequiredMap.Load(cloneDir); !exists { + logger.Debug("Skipping upstream check. Some other thread has done this for us") + return false, nil + } + recheckRequiredMap.Delete(cloneDir) + + c := wrappedGitContext{cloneDir, headRepo, p} + if w.recheckDiverged(logger, p, headRepo, cloneDir) { + logger.Info("base branch has been updated, using merge strategy and will merge again") + return true, w.mergeAgain(logger, c) + } + return false, nil } // recheckDiverged returns true if the branch we're merging into has diverged diff --git a/server/events/working_dir_test.go b/server/events/working_dir_test.go index e25c420100..26c82823b7 100644 --- a/server/events/working_dir_test.go +++ b/server/events/working_dir_test.go @@ -45,7 +45,7 @@ func TestClone_NoneExisting(t *testing.T) { GpgNoSigningEnabled: true, } - cloneDir, _, err := wd.Clone(logger, models.Repo{}, models.PullRequest{ + cloneDir, err := wd.Clone(logger, models.Repo{}, models.PullRequest{ BaseRepo: models.Repo{}, HeadBranch: "branch", }, "default") @@ -97,13 +97,12 @@ func TestClone_CheckoutMergeNoneExisting(t *testing.T) { GpgNoSigningEnabled: true, } - cloneDir, mergedAgain, err := wd.Clone(logger, models.Repo{}, models.PullRequest{ + cloneDir, err := wd.Clone(logger, models.Repo{}, models.PullRequest{ BaseRepo: models.Repo{}, HeadBranch: "branch", BaseBranch: "main", }, "default") Ok(t, err) - Equals(t, false, mergedAgain) // Check the commits. actBaseCommit := runCmd(t, cloneDir, "git", "rev-parse", "HEAD~1") @@ -148,25 +147,23 @@ func TestClone_CheckoutMergeNoReclone(t *testing.T) { GpgNoSigningEnabled: true, } - _, mergedAgain, err := wd.Clone(logger, models.Repo{}, models.PullRequest{ + _, err := wd.Clone(logger, models.Repo{}, models.PullRequest{ BaseRepo: models.Repo{}, HeadBranch: "branch", BaseBranch: "main", }, "default") Ok(t, err) - Equals(t, false, mergedAgain) // Create a file that we can use to check if the repo was recloned. runCmd(t, dataDir, "touch", "repos/0/default/proof") // Now run the clone again. - cloneDir, mergedAgain, err := wd.Clone(logger, models.Repo{}, models.PullRequest{ + cloneDir, err := wd.Clone(logger, models.Repo{}, models.PullRequest{ BaseRepo: models.Repo{}, HeadBranch: "branch", BaseBranch: "main", }, "default") Ok(t, err) - Equals(t, false, mergedAgain) // Check that our proof file is still there, proving that we didn't reclone. _, err = os.Stat(filepath.Join(cloneDir, "proof")) @@ -200,25 +197,23 @@ func TestClone_CheckoutMergeNoRecloneFastForward(t *testing.T) { GpgNoSigningEnabled: true, } - _, mergedAgain, err := wd.Clone(logger, models.Repo{}, models.PullRequest{ + _, err := wd.Clone(logger, models.Repo{}, models.PullRequest{ BaseRepo: models.Repo{}, HeadBranch: "branch", BaseBranch: "main", }, "default") Ok(t, err) - Equals(t, false, mergedAgain) // Create a file that we can use to check if the repo was recloned. runCmd(t, dataDir, "touch", "repos/0/default/proof") // Now run the clone again. - cloneDir, mergedAgain, err := wd.Clone(logger, models.Repo{}, models.PullRequest{ + cloneDir, err := wd.Clone(logger, models.Repo{}, models.PullRequest{ BaseRepo: models.Repo{}, HeadBranch: "branch", BaseBranch: "main", }, "default") Ok(t, err) - Equals(t, false, mergedAgain) // Check that our proof file is still there, proving that we didn't reclone. _, err = os.Stat(filepath.Join(cloneDir, "proof")) @@ -257,7 +252,7 @@ func TestClone_CheckoutMergeConflict(t *testing.T) { GpgNoSigningEnabled: true, } - _, _, err := wd.Clone(logger, models.Repo{}, models.PullRequest{ + _, err := wd.Clone(logger, models.Repo{}, models.PullRequest{ BaseRepo: models.Repo{}, HeadBranch: "branch", BaseBranch: "main", @@ -316,13 +311,12 @@ func TestClone_CheckoutMergeShallow(t *testing.T) { GpgNoSigningEnabled: true, } - cloneDir, mergedAgain, err := wd.Clone(logger, models.Repo{}, models.PullRequest{ + cloneDir, err := wd.Clone(logger, models.Repo{}, models.PullRequest{ BaseRepo: models.Repo{}, HeadBranch: "branch", BaseBranch: "main", }, "default") Ok(t, err) - Equals(t, false, mergedAgain) gotBaseCommitType := runCmd(t, cloneDir, "git", "cat-file", "-t", baseCommit) Assert(t, gotBaseCommitType == "commit\n", "should have merge-base in shallow repo") @@ -346,13 +340,12 @@ func TestClone_CheckoutMergeShallow(t *testing.T) { GpgNoSigningEnabled: true, } - cloneDir, mergedAgain, err := wd.Clone(logger, models.Repo{}, models.PullRequest{ + cloneDir, err := wd.Clone(logger, models.Repo{}, models.PullRequest{ BaseRepo: models.Repo{}, HeadBranch: "branch", BaseBranch: "main", }, "default") Ok(t, err) - Equals(t, false, mergedAgain) gotBaseCommitType := runCmd(t, cloneDir, "git", "cat-file", "-t", baseCommit) Assert(t, gotBaseCommitType == "commit\n", "should have merge-base in full repo") @@ -381,12 +374,11 @@ func TestClone_NoReclone(t *testing.T) { TestingOverrideHeadCloneURL: fmt.Sprintf("file://%s", repoDir), GpgNoSigningEnabled: true, } - cloneDir, mergedAgain, err := wd.Clone(logger, models.Repo{}, models.PullRequest{ + cloneDir, err := wd.Clone(logger, models.Repo{}, models.PullRequest{ BaseRepo: models.Repo{}, HeadBranch: "branch", }, "default") Ok(t, err) - Equals(t, false, mergedAgain) // Check that our proof file is still there. _, err = os.Stat(filepath.Join(cloneDir, "proof")) @@ -425,13 +417,12 @@ func TestClone_RecloneWrongCommit(t *testing.T) { TestingOverrideHeadCloneURL: fmt.Sprintf("file://%s", repoDir), GpgNoSigningEnabled: true, } - cloneDir, mergedAgain, err := wd.Clone(logger, models.Repo{}, models.PullRequest{ + cloneDir, err := wd.Clone(logger, models.Repo{}, models.PullRequest{ BaseRepo: models.Repo{}, HeadBranch: "branch", HeadCommit: expCommit, }, "default") Ok(t, err) - Equals(t, false, mergedAgain) assert.NoFileExists(t, planFile, "Plan file should have been wiped out by Clone") // Use rev-parse to verify at correct commit. @@ -506,23 +497,28 @@ func TestClone_MasterHasDiverged(t *testing.T) { Assert(t, err == nil, "creating plan file: %v", err) assert.FileExists(t, planFile) - // Run the clone without the checkout merge strategy. It should return + // Run MergeAgain without the checkout merge strategy. It should return // false for mergedAgain - _, mergedAgain, err := wd.Clone(logger, models.Repo{}, models.PullRequest{ + _, err = wd.Clone(logger, models.Repo{}, models.PullRequest{ BaseRepo: models.Repo{}, HeadBranch: "second-pr", BaseBranch: "main", }, "default") Ok(t, err) - Assert(t, mergedAgain == false, "Clone with CheckoutMerge=false should not merge") - assert.FileExists(t, planFile, "Existing plan file should not be deleted by Clone with merge disabled") + mergedAgain, err := wd.MergeAgain(logger, models.Repo{CloneURL: repoDir}, models.PullRequest{ + BaseRepo: models.Repo{CloneURL: repoDir}, + HeadBranch: "second-pr", + BaseBranch: "main", + }, "default") + Ok(t, err) + assert.FileExists(t, planFile, "Existing plan file should not be deleted by merging again") + Assert(t, mergedAgain == false, "MergeAgain with CheckoutMerge=false should not merge") wd.CheckoutMerge = true - wd.SetCheckForUpstreamChanges() // Run the clone twice with the merge strategy, the first run should // return true for mergedAgain, subsequent runs should // return false since the first call is supposed to merge. - _, mergedAgain, err = wd.Clone(logger, models.Repo{CloneURL: repoDir}, models.PullRequest{ + mergedAgain, err = wd.MergeAgain(logger, models.Repo{CloneURL: repoDir}, models.PullRequest{ BaseRepo: models.Repo{CloneURL: repoDir}, HeadBranch: "second-pr", BaseBranch: "main", @@ -531,8 +527,7 @@ func TestClone_MasterHasDiverged(t *testing.T) { assert.FileExists(t, planFile, "Existing plan file should not be deleted by merging again") Assert(t, mergedAgain == true, "First clone with CheckoutMerge=true with diverged base should have merged") - wd.SetCheckForUpstreamChanges() - _, mergedAgain, err = wd.Clone(logger, models.Repo{CloneURL: repoDir}, models.PullRequest{ + mergedAgain, err = wd.MergeAgain(logger, models.Repo{CloneURL: repoDir}, models.PullRequest{ BaseRepo: models.Repo{CloneURL: repoDir}, HeadBranch: "second-pr", BaseBranch: "main",