Skip to content

Commit

Permalink
refactor: split Clone into Clone and MergeAgain
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
finnag committed Sep 29, 2023
1 parent 7978762 commit 580ea23
Show file tree
Hide file tree
Showing 16 changed files with 230 additions and 166 deletions.
2 changes: 1 addition & 1 deletion server/events/github_app_working_dir.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ type GithubAppWorkingDir struct {
}

// Clone writes a fresh token for Github App authentication
func (g *GithubAppWorkingDir) Clone(headRepo models.Repo, p models.PullRequest, workspace string) (string, bool, error) {
func (g *GithubAppWorkingDir) Clone(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
Expand Down
8 changes: 4 additions & 4 deletions server/events/github_app_working_dir_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ func TestClone_GithubAppNoneExisting(t *testing.T) {
GithubHostname: testServer,
}

cloneDir, _, err := gwd.Clone(models.Repo{}, models.PullRequest{
cloneDir, err := gwd.Clone(models.Repo{}, models.PullRequest{
BaseRepo: models.Repo{},
HeadBranch: "branch",
}, "default")
Expand Down Expand Up @@ -89,12 +89,12 @@ func TestClone_GithubAppSetsCorrectUrl(t *testing.T) {

When(credentials.GetToken()).ThenReturn("token", nil)
When(workingDir.Clone(modifiedBaseRepo, models.PullRequest{BaseRepo: modifiedBaseRepo}, "default")).ThenReturn(
"", true, nil,
"", nil,
)

_, success, _ := ghAppWorkingDir.Clone(headRepo, models.PullRequest{BaseRepo: baseRepo}, "default")
_, err := ghAppWorkingDir.Clone(headRepo, models.PullRequest{BaseRepo: baseRepo}, "default")

workingDir.VerifyWasCalledOnce().Clone(modifiedBaseRepo, models.PullRequest{BaseRepo: modifiedBaseRepo}, "default")

Assert(t, success == true, "clone url mutation error")
Ok(t, err)
}
63 changes: 44 additions & 19 deletions server/events/mock_workingdir_test.go

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

20 changes: 14 additions & 6 deletions server/events/mocks/mock_event_parsing.go

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

63 changes: 44 additions & 19 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.

2 changes: 1 addition & 1 deletion server/events/post_workflow_hooks_command_runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ func (w *DefaultPostWorkflowHooksCommandRunner) RunPostHooks(
log.Debug("got workspace lock")
defer unlockFn()

repoDir, _, err := w.WorkingDir.Clone(headRepo, pull, DefaultWorkspace)
repoDir, err := w.WorkingDir.Clone(headRepo, pull, DefaultWorkspace)
if err != nil {
return err
}
Expand Down
14 changes: 7 additions & 7 deletions server/events/post_workflow_hooks_command_runner_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ func TestRunPostHooks_Clone(t *testing.T) {
postWh.GlobalCfg = globalCfg

When(postWhWorkingDirLocker.TryLock(testdata.GithubRepo.FullName, newPull.Num, events.DefaultWorkspace, events.DefaultRepoRelDir)).ThenReturn(unlockFn, nil)
When(postWhWorkingDir.Clone(testdata.GithubRepo, newPull, events.DefaultWorkspace)).ThenReturn(repoDir, false, nil)
When(postWhWorkingDir.Clone(testdata.GithubRepo, newPull, events.DefaultWorkspace)).ThenReturn(repoDir, nil)
When(whPostWorkflowHookRunner.Run(Any[models.WorkflowHookCommandContext](), Eq(testHook.RunCommand), Any[string](),
Any[string](), Eq(repoDir))).ThenReturn(result, runtimeDesc, nil)

Expand Down Expand Up @@ -214,7 +214,7 @@ func TestRunPostHooks_Clone(t *testing.T) {
postWh.GlobalCfg = globalCfg

When(postWhWorkingDirLocker.TryLock(testdata.GithubRepo.FullName, newPull.Num, events.DefaultWorkspace, events.DefaultRepoRelDir)).ThenReturn(unlockFn, nil)
When(postWhWorkingDir.Clone(testdata.GithubRepo, newPull, events.DefaultWorkspace)).ThenReturn(repoDir, false, errors.New("some error"))
When(postWhWorkingDir.Clone(testdata.GithubRepo, newPull, events.DefaultWorkspace)).ThenReturn(repoDir, errors.New("some error"))

err := postWh.RunPostHooks(ctx, cmd)

Expand Down Expand Up @@ -247,7 +247,7 @@ func TestRunPostHooks_Clone(t *testing.T) {
postWh.GlobalCfg = globalCfg

When(postWhWorkingDirLocker.TryLock(testdata.GithubRepo.FullName, newPull.Num, events.DefaultWorkspace, events.DefaultRepoRelDir)).ThenReturn(unlockFn, nil)
When(postWhWorkingDir.Clone(testdata.GithubRepo, newPull, events.DefaultWorkspace)).ThenReturn(repoDir, false, nil)
When(postWhWorkingDir.Clone(testdata.GithubRepo, newPull, 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"))

Expand Down Expand Up @@ -287,7 +287,7 @@ func TestRunPostHooks_Clone(t *testing.T) {
postWh.GlobalCfg = globalCfg

When(postWhWorkingDirLocker.TryLock(testdata.GithubRepo.FullName, newPull.Num, events.DefaultWorkspace, events.DefaultRepoRelDir)).ThenReturn(unlockFn, nil)
When(postWhWorkingDir.Clone(testdata.GithubRepo, newPull, events.DefaultWorkspace)).ThenReturn(repoDir, false, nil)
When(postWhWorkingDir.Clone(testdata.GithubRepo, newPull, events.DefaultWorkspace)).ThenReturn(repoDir, nil)
When(whPostWorkflowHookRunner.Run(Any[models.WorkflowHookCommandContext](), Eq(testHook.RunCommand),
Any[string](), Any[string](), Eq(repoDir))).ThenReturn(result, runtimeDesc, nil)

Expand Down Expand Up @@ -321,7 +321,7 @@ func TestRunPostHooks_Clone(t *testing.T) {
postWh.GlobalCfg = globalCfg

When(postWhWorkingDirLocker.TryLock(testdata.GithubRepo.FullName, newPull.Num, events.DefaultWorkspace, events.DefaultRepoRelDir)).ThenReturn(unlockFn, nil)
When(postWhWorkingDir.Clone(testdata.GithubRepo, newPull, events.DefaultWorkspace)).ThenReturn(repoDir, false, nil)
When(postWhWorkingDir.Clone(testdata.GithubRepo, newPull, events.DefaultWorkspace)).ThenReturn(repoDir, nil)
When(whPostWorkflowHookRunner.Run(Any[models.WorkflowHookCommandContext](), Eq(testHookWithShell.RunCommand),
Any[string](), Any[string](), Eq(repoDir))).ThenReturn(result, runtimeDesc, nil)

Expand Down Expand Up @@ -355,7 +355,7 @@ func TestRunPostHooks_Clone(t *testing.T) {
postWh.GlobalCfg = globalCfg

When(postWhWorkingDirLocker.TryLock(testdata.GithubRepo.FullName, newPull.Num, events.DefaultWorkspace, events.DefaultRepoRelDir)).ThenReturn(unlockFn, nil)
When(postWhWorkingDir.Clone(testdata.GithubRepo, newPull, events.DefaultWorkspace)).ThenReturn(repoDir, false, nil)
When(postWhWorkingDir.Clone(testdata.GithubRepo, newPull, events.DefaultWorkspace)).ThenReturn(repoDir, nil)
When(whPostWorkflowHookRunner.Run(Any[models.WorkflowHookCommandContext](), Eq(testHook.RunCommand),
Any[string](), Any[string](), Eq(repoDir))).ThenReturn(result, runtimeDesc, nil)

Expand Down Expand Up @@ -389,7 +389,7 @@ func TestRunPostHooks_Clone(t *testing.T) {
postWh.GlobalCfg = globalCfg

When(postWhWorkingDirLocker.TryLock(testdata.GithubRepo.FullName, newPull.Num, events.DefaultWorkspace, events.DefaultRepoRelDir)).ThenReturn(unlockFn, nil)
When(postWhWorkingDir.Clone(testdata.GithubRepo, newPull, events.DefaultWorkspace)).ThenReturn(repoDir, false, nil)
When(postWhWorkingDir.Clone(testdata.GithubRepo, newPull, events.DefaultWorkspace)).ThenReturn(repoDir, nil)
When(whPostWorkflowHookRunner.Run(Any[models.WorkflowHookCommandContext](),
Eq(testHookWithShellandShellArgs.RunCommand), Any[string](), Any[string](), Eq(repoDir))).ThenReturn(result, runtimeDesc, nil)

Expand Down
2 changes: 1 addition & 1 deletion server/events/pre_workflow_hooks_command_runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ func (w *DefaultPreWorkflowHooksCommandRunner) RunPreHooks(ctx *command.Context,
log.Debug("got workspace lock")
defer unlockFn()

repoDir, _, err := w.WorkingDir.Clone(headRepo, pull, DefaultWorkspace)
repoDir, err := w.WorkingDir.Clone(headRepo, pull, DefaultWorkspace)
if err != nil {
return err
}
Expand Down
Loading

0 comments on commit 580ea23

Please sign in to comment.