Repository: runatlantis/atlantis (language: go)

Issue:
Discard approvals by default in v0.22.0 due to missing logic
<!--- Please keep this note for the community --->

### Community Note

* Please vote on this issue by adding a 👍 [reaction](https://blog.github.com/2016-03-10-add-reactions-to-pull-requests-issues-and-comments/) to the original issue to help the community and maintainers prioritize this request. Searching for pre-existing feature requests helps us consolidate datapoints for identical requirements into a single place, thank you!
* Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request.
* If you are interested in working on this issue or have submitted a pull request, please leave a comment.

<!--- Thank you for keeping this note for the community --->

---

<!--- When filing a bug, please include the following headings if possible. Any example text in this template can be deleted. --->

### Overview of the Issue

<!--- Please describe the issue you are having and how you encountered the problem. --->
See https://github.com/runatlantis/atlantis/pull/2696#issuecomment-1369389741

The discarding of approvals is default on accident. There isn't any logic to enable or disable it despite the value of the flag 

### Reproduction Steps

<!--- 

In order to effectively and quickly resolve the issue, please provide exact steps that allow us the reproduce the problem. If no steps are provided, then it will likely take longer to get the issue resolved.
  
--->

### Logs

<!---

Provide log files from Atlantis server

<details>
  <summary>Logs</summary>

```
log output
```

</details>

--->

### Environment details

v0.22.0
<!---

If not already included, please provide the following:
- Atlantis version:
- If not running the latest Atlantis version have you tried to reproduce this issue on the latest version: 
- Atlantis flags:

Atlantis server-side config file:
```
# config file
```

Repo `atlantis.yaml` file:
```
# config file
```

Any other information you can provide about the environment/deployment.
--->


### Additional Context

<!---
Additional context on the problem. Docs, links to blogs, or other material that lead you to discover this issue or were helpful in troubleshooting the issue. 
--->
Workaround is to use v0.21.0

Edit: we need a check here

https://github.com/runatlantis/atlantis/blob/9595b230e859ef800d990fcde2193d3606525a15/server/events/plan_command_runner.go#L161-L168

Before a check can be added, we need to add a boolean here to `PlanCommandRunner`

https://github.com/runatlantis/atlantis/blob/9595b230e859ef800d990fcde2193d3606525a15/server/events/plan_command_runner.go#L48-L70

Before doing that, you'd need to pass the flag here

https://github.com/runatlantis/atlantis/blob/9595b230e859ef800d990fcde2193d3606525a15/server/server.go#L660-L668

We'd also need some tests with this flag and without this flag.

Honestly, it might be better to revert the pr for now and cut a v0.22.1 release.


Discussion / hints:

Fix patch:
diff --git a/server/events/plan_command_runner.go b/server/events/plan_command_runner.go
index 0344ffc366..89b9309acc 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 5907c5afbb..4d87277bbb 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)


Test patch:
diff --git a/server/controllers/events/events_controller_e2e_test.go b/server/controllers/events/events_controller_e2e_test.go
index 0cd8dfa458..32309fb93a 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 3cb6464897..16d0026523 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) {