fix(discardApprovalOnPlan): add missing conditional and tests (runatl…

…antis#2913) * fix(discardApprovalOnPlan): add missing conditional and tests * tests(discardApprovalOnPlan): match for any Pull request or repo for turned off discarding * Apply suggestions from code review Co-authored-by: nitrocode <[email protected]> * Update server/events/plan_command_runner.go * Update server/events/plan_command_runner.go Co-authored-by: nitrocode <[email protected]>
vincentgna · Jan 3, 2023 · a508bdb · a508bdb
1 parent 8b9a70d
commit a508bdb
Show file tree

Hide file tree

Showing 4 changed files with 75 additions and 17 deletions.
diff --git 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
@@ -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
@@ -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
@@ -684,6 +684,7 @@ func NewServer(userConfig UserConfig, config Config) (*Server, error) {
 		userConfig.SilenceNoProjects,
 		backend,
 		lockingClient,
+		userConfig.DiscardApprovalOnPlanFlag,
 	)
 
 	pullReqStatusFetcher := vcs.NewPullReqStatusFetcher(vcsClient)