feat: Add an option to discard PR approvals after a plan

cmd/server.go

@@ -82,6 +82,7 @@ const (
 	DisableGlobalApplyLockFlag       = "disable-global-apply-lock"
 	DisableUnlockLabelFlag           = "disable-unlock-label"
 	DiscardApprovalOnPlanFlag        = "discard-approval-on-plan"
+	DiscardApprovalAfterPlanFlag     = "discard-approval-after-plan"
 	EmojiReaction                    = "emoji-reaction"
 	EnableDiffMarkdownFormat         = "enable-diff-markdown-format"
 	EnablePolicyChecksFlag           = "enable-policy-checks"
@@ -519,7 +520,11 @@ var boolFlags = map[string]boolFlag{
 		description: "Disable atlantis global apply lock in UI",
 	},
 	DiscardApprovalOnPlanFlag: {
-		description:  "Enables the discarding of approval if a new plan has been executed. Currently only Github is supported",
+		description:  "Enables the discarding of approval if a new plan has been executed. Currently supported on GitHub, GitLab, and Gitea",
+		defaultValue: false,
+	},
+	DiscardApprovalAfterPlanFlag: {
+		description:  "Discard approval after plan has been executed (regardless of trigger). Currently supported on GitHub, GitLab, and Gitea",
 		defaultValue: false,
 	},
 	EnablePolicyChecksFlag: {

cmd/server_test.go

@@ -83,6 +83,7 @@ var testFlags = map[string]interface{}{
 	DisableRepoLockingFlag:           true,
 	DisableGlobalApplyLockFlag:       false,
 	DiscardApprovalOnPlanFlag:        true,
+	DiscardApprovalAfterPlanFlag:     true,
 	EmojiReaction:                    "eyes",
 	ExecutableName:                   "atlantis",
 	FailOnPreWorkflowHookError:       false,

runatlantis.io/docs/server-configuration.md

@@ -493,6 +493,17 @@ ATLANTIS_DISABLE_UNLOCK_LABEL="do-not-unlock"
 
 Stops atlantis from unlocking a pull request with this label. Defaults to "" (feature disabled).
 
+### `--discard-approval-after-plan`
+
+```bash
+atlantis server --discard-approval-after-plan
+# or
+ATLANTIS_DISCARD_APPROVAL_AFTER_PLAN=true
+```
+
+If set, discard approval **after** a plan has been executed (regardless of whether it's triggered via a comment or from the PR being newly created). This addresses race conditions where someone could approve a plan when a PR is immediately opened and the plan is not ready yet. Currently supported on GitHub, GitLab, and Gitea. For GitLab a bot, group or project token is required for this feature.
+ Reference: [reset-approvals-of-a-merge-request](https://docs.gitlab.com/api/merge_request_approvals/#reset-approvals-of-a-merge-request)
+
 ### `--discard-approval-on-plan` <Badge text="v0.29.0+" type="info"/>
 
 ```bash
@@ -501,7 +512,7 @@ atlantis server --discard-approval-on-plan
 ATLANTIS_DISCARD_APPROVAL_ON_PLAN=true
 ```
 
-If set, discard approval if a new plan has been executed. Currently only supported on GitHub and GitLab. For GitLab a bot, group or project token is required for this feature.  
+If set, discard approval if a new plan has been executed. Currently supported on GitHub, GitLab, and Gitea. For GitLab a bot, group or project token is required for this feature.  
  Reference: [reset-approvals-of-a-merge-request](https://docs.gitlab.com/api/merge_request_approvals/#reset-approvals-of-a-merge-request)
 
 ### `--emoji-reaction` <Badge text="v0.29.0+" type="info"/>

server/controllers/events/events_controller_e2e_test.go

@@ -1299,6 +1299,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
+	discardApprovalAfterPlan := false
 	dataDir, binDir, cacheDir := mkSubDirs(t)
 	// Mocks.
 	e2eVCSClient := vcsmocks.NewMockClient()
@@ -1561,6 +1562,7 @@ func setupE2E(t *testing.T, repoDir string, opt setupOption) (events_controllers
 		boltdb,
 		lockingClient,
 		discardApprovalOnPlan,
+		discardApprovalAfterPlan,
 		e2ePullReqStatusFetcher,
 	)
 

server/events/command_runner_test.go

@@ -76,6 +76,7 @@ type TestConfig struct {
 	silenceVCSStatusNoProjects bool
 	StatusName                 string
 	discardApprovalOnPlan      bool
+	discardApprovalAfterPlan   bool
 	backend                    locking.Backend
 	DisableUnlockLabel         string
 }
@@ -92,12 +93,13 @@ func setup(t *testing.T, options ...func(testConfig *TestConfig)) *vcsmocks.Mock
 	Ok(t, err)
 
 	testConfig := &TestConfig{
-		parallelPoolSize:      1,
-		SilenceNoProjects:     false,
-		StatusName:            "atlantis-test",
-		discardApprovalOnPlan: false,
-		backend:               defaultBoltDB,
-		DisableUnlockLabel:    "do-not-unlock",
+		parallelPoolSize:         1,
+		SilenceNoProjects:        false,
+		StatusName:               "atlantis-test",
+		discardApprovalOnPlan:    false,
+		discardApprovalAfterPlan: false,
+		backend:                  defaultBoltDB,
+		DisableUnlockLabel:       "do-not-unlock",
 	}
 
 	for _, op := range options {
@@ -165,6 +167,7 @@ func setup(t *testing.T, options ...func(testConfig *TestConfig)) *vcsmocks.Mock
 		testConfig.backend,
 		lockingLocker,
 		testConfig.discardApprovalOnPlan,
+		testConfig.discardApprovalAfterPlan,
 		pullReqStatusFetcher,
 	)
 
@@ -1069,6 +1072,137 @@ func TestRunGenericPlanCommand_DiscardApprovals(t *testing.T) {
 	vcsClient.VerifyWasCalledOnce().DiscardReviews(Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest]())
 }
 
+func TestRunGenericPlanCommand_DiscardApprovalsAfterPlan(t *testing.T) {
+	vcsClient := setup(t, func(testConfig *TestConfig) {
+		testConfig.discardApprovalAfterPlan = true
+	})
+
+	tmp := t.TempDir()
+	boltDB, err := db.New(tmp)
+	t.Cleanup(func() {
+		boltDB.Close()
+	})
+	Ok(t, err)
+	dbUpdater.Backend = boltDB
+	applyCommandRunner.Backend = boltDB
+	autoMerger.GlobalAutomerge = true
+	defer func() { autoMerger.GlobalAutomerge = false }()
+
+	When(projectCommandBuilder.BuildPlanCommands(Any[*command.Context](), Any[*events.CommentCommand]())).ThenReturn([]command.ProjectContext{
+		{
+			CommandName: command.Plan,
+		},
+	}, nil)
+	When(projectCommandRunner.Plan(Any[command.ProjectContext]())).ThenReturn(command.ProjectResult{PlanSuccess: &models.PlanSuccess{}})
+	When(workingDir.GetPullDir(Any[models.Repo](), Any[models.PullRequest]())).ThenReturn(tmp, nil)
+	pull := &github.PullRequest{State: github.Ptr("open")}
+	modelPull := models.PullRequest{BaseRepo: testdata.GithubRepo, State: models.OpenPullState, Num: testdata.Pull.Num}
+	When(githubGetter.GetPullRequest(Any[logging.SimpleLogging](), Eq(testdata.GithubRepo), Eq(testdata.Pull.Num))).ThenReturn(pull, nil)
+	When(eventParsing.ParseGithubPull(Any[logging.SimpleLogging](), Eq(pull))).ThenReturn(modelPull, modelPull.BaseRepo, testdata.GithubRepo, nil)
+	testdata.Pull.BaseRepo = testdata.GithubRepo
+	ch.RunCommentCommand(testdata.GithubRepo, nil, nil, testdata.User, testdata.Pull.Num, &events.CommentCommand{Name: command.Plan})
+	pendingPlanFinder.VerifyWasCalledOnce().DeletePlans(tmp)
+
+	// Verify DiscardReviews is called after plan completion (should be called once)
+	vcsClient.VerifyWasCalledOnce().DiscardReviews(Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest]())
+}
+
+func TestRunGenericPlanCommand_DiscardApprovalsAfterPlan_EvenWhenPlanFails(t *testing.T) {
+	vcsClient := setup(t, func(testConfig *TestConfig) {
+		testConfig.discardApprovalAfterPlan = true
+	})
+
+	tmp := t.TempDir()
+	boltDB, err := db.New(tmp)
+	t.Cleanup(func() {
+		boltDB.Close()
+	})
+	Ok(t, err)
+	dbUpdater.Backend = boltDB
+	applyCommandRunner.Backend = boltDB
+	autoMerger.GlobalAutomerge = true
+	defer func() { autoMerger.GlobalAutomerge = false }()
+
+	// Set up project builder to return a project, then plan fails with an error
+	When(projectCommandBuilder.BuildPlanCommands(Any[*command.Context](), Any[*events.CommentCommand]())).ThenReturn([]command.ProjectContext{
+		{
+			CommandName: command.Plan,
+		},
+	}, nil)
+	When(projectCommandRunner.Plan(Any[command.ProjectContext]())).ThenReturn(command.ProjectResult{Error: errors.New("plan failed")})
+	When(workingDir.GetPullDir(Any[models.Repo](), Any[models.PullRequest]())).ThenReturn(tmp, nil)
+	pull := &github.PullRequest{State: github.Ptr("open")}
+	modelPull := models.PullRequest{BaseRepo: testdata.GithubRepo, State: models.OpenPullState, Num: testdata.Pull.Num}
+	When(githubGetter.GetPullRequest(Any[logging.SimpleLogging](), Eq(testdata.GithubRepo), Eq(testdata.Pull.Num))).ThenReturn(pull, nil)
+	When(eventParsing.ParseGithubPull(Any[logging.SimpleLogging](), Eq(pull))).ThenReturn(modelPull, modelPull.BaseRepo, testdata.GithubRepo, nil)
+	testdata.Pull.BaseRepo = testdata.GithubRepo
+	ch.RunCommentCommand(testdata.GithubRepo, nil, nil, testdata.User, testdata.Pull.Num, &events.CommentCommand{Name: command.Plan})
+	pendingPlanFinder.VerifyWasCalledOnce().DeletePlans(tmp)
+
+	// Verify DiscardReviews IS called even when plan fails (because the timing issue still applies)
+	vcsClient.VerifyWasCalledOnce().DiscardReviews(Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest]())
+}
+
+func TestRunAutoplanCommand_DiscardApprovalsAfterPlan(t *testing.T) {
+	vcsClient := setup(t, func(testConfig *TestConfig) {
+		testConfig.discardApprovalAfterPlan = true
+	})
+
+	tmp := t.TempDir()
+	boltDB, err := db.New(tmp)
+	t.Cleanup(func() {
+		boltDB.Close()
+	})
+	Ok(t, err)
+	dbUpdater.Backend = boltDB
+
+	When(projectCommandRunner.Plan(Any[command.ProjectContext]())).ThenReturn(command.ProjectResult{PlanSuccess: &models.PlanSuccess{}})
+	When(projectCommandBuilder.BuildAutoplanCommands(Any[*command.Context]())).ThenReturn([]command.ProjectContext{
+		{
+			CommandName: command.Plan,
+		},
+	}, nil)
+
+	When(workingDir.GetPullDir(Any[models.Repo](), Any[models.PullRequest]())).
+		ThenReturn(tmp, nil)
+	testdata.Pull.BaseRepo = testdata.GithubRepo
+	ch.RunAutoplanCommand(testdata.GithubRepo, testdata.GithubRepo, testdata.Pull, testdata.User)
+
+	// Verify DiscardReviews is called after autoplan completion
+	vcsClient.VerifyWasCalledOnce().DiscardReviews(Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest]())
+}
+
+func TestRunGenericPlanCommand_DiscardApprovalsAfterPlan_EvenWithNoProjects(t *testing.T) {
+	vcsClient := setup(t, func(testConfig *TestConfig) {
+		testConfig.discardApprovalAfterPlan = true
+	})
+
+	tmp := t.TempDir()
+	boltDB, err := db.New(tmp)
+	t.Cleanup(func() {
+		boltDB.Close()
+	})
+	Ok(t, err)
+	dbUpdater.Backend = boltDB
+	applyCommandRunner.Backend = boltDB
+	autoMerger.GlobalAutomerge = true
+	defer func() { autoMerger.GlobalAutomerge = false }()
+
+	// No projects found
+	When(projectCommandBuilder.BuildPlanCommands(Any[*command.Context](), Any[*events.CommentCommand]())).ThenReturn([]command.ProjectContext{}, nil)
+	When(workingDir.GetPullDir(Any[models.Repo](), Any[models.PullRequest]())).ThenReturn(tmp, nil)
+	pull := &github.PullRequest{State: github.Ptr("open")}
+	modelPull := models.PullRequest{BaseRepo: testdata.GithubRepo, State: models.OpenPullState, Num: testdata.Pull.Num}
+	When(githubGetter.GetPullRequest(Any[logging.SimpleLogging](), Eq(testdata.GithubRepo), Eq(testdata.Pull.Num))).ThenReturn(pull, nil)
+	When(eventParsing.ParseGithubPull(Any[logging.SimpleLogging](), Eq(pull))).ThenReturn(modelPull, modelPull.BaseRepo, testdata.GithubRepo, nil)
+	testdata.Pull.BaseRepo = testdata.GithubRepo
+	ch.RunCommentCommand(testdata.GithubRepo, nil, nil, testdata.User, testdata.Pull.Num, &events.CommentCommand{Name: command.Plan})
+	pendingPlanFinder.VerifyWasCalledOnce().DeletePlans(tmp)
+
+	// Verify DiscardReviews is called even when no projects are found (addresses race condition)
+	vcsClient.VerifyWasCalledOnce().DiscardReviews(Any[logging.SimpleLogging](), Any[models.Repo](), Any[models.PullRequest]())
+}
+
 func TestFailedApprovalCreatesFailedStatusUpdate(t *testing.T) {
 	t.Log("if \"atlantis approve_policies\" is run by non policy owner policy check status fails.")
 	setup(t)

server/events/plan_command_runner.go

@@ -33,6 +33,7 @@ func NewPlanCommandRunner(
 	pullStatusFetcher PullStatusFetcher,
 	lockingLocker locking.Locker,
 	discardApprovalOnPlan bool,
+	discardApprovalAfterPlan bool,
 	pullReqStatusFetcher vcs.PullReqStatusFetcher,
 ) *PlanCommandRunner {
 	return &PlanCommandRunner{
@@ -53,6 +54,7 @@ func NewPlanCommandRunner(
 		pullStatusFetcher:          pullStatusFetcher,
 		lockingLocker:              lockingLocker,
 		DiscardApprovalOnPlan:      discardApprovalOnPlan,
+		DiscardApprovalAfterPlan:   discardApprovalAfterPlan,
 		pullReqStatusFetcher:       pullReqStatusFetcher,
 	}
 }
@@ -83,8 +85,11 @@ type PlanCommandRunner struct {
 	// DiscardApprovalOnPlan controls if all already existing approvals should be removed/dismissed before executing
 	// a plan.
 	DiscardApprovalOnPlan bool
-	pullReqStatusFetcher  vcs.PullReqStatusFetcher
-	SilencePRComments     []string
+	// DiscardApprovalAfterPlan controls if all already existing approvals should be removed/dismissed after executing
+	// a plan.
+	DiscardApprovalAfterPlan bool
+	pullReqStatusFetcher     vcs.PullReqStatusFetcher
+	SilencePRComments        []string
 }
 
 func (p *PlanCommandRunner) runAutoplan(ctx *command.Context) {
@@ -150,6 +155,13 @@ func (p *PlanCommandRunner) runAutoplan(ctx *command.Context) {
 
 	p.pullUpdater.updatePull(ctx, AutoplanCommand{}, result)
 
+	// Discard approvals after autoplan completion if flag is set
+	if p.DiscardApprovalAfterPlan {
+		if err := p.pullUpdater.VCSClient.DiscardReviews(ctx.Log, baseRepo, pull); err != nil {
+			ctx.Log.Err("removing approvals after autoplan - %s", err)
+		}
+	}
+
 	pullStatus, err := p.dbUpdater.updateDB(ctx, ctx.Pull, result.ProjectResults)
 	if err != nil {
 		ctx.Log.Err("writing results: %s", err)
@@ -294,6 +306,13 @@ func (p *PlanCommandRunner) run(ctx *command.Context, cmd *CommentCommand) {
 		cmd,
 		result)
 
+	// Discard approvals after plan completion if flag is set
+	if p.DiscardApprovalAfterPlan {
+		if err := p.pullUpdater.VCSClient.DiscardReviews(ctx.Log, baseRepo, pull); err != nil {
+			ctx.Log.Err("removing approvals after plan - %s", err)
+		}
+	}
+
 	pullStatus, err := p.dbUpdater.updateDB(ctx, pull, result.ProjectResults)
 	if err != nil {
 		ctx.Log.Err("writing results: %s", err)

server/server.go

@@ -788,6 +788,7 @@ func NewServer(userConfig UserConfig, config Config) (*Server, error) {
 		backend,
 		lockingClient,
 		userConfig.DiscardApprovalOnPlanFlag,
+		userConfig.DiscardApprovalAfterPlanFlag,
 		pullReqStatusFetcher,
 	)
 

server/user_config.go

@@ -14,40 +14,41 @@ import (
 // The mapstructure tags correspond to flags in cmd/server.go and are used when
 // the config is parsed from a YAML file.
 type UserConfig struct {
-	AllowForkPRs                bool   `mapstructure:"allow-fork-prs"`
-	AllowCommands               string `mapstructure:"allow-commands"`
-	AtlantisURL                 string `mapstructure:"atlantis-url"`
-	AutoDiscoverModeFlag        string `mapstructure:"autodiscover-mode"`
-	Automerge                   bool   `mapstructure:"automerge"`
-	AutoplanFileList            string `mapstructure:"autoplan-file-list"`
-	AutoplanModules             bool   `mapstructure:"autoplan-modules"`
-	AutoplanModulesFromProjects string `mapstructure:"autoplan-modules-from-projects"`
-	AzureDevopsToken            string `mapstructure:"azuredevops-token"`
-	AzureDevopsUser             string `mapstructure:"azuredevops-user"`
-	AzureDevopsWebhookPassword  string `mapstructure:"azuredevops-webhook-password"`
-	AzureDevopsWebhookUser      string `mapstructure:"azuredevops-webhook-user"`
-	AzureDevOpsHostname         string `mapstructure:"azuredevops-hostname"`
-	BitbucketBaseURL            string `mapstructure:"bitbucket-base-url"`
-	BitbucketToken              string `mapstructure:"bitbucket-token"`
-	BitbucketUser               string `mapstructure:"bitbucket-user"`
-	BitbucketWebhookSecret      string `mapstructure:"bitbucket-webhook-secret"`
-	CheckoutDepth               int    `mapstructure:"checkout-depth"`
-	CheckoutStrategy            string `mapstructure:"checkout-strategy"`
-	DataDir                     string `mapstructure:"data-dir"`
-	DisableApplyAll             bool   `mapstructure:"disable-apply-all"`
-	DisableAutoplan             bool   `mapstructure:"disable-autoplan"`
-	DisableAutoplanLabel        string `mapstructure:"disable-autoplan-label"`
-	DisableMarkdownFolding      bool   `mapstructure:"disable-markdown-folding"`
-	DisableRepoLocking          bool   `mapstructure:"disable-repo-locking"`
-	DisableGlobalApplyLock      bool   `mapstructure:"disable-global-apply-lock"`
-	DisableUnlockLabel          string `mapstructure:"disable-unlock-label"`
-	DiscardApprovalOnPlanFlag   bool   `mapstructure:"discard-approval-on-plan"`
-	EmojiReaction               string `mapstructure:"emoji-reaction"`
-	EnablePolicyChecksFlag      bool   `mapstructure:"enable-policy-checks"`
-	EnableRegExpCmd             bool   `mapstructure:"enable-regexp-cmd"`
-	EnableProfilingAPI          bool   `mapstructure:"enable-profiling-api"`
-	EnableDiffMarkdownFormat    bool   `mapstructure:"enable-diff-markdown-format"`
-	ExecutableName              string `mapstructure:"executable-name"`
+	AllowForkPRs                 bool   `mapstructure:"allow-fork-prs"`
+	AllowCommands                string `mapstructure:"allow-commands"`
+	AtlantisURL                  string `mapstructure:"atlantis-url"`
+	AutoDiscoverModeFlag         string `mapstructure:"autodiscover-mode"`
+	Automerge                    bool   `mapstructure:"automerge"`
+	AutoplanFileList             string `mapstructure:"autoplan-file-list"`
+	AutoplanModules              bool   `mapstructure:"autoplan-modules"`
+	AutoplanModulesFromProjects  string `mapstructure:"autoplan-modules-from-projects"`
+	AzureDevopsToken             string `mapstructure:"azuredevops-token"`
+	AzureDevopsUser              string `mapstructure:"azuredevops-user"`
+	AzureDevopsWebhookPassword   string `mapstructure:"azuredevops-webhook-password"`
+	AzureDevopsWebhookUser       string `mapstructure:"azuredevops-webhook-user"`
+	AzureDevOpsHostname          string `mapstructure:"azuredevops-hostname"`
+	BitbucketBaseURL             string `mapstructure:"bitbucket-base-url"`
+	BitbucketToken               string `mapstructure:"bitbucket-token"`
+	BitbucketUser                string `mapstructure:"bitbucket-user"`
+	BitbucketWebhookSecret       string `mapstructure:"bitbucket-webhook-secret"`
+	CheckoutDepth                int    `mapstructure:"checkout-depth"`
+	CheckoutStrategy             string `mapstructure:"checkout-strategy"`
+	DataDir                      string `mapstructure:"data-dir"`
+	DisableApplyAll              bool   `mapstructure:"disable-apply-all"`
+	DisableAutoplan              bool   `mapstructure:"disable-autoplan"`
+	DisableAutoplanLabel         string `mapstructure:"disable-autoplan-label"`
+	DisableMarkdownFolding       bool   `mapstructure:"disable-markdown-folding"`
+	DisableRepoLocking           bool   `mapstructure:"disable-repo-locking"`
+	DisableGlobalApplyLock       bool   `mapstructure:"disable-global-apply-lock"`
+	DisableUnlockLabel           string `mapstructure:"disable-unlock-label"`
+	DiscardApprovalOnPlanFlag    bool   `mapstructure:"discard-approval-on-plan"`
+	DiscardApprovalAfterPlanFlag bool   `mapstructure:"discard-approval-after-plan"`
+	EmojiReaction                string `mapstructure:"emoji-reaction"`
+	EnablePolicyChecksFlag       bool   `mapstructure:"enable-policy-checks"`
+	EnableRegExpCmd              bool   `mapstructure:"enable-regexp-cmd"`
+	EnableProfilingAPI           bool   `mapstructure:"enable-profiling-api"`
+	EnableDiffMarkdownFormat     bool   `mapstructure:"enable-diff-markdown-format"`
+	ExecutableName               string `mapstructure:"executable-name"`
 	// Fail and do not run the Atlantis command request if any of the pre workflow hooks error.
 	FailOnPreWorkflowHookError      bool   `mapstructure:"fail-on-pre-workflow-hook-error"`
 	HideUnchangedPlanComments       bool   `mapstructure:"hide-unchanged-plan-comments"`