Merge pull request #1225 from lyft/issue-1193

[1193] Fix merge strategy for gh apps

server/events/command_context.go

@@ -21,8 +21,6 @@ import (
 // CommandContext represents the context of a command that should be executed
 // for a pull request.
 type CommandContext struct {
-	// BaseRepo is the repository that the pull request will be merged into.
-	BaseRepo models.Repo
 	// HeadRepo is the repository that is getting merged into the BaseRepo.
 	// If the pull request branch is from the same repository then HeadRepo will
 	// be the same as BaseRepo.

server/events/command_runner.go

@@ -126,7 +126,6 @@ func (c *DefaultCommandRunner) RunAutoplanCommand(baseRepo models.Repo, headRepo
 		Log:      log,
 		Pull:     pull,
 		HeadRepo: headRepo,
-		BaseRepo: baseRepo,
 	}
 	if !c.validateCtxAndComment(ctx) {
 		return
@@ -137,7 +136,7 @@ func (c *DefaultCommandRunner) RunAutoplanCommand(baseRepo models.Repo, headRepo
 
 	projectCmds, err := c.ProjectCommandBuilder.BuildAutoplanCommands(ctx)
 	if err != nil {
-		if statusErr := c.CommitStatusUpdater.UpdateCombined(ctx.BaseRepo, ctx.Pull, models.FailedCommitStatus, models.PlanCommand); statusErr != nil {
+		if statusErr := c.CommitStatusUpdater.UpdateCombined(ctx.Pull.BaseRepo, ctx.Pull, models.FailedCommitStatus, models.PlanCommand); statusErr != nil {
 			ctx.Log.Warn("unable to update commit status: %s", statusErr)
 		}
 
@@ -159,7 +158,7 @@ func (c *DefaultCommandRunner) RunAutoplanCommand(baseRepo models.Repo, headRepo
 	}
 
 	// At this point we are sure Atlantis has work to do, so set commit status to pending
-	if err := c.CommitStatusUpdater.UpdateCombined(ctx.BaseRepo, ctx.Pull, models.PendingCommitStatus, models.PlanCommand); err != nil {
+	if err := c.CommitStatusUpdater.UpdateCombined(ctx.Pull.BaseRepo, ctx.Pull, models.PendingCommitStatus, models.PlanCommand); err != nil {
 		ctx.Log.Warn("unable to update commit status: %s", err)
 	}
 
@@ -246,7 +245,6 @@ func (c *DefaultCommandRunner) RunCommentCommand(baseRepo models.Repo, maybeHead
 		Log:      log,
 		Pull:     pull,
 		HeadRepo: headRepo,
-		BaseRepo: baseRepo,
 	}
 	if !c.validateCtxAndComment(ctx) {
 		return
@@ -296,7 +294,7 @@ func (c *DefaultCommandRunner) RunCommentCommand(baseRepo models.Repo, maybeHead
 		return
 	}
 	if err != nil {
-		if statusErr := c.CommitStatusUpdater.UpdateCombined(ctx.BaseRepo, ctx.Pull, models.FailedCommitStatus, cmd.CommandName()); statusErr != nil {
+		if statusErr := c.CommitStatusUpdater.UpdateCombined(ctx.Pull.BaseRepo, ctx.Pull, models.FailedCommitStatus, cmd.CommandName()); statusErr != nil {
 			ctx.Log.Warn("unable to update commit status: %s", statusErr)
 		}
 		c.updatePull(ctx, cmd, CommandResult{Error: err})
@@ -366,7 +364,7 @@ func (c *DefaultCommandRunner) updateCommitStatus(ctx *CommandContext, cmd model
 		}
 	}
 
-	if err := c.CommitStatusUpdater.UpdateCombinedCount(ctx.BaseRepo, ctx.Pull, status, cmd, numSuccess, len(pullStatus.Projects)); err != nil {
+	if err := c.CommitStatusUpdater.UpdateCombinedCount(ctx.Pull.BaseRepo, ctx.Pull, status, cmd, numSuccess, len(pullStatus.Projects)); err != nil {
 		ctx.Log.Warn("unable to update commit status: %s", err)
 	}
 }
@@ -381,7 +379,7 @@ func (c *DefaultCommandRunner) automerge(ctx *CommandContext, pullStatus models.
 	}
 
 	// Comment that we're automerging the pull request.
-	if err := c.VCSClient.CreateComment(ctx.BaseRepo, ctx.Pull.Num, automergeComment, models.ApplyCommand.String()); err != nil {
+	if err := c.VCSClient.CreateComment(ctx.Pull.BaseRepo, ctx.Pull.Num, automergeComment, models.ApplyCommand.String()); err != nil {
 		ctx.Log.Err("failed to comment about automerge: %s", err)
 		// Commenting isn't required so continue.
 	}
@@ -394,7 +392,7 @@ func (c *DefaultCommandRunner) automerge(ctx *CommandContext, pullStatus models.
 		ctx.Log.Err("automerging failed: %s", err)
 
 		failureComment := fmt.Sprintf("Automerging failed:\n```\n%s\n```", err)
-		if commentErr := c.VCSClient.CreateComment(ctx.BaseRepo, ctx.Pull.Num, failureComment, models.ApplyCommand.String()); commentErr != nil {
+		if commentErr := c.VCSClient.CreateComment(ctx.Pull.BaseRepo, ctx.Pull.Num, failureComment, models.ApplyCommand.String()); commentErr != nil {
 			ctx.Log.Err("failed to comment about automerge failing: %s", err)
 		}
 	}
@@ -498,20 +496,20 @@ func (c *DefaultCommandRunner) buildLogger(repoFullName string, pullNum int) *lo
 }
 
 func (c *DefaultCommandRunner) validateCtxAndComment(ctx *CommandContext) bool {
-	if !c.AllowForkPRs && ctx.HeadRepo.Owner != ctx.BaseRepo.Owner {
+	if !c.AllowForkPRs && ctx.HeadRepo.Owner != ctx.Pull.BaseRepo.Owner {
 		if c.SilenceForkPRErrors {
 			return false
 		}
 		ctx.Log.Info("command was run on a fork pull request which is disallowed")
-		if err := c.VCSClient.CreateComment(ctx.BaseRepo, ctx.Pull.Num, fmt.Sprintf("Atlantis commands can't be run on fork pull requests. To enable, set --%s  or, to disable this message, set --%s", c.AllowForkPRsFlag, c.SilenceForkPRErrorsFlag), ""); err != nil {
+		if err := c.VCSClient.CreateComment(ctx.Pull.BaseRepo, ctx.Pull.Num, fmt.Sprintf("Atlantis commands can't be run on fork pull requests. To enable, set --%s  or, to disable this message, set --%s", c.AllowForkPRsFlag, c.SilenceForkPRErrorsFlag), ""); err != nil {
 			ctx.Log.Err("unable to comment: %s", err)
 		}
 		return false
 	}
 
 	if ctx.Pull.State != models.OpenPullState {
 		ctx.Log.Info("command was run on closed pull request")
-		if err := c.VCSClient.CreateComment(ctx.BaseRepo, ctx.Pull.Num, "Atlantis commands can't be run on closed pull requests", ""); err != nil {
+		if err := c.VCSClient.CreateComment(ctx.Pull.BaseRepo, ctx.Pull.Num, "Atlantis commands can't be run on closed pull requests", ""); err != nil {
 			ctx.Log.Err("unable to comment: %s", err)
 		}
 		return false
@@ -531,13 +529,13 @@ func (c *DefaultCommandRunner) updatePull(ctx *CommandContext, command PullComma
 	// clutter in a pull/merge request. This will not delete the comment, since the
 	// comment trail may be useful in auditing or backtracing problems.
 	if c.HidePrevPlanComments {
-		if err := c.VCSClient.HidePrevPlanComments(ctx.BaseRepo, ctx.Pull.Num); err != nil {
+		if err := c.VCSClient.HidePrevPlanComments(ctx.Pull.BaseRepo, ctx.Pull.Num); err != nil {
 			ctx.Log.Err("unable to hide old comments: %s", err)
 		}
 	}
 
-	comment := c.MarkdownRenderer.Render(res, command.CommandName(), ctx.Log.History.String(), command.IsVerbose(), ctx.BaseRepo.VCSHost.Type)
-	if err := c.VCSClient.CreateComment(ctx.BaseRepo, ctx.Pull.Num, comment, command.CommandName().String()); err != nil {
+	comment := c.MarkdownRenderer.Render(res, command.CommandName(), ctx.Log.History.String(), command.IsVerbose(), ctx.Pull.BaseRepo.VCSHost.Type)
+	if err := c.VCSClient.CreateComment(ctx.Pull.BaseRepo, ctx.Pull.Num, comment, command.CommandName().String()); err != nil {
 		ctx.Log.Err("unable to comment: %s", err)
 	}
 }
@@ -560,7 +558,7 @@ func (c *DefaultCommandRunner) logPanics(baseRepo models.Repo, pullNum int, logg
 
 // deletePlans deletes all plans generated in this ctx.
 func (c *DefaultCommandRunner) deletePlans(ctx *CommandContext) {
-	pullDir, err := c.WorkingDir.GetPullDir(ctx.BaseRepo, ctx.Pull)
+	pullDir, err := c.WorkingDir.GetPullDir(ctx.Pull.BaseRepo, ctx.Pull)
 	if err != nil {
 		ctx.Log.Err("getting pull dir: %s", err)
 	}

server/events/command_runner_test.go

@@ -155,7 +155,10 @@ func TestRunCommentCommand_ForkPRDisabled(t *testing.T) {
 	ch.AllowForkPRs = false
 	ch.SilenceForkPRErrors = false
 	var pull github.PullRequest
-	modelPull := models.PullRequest{State: models.OpenPullState}
+	modelPull := models.PullRequest{
+		BaseRepo: fixtures.GithubRepo,
+		State:    models.OpenPullState,
+	}
 	When(githubGetter.GetPullRequest(fixtures.GithubRepo, fixtures.Pull.Num)).ThenReturn(&pull, nil)
 
 	headRepo := fixtures.GithubRepo
@@ -174,7 +177,7 @@ func TestRunCommentCommand_ForkPRDisabled_SilenceEnabled(t *testing.T) {
 	ch.AllowForkPRs = false // by default it's false so don't need to reset
 	ch.SilenceForkPRErrors = true
 	var pull github.PullRequest
-	modelPull := models.PullRequest{State: models.OpenPullState}
+	modelPull := models.PullRequest{BaseRepo: fixtures.GithubRepo, State: models.OpenPullState}
 	When(githubGetter.GetPullRequest(fixtures.GithubRepo, fixtures.Pull.Num)).ThenReturn(&pull, nil)
 
 	headRepo := fixtures.GithubRepo
@@ -191,7 +194,7 @@ func TestRunCommentCommand_DisableApplyAllDisabled(t *testing.T) {
 		" comment saying that this is not allowed")
 	vcsClient := setup(t)
 	ch.DisableApplyAll = true
-	modelPull := models.PullRequest{State: models.OpenPullState}
+	modelPull := models.PullRequest{BaseRepo: fixtures.GithubRepo, State: models.OpenPullState}
 	ch.RunCommentCommand(fixtures.GithubRepo, nil, nil, fixtures.User, modelPull.Num, &events.CommentCommand{Name: models.ApplyCommand})
 	vcsClient.VerifyWasCalledOnce().CreateComment(fixtures.GithubRepo, modelPull.Num, "**Error:** Running `atlantis apply` without flags is disabled. You must specify which project to apply via the `-d <dir>`, `-w <workspace>` or `-p <project name>` flags.", "apply")
 }
@@ -219,7 +222,7 @@ func TestRunCommentCommand_ClosedPull(t *testing.T) {
 	pull := &github.PullRequest{
 		State: github.String("closed"),
 	}
-	modelPull := models.PullRequest{State: models.ClosedPullState}
+	modelPull := models.PullRequest{BaseRepo: fixtures.GithubRepo, State: models.ClosedPullState}
 	When(githubGetter.GetPullRequest(fixtures.GithubRepo, fixtures.Pull.Num)).ThenReturn(pull, nil)
 	When(eventParsing.ParseGithubPull(pull)).ThenReturn(modelPull, modelPull.BaseRepo, fixtures.GithubRepo, nil)
 
@@ -235,7 +238,7 @@ func TestRunUnlockCommand_VCSComment(t *testing.T) {
 	pull := &github.PullRequest{
 		State: github.String("open"),
 	}
-	modelPull := models.PullRequest{State: models.OpenPullState}
+	modelPull := models.PullRequest{BaseRepo: fixtures.GithubRepo, State: models.OpenPullState}
 	When(githubGetter.GetPullRequest(fixtures.GithubRepo, fixtures.Pull.Num)).ThenReturn(pull, nil)
 	When(eventParsing.ParseGithubPull(pull)).ThenReturn(modelPull, modelPull.BaseRepo, fixtures.GithubRepo, nil)
 
@@ -253,7 +256,7 @@ func TestRunUnlockCommandFail_VCSComment(t *testing.T) {
 	pull := &github.PullRequest{
 		State: github.String("open"),
 	}
-	modelPull := models.PullRequest{State: models.OpenPullState}
+	modelPull := models.PullRequest{BaseRepo: fixtures.GithubRepo, State: models.OpenPullState}
 	When(githubGetter.GetPullRequest(fixtures.GithubRepo, fixtures.Pull.Num)).ThenReturn(pull, nil)
 	When(eventParsing.ParseGithubPull(pull)).ThenReturn(modelPull, modelPull.BaseRepo, fixtures.GithubRepo, nil)
 	When(deleteLockCommand.DeleteLocksByPull(fixtures.GithubRepo.FullName, fixtures.Pull.Num)).ThenReturn(errors.New("err"))
@@ -301,6 +304,7 @@ func TestRunAutoplanCommand_DeletePlans(t *testing.T) {
 
 	When(workingDir.GetPullDir(matchers.AnyModelsRepo(), matchers.AnyModelsPullRequest())).
 		ThenReturn(tmp, nil)
+	fixtures.Pull.BaseRepo = fixtures.GithubRepo
 	ch.RunAutoplanCommand(fixtures.GithubRepo, fixtures.GithubRepo, fixtures.Pull, fixtures.User)
 	pendingPlanFinder.VerifyWasCalledOnce().DeletePlans(tmp)
 }
@@ -312,7 +316,7 @@ func TestApplyWithAutoMerge_VSCMerge(t *testing.T) {
 	pull := &github.PullRequest{
 		State: github.String("open"),
 	}
-	modelPull := models.PullRequest{State: models.OpenPullState}
+	modelPull := models.PullRequest{BaseRepo: fixtures.GithubRepo, State: models.OpenPullState}
 	When(githubGetter.GetPullRequest(fixtures.GithubRepo, fixtures.Pull.Num)).ThenReturn(pull, nil)
 	When(eventParsing.ParseGithubPull(pull)).ThenReturn(modelPull, modelPull.BaseRepo, fixtures.GithubRepo, nil)
 	ch.GlobalAutomerge = true
@@ -387,6 +391,7 @@ func TestRunAutoplanCommand_DrainOngoing(t *testing.T) {
 func TestRunAutoplanCommand_DrainNotOngoing(t *testing.T) {
 	t.Log("if drain is not ongoing then remove ongoing operation must be called even if panic occured")
 	setup(t)
+	fixtures.Pull.BaseRepo = fixtures.GithubRepo
 	When(projectCommandBuilder.BuildAutoplanCommands(matchers.AnyPtrToEventsCommandContext())).ThenPanic("panic test - if you're seeing this in a test failure this isn't the failing test")
 	ch.RunAutoplanCommand(fixtures.GithubRepo, fixtures.GithubRepo, fixtures.Pull, fixtures.User)
 	projectCommandBuilder.VerifyWasCalledOnce().BuildAutoplanCommands(matchers.AnyPtrToEventsCommandContext())

server/events/github_app_working_dir.go

@@ -21,7 +21,7 @@ type GithubAppWorkingDir struct {
 }
 
 // Clone writes a fresh token for Github App authentication
-func (g *GithubAppWorkingDir) Clone(log *logging.SimpleLogger, baseRepo models.Repo, headRepo models.Repo, p models.PullRequest, workspace string) (string, bool, error) {
+func (g *GithubAppWorkingDir) Clone(log *logging.SimpleLogger, headRepo models.Repo, p models.PullRequest, workspace string) (string, bool, error) {
 
 	log.Info("Refreshing git tokens for Github App")
 
@@ -40,8 +40,16 @@ func (g *GithubAppWorkingDir) Clone(log *logging.SimpleLogger, baseRepo models.R
 		return "", false, err
 	}
 
+	baseRepo := &p.BaseRepo
+
+	// Realistically, this is a super brittle way of supporting clones using gh app installation tokens
+	// This URL should be built during Repo creation and the struct should be immutable going forward.
+	// Doing this requires a larger refactor however, and can probably be coupled with supporting > 1 installation
 	authURL := fmt.Sprintf("://x-access-token:%s", token)
 	baseRepo.CloneURL = strings.Replace(baseRepo.CloneURL, "://:", authURL, 1)
+	baseRepo.SanitizedCloneURL = strings.Replace(baseRepo.SanitizedCloneURL, "://:", "://x-access-token:", 1)
 	headRepo.CloneURL = strings.Replace(headRepo.CloneURL, "://:", authURL, 1)
-	return g.WorkingDir.Clone(log, baseRepo, headRepo, p, workspace)
+	headRepo.SanitizedCloneURL = strings.Replace(baseRepo.SanitizedCloneURL, "://:", "://x-access-token:", 1)
+
+	return g.WorkingDir.Clone(log, headRepo, p, workspace)
 }

server/events/github_app_working_dir_test.go

@@ -4,10 +4,13 @@ import (
 	"fmt"
 	"testing"
 
+	. "github.com/petergtz/pegomock"
 	"github.com/runatlantis/atlantis/server/events"
+	eventMocks "github.com/runatlantis/atlantis/server/events/mocks"
 	"github.com/runatlantis/atlantis/server/events/models"
 	"github.com/runatlantis/atlantis/server/events/vcs"
 	"github.com/runatlantis/atlantis/server/events/vcs/fixtures"
+	vcsMocks "github.com/runatlantis/atlantis/server/events/vcs/mocks"
 	. "github.com/runatlantis/atlantis/testing"
 )
 
@@ -46,7 +49,8 @@ func TestClone_GithubAppNoneExisting(t *testing.T) {
 		GithubHostname: testServer,
 	}
 
-	cloneDir, _, err := gwd.Clone(nil, models.Repo{}, models.Repo{}, models.PullRequest{
+	cloneDir, _, err := gwd.Clone(nil, models.Repo{}, models.PullRequest{
+		BaseRepo:   models.Repo{},
 		HeadBranch: "branch",
 	}, "default")
 	Ok(t, err)
@@ -55,3 +59,40 @@ func TestClone_GithubAppNoneExisting(t *testing.T) {
 	actCommit := runCmd(t, cloneDir, "git", "rev-parse", "HEAD")
 	Equals(t, expCommit, actCommit)
 }
+
+func TestClone_GithubAppSetsCorrectUrl(t *testing.T) {
+	workingDir := eventMocks.NewMockWorkingDir()
+
+	credentials := vcsMocks.NewMockGithubCredentials()
+
+	ghAppWorkingDir := events.GithubAppWorkingDir{
+		WorkingDir:     workingDir,
+		Credentials:    credentials,
+		GithubHostname: "some-host",
+	}
+
+	baseRepo, _ := models.NewRepo(
+		models.Github,
+		"runatlantis/atlantis",
+		"https://github.com/runatlantis/atlantis.git",
+
+		// user and token have to be blank otherwise this proxy wouldn't be invoked to begin with
+		"",
+		"",
+	)
+
+	headRepo := baseRepo
+
+	modifiedBaseRepo := baseRepo
+	modifiedBaseRepo.CloneURL = "https://x-access-token:[email protected]/runatlantis/atlantis.git"
+	modifiedBaseRepo.SanitizedCloneURL = "https://x-access-token:<redacted>@github.com/runatlantis/atlantis.git"
+
+	When(credentials.GetToken()).ThenReturn("token", nil)
+	When(workingDir.Clone(nil, modifiedBaseRepo, models.PullRequest{BaseRepo: modifiedBaseRepo}, "default")).ThenReturn(
+		"", true, nil,
+	)
+
+	_, success, _ := ghAppWorkingDir.Clone(nil, headRepo, models.PullRequest{BaseRepo: baseRepo}, "default")
+
+	Assert(t, success == true, "clone url mutation error")
+}