diff --git a/server/controllers/events/events_controller_e2e_test.go b/server/controllers/events/events_controller_e2e_test.go index 0cd8dfa45..32309fb93 100644 --- a/server/controllers/events/events_controller_e2e_test.go +++ b/server/controllers/events/events_controller_e2e_test.go @@ -972,6 +972,7 @@ type setupOption struct { func setupE2E(t *testing.T, repoDir string, opt setupOption) (events_controllers.VCSEventsController, *vcsmocks.MockClient, *mocks.MockGithubPullGetter, *events.FileWorkspace) { allowForkPRs := false + discardApprovalOnPlan := true dataDir, binDir, cacheDir := mkSubDirs(t) // Mocks. @@ -1191,6 +1192,7 @@ func setupE2E(t *testing.T, repoDir string, opt setupOption) (events_controllers silenceNoProjects, boltdb, lockingClient, + discardApprovalOnPlan, ) e2ePullReqStatusFetcher := vcs.NewPullReqStatusFetcher(e2eVCSClient) diff --git a/server/events/command_runner_test.go b/server/events/command_runner_test.go index 3cb646489..16d002652 100644 --- a/server/events/command_runner_test.go +++ b/server/events/command_runner_test.go @@ -70,8 +70,27 @@ var importCommandRunner *events.ImportCommandRunner var preWorkflowHooksCommandRunner events.PreWorkflowHooksCommandRunner var postWorkflowHooksCommandRunner events.PostWorkflowHooksCommandRunner -func setup(t *testing.T) *vcsmocks.MockClient { +type TestConfig struct { + parallelPoolSize int + SilenceNoProjects bool + StatusName string + discardApprovalOnPlan bool +} + +func setup(t *testing.T, options ...func(testConfig *TestConfig)) *vcsmocks.MockClient { RegisterMockTestingT(t) + + testConfig := &TestConfig{ + parallelPoolSize: 1, + SilenceNoProjects: false, + StatusName: "atlantis-test", + discardApprovalOnPlan: false, + } + + for _, op := range options { + op(testConfig) + } + projectCommandBuilder = mocks.NewMockProjectCommandBuilder() eventParsing = mocks.NewMockEventParsing() vcsClient := vcsmocks.NewMockClient() @@ -107,15 +126,12 @@ func setup(t *testing.T) *vcsmocks.MockClient { GlobalAutomerge: false, } - parallelPoolSize := 1 - SilenceNoProjects := false - StatusName := "atlantis-test" policyCheckCommandRunner = events.NewPolicyCheckCommandRunner( dbUpdater, pullUpdater, commitUpdater, projectCommandRunner, - parallelPoolSize, + testConfig.parallelPoolSize, false, false, ) @@ -133,10 +149,11 @@ func setup(t *testing.T) *vcsmocks.MockClient { pullUpdater, policyCheckCommandRunner, autoMerger, - parallelPoolSize, - SilenceNoProjects, + testConfig.parallelPoolSize, + testConfig.SilenceNoProjects, defaultBoltDB, lockingLocker, + testConfig.discardApprovalOnPlan, ) pullReqStatusFetcher := vcs.NewPullReqStatusFetcher(vcsClient) @@ -152,10 +169,10 @@ func setup(t *testing.T) *vcsmocks.MockClient { pullUpdater, dbUpdater, defaultBoltDB, - parallelPoolSize, - SilenceNoProjects, + testConfig.parallelPoolSize, + testConfig.SilenceNoProjects, false, - StatusName, + testConfig.StatusName, pullReqStatusFetcher, ) @@ -165,22 +182,22 @@ func setup(t *testing.T) *vcsmocks.MockClient { projectCommandRunner, pullUpdater, dbUpdater, - SilenceNoProjects, + testConfig.SilenceNoProjects, false, ) unlockCommandRunner = events.NewUnlockCommandRunner( deleteLockCommand, vcsClient, - SilenceNoProjects, + testConfig.SilenceNoProjects, ) versionCommandRunner := events.NewVersionCommandRunner( pullUpdater, projectCommandBuilder, projectCommandRunner, - parallelPoolSize, - SilenceNoProjects, + testConfig.parallelPoolSize, + testConfig.SilenceNoProjects, ) importCommandRunner = events.NewImportCommandRunner( @@ -226,6 +243,7 @@ func setup(t *testing.T) *vcsmocks.MockClient { PostWorkflowHooksCommandRunner: postWorkflowHooksCommandRunner, PullStatusFetcher: defaultBoltDB, } + return vcsClient } @@ -609,7 +627,8 @@ func TestRunSpecificPlanCommandDoesnt_DeletePlans(t *testing.T) { // Test that if one plan fails and we are using automerge, that // we delete the plans. func TestRunAutoplanCommandWithError_DeletePlans(t *testing.T) { - setup(t) + vcsClient := setup(t) + tmp := t.TempDir() boltDB, err := db.New(tmp) Ok(t, err) @@ -652,6 +671,35 @@ func TestRunAutoplanCommandWithError_DeletePlans(t *testing.T) { ch.RunAutoplanCommand(fixtures.GithubRepo, fixtures.GithubRepo, fixtures.Pull, fixtures.User) // gets called twice: the first time before the plan starts, the second time after the plan errors pendingPlanFinder.VerifyWasCalled(Times(2)).DeletePlans(tmp) + + vcsClient.VerifyWasCalled(Times(0)).DiscardReviews(matchers.AnyModelsRepo(), matchers.AnyModelsPullRequest()) +} + +func TestRunGenericPlanCommand_DiscardApprovals(t *testing.T) { + vcsClient := setup(t, func(testConfig *TestConfig) { + testConfig.discardApprovalOnPlan = true + }) + + tmp := t.TempDir() + boltDB, err := db.New(tmp) + Ok(t, err) + dbUpdater.Backend = boltDB + applyCommandRunner.Backend = boltDB + autoMerger.GlobalAutomerge = true + defer func() { autoMerger.GlobalAutomerge = false }() + + When(projectCommandRunner.Plan(matchers.AnyCommandProjectContext())).ThenReturn(command.ProjectResult{PlanSuccess: &models.PlanSuccess{}}) + When(workingDir.GetPullDir(matchers.AnyModelsRepo(), matchers.AnyModelsPullRequest())).ThenReturn(tmp, nil) + pull := &github.PullRequest{State: github.String("open")} + modelPull := models.PullRequest{BaseRepo: fixtures.GithubRepo, State: models.OpenPullState, Num: fixtures.Pull.Num} + When(githubGetter.GetPullRequest(fixtures.GithubRepo, fixtures.Pull.Num)).ThenReturn(pull, nil) + When(eventParsing.ParseGithubPull(pull)).ThenReturn(modelPull, modelPull.BaseRepo, fixtures.GithubRepo, nil) + fixtures.Pull.BaseRepo = fixtures.GithubRepo + ch.RunCommentCommand(fixtures.GithubRepo, nil, nil, fixtures.User, fixtures.Pull.Num, &events.CommentCommand{Name: command.Plan}) + pendingPlanFinder.VerifyWasCalledOnce().DeletePlans(tmp) + lockingLocker.VerifyWasCalledOnce().UnlockByPull(fixtures.Pull.BaseRepo.FullName, fixtures.Pull.Num) + + vcsClient.VerifyWasCalledOnce().DiscardReviews(matchers.AnyModelsRepo(), matchers.AnyModelsPullRequest()) } func TestFailedApprovalCreatesFailedStatusUpdate(t *testing.T) { diff --git a/server/events/plan_command_runner.go b/server/events/plan_command_runner.go index 0344ffc36..89b9309ac 100644 --- a/server/events/plan_command_runner.go +++ b/server/events/plan_command_runner.go @@ -24,6 +24,7 @@ func NewPlanCommandRunner( SilenceNoProjects bool, pullStatusFetcher PullStatusFetcher, lockingLocker locking.Locker, + discardApprovalOnPlan bool, ) *PlanCommandRunner { return &PlanCommandRunner{ silenceVCSStatusNoPlans: silenceVCSStatusNoPlans, @@ -42,6 +43,7 @@ func NewPlanCommandRunner( SilenceNoProjects: SilenceNoProjects, pullStatusFetcher: pullStatusFetcher, lockingLocker: lockingLocker, + DiscardApprovalOnPlan: discardApprovalOnPlan, } } @@ -68,6 +70,9 @@ type PlanCommandRunner struct { parallelPoolSize int pullStatusFetcher PullStatusFetcher lockingLocker locking.Locker + // DiscardApprovalOnPlan controls if all already existing approvals should be removed/dismissed before executing + // a plan. + DiscardApprovalOnPlan bool } func (p *PlanCommandRunner) runAutoplan(ctx *command.Context) { @@ -163,8 +168,10 @@ func (p *PlanCommandRunner) run(ctx *command.Context, cmd *CommentCommand) { baseRepo := ctx.Pull.BaseRepo pull := ctx.Pull - if err = p.pullUpdater.VCSClient.DiscardReviews(baseRepo, pull); err != nil { - ctx.Log.Err("failed to remove approvals: %s", err) + if p.DiscardApprovalOnPlan { + if err = p.pullUpdater.VCSClient.DiscardReviews(baseRepo, pull); err != nil { + ctx.Log.Err("failed to remove approvals: %s", err) + } } if err = p.commitStatusUpdater.UpdateCombined(baseRepo, pull, models.PendingCommitStatus, command.Plan); err != nil { diff --git a/server/server.go b/server/server.go index 5907c5afb..4d87277bb 100644 --- a/server/server.go +++ b/server/server.go @@ -684,6 +684,7 @@ func NewServer(userConfig UserConfig, config Config) (*Server, error) { userConfig.SilenceNoProjects, backend, lockingClient, + userConfig.DiscardApprovalOnPlanFlag, ) pullReqStatusFetcher := vcs.NewPullReqStatusFetcher(vcsClient)