fix: Delete locks and workdirs with potentially stale previous plans …

…which fixes 1624 (runatlantis#1704)

* Delete previous plans on autoplan or atlantis plan

When using non-default workspaces, plans are stored
in pr-and-workspace-specific directories.
If a PR is subsequently updated it might happen that
some of the plans are no longer relevant with regards
to the latest changes.
This change ensures that plans are always deleted
when a generic plan is triggered either by autoplan
or by a "atlantis plan" command.
NB Plans are not cleaned up when specific projects are
planned explicitly with "atlantis plan -p/-d/-w".

* Use DeleteLockCommand to delete locks and workdirs
containing previous plans

Co-authored-by: giuli007 <giuglioz@gmail.com>

runatlantis.io/docs/using-atlantis.md

@@ -45,6 +45,10 @@ atlantis plan -w staging
 * `-w workspace` Switch to this [Terraform workspace](https://www.terraform.io/docs/state/workspaces.html) before planning. Defaults to `default`. If not using Terraform workspaces you can ignore this.
 * `--verbose` Append Atlantis log to comment.
 
+::: warning NOTE
+A `atlantis plan` (without flags), like autoplans, discards all plans previously created with `atlantis plan` `-p`/`-d`/`-w`
+:::
+
 ### Additional Terraform flags
 
 If you need to run `terraform plan` with additional arguments, like `-target=resource` or `-var 'foo-bar'` or `-var-file myfile.tfvars`
@@ -65,6 +69,7 @@ Runs `terraform apply` for the plan that matches the directory/project/workspace
 
 ::: tip
 If no directory/project/workspace is specified, ex. `atlantis apply`, this command will apply **all unapplied plans from this pull request**.
+This includes all projects that have been planned manually with `atlantis plan` `-p`/`-d`/`-w` since the last autoplan or `atlantis plan` command.
 :::
 
 ### Examples

server/controllers/events/events_controller_e2e_test.go

@@ -872,6 +872,8 @@ func setupE2E(t *testing.T, repoDir string) (events_controllers.VCSEventsControl
 	locker := events.NewDefaultWorkingDirLocker()
 	parser := &config.ParserValidator{}
 
+	deleteLockCommand := mocks.NewMockDeleteLockCommand()
+
 	globalCfgArgs := valid.GlobalCfgArgs{
 		AllowRepoCfg: true,
 		MergeableReq: false,
@@ -1024,6 +1026,7 @@ func setupE2E(t *testing.T, repoDir string) (events_controllers.VCSEventsControl
 		parallelPoolSize,
 		silenceNoProjects,
 		boltdb,
+		deleteLockCommand,
 	)
 
 	e2ePullReqStatusFetcher := vcs.NewPullReqStatusFetcher(e2eVCSClient)

server/events/command_runner_test.go

@@ -132,6 +132,7 @@ func setup(t *testing.T) *vcsmocks.MockClient {
 		parallelPoolSize,
 		SilenceNoProjects,
 		defaultBoltDB,
+		deleteLockCommand,
 	)
 
 	pullReqStatusFetcher := vcs.NewPullReqStatusFetcher(vcsClient)
@@ -527,9 +528,77 @@ func TestRunUnlockCommandFail_VCSComment(t *testing.T) {
 	vcsClient.VerifyWasCalledOnce().CreateComment(fixtures.GithubRepo, fixtures.Pull.Num, "Failed to delete PR locks", "unlock")
 }
 
+func TestRunAutoplanCommand_DeleteLocksByPull(t *testing.T) {
+	setup(t)
+	tmp, cleanup := TempDir(t)
+	defer cleanup()
+	boltDB, err := db.New(tmp)
+	Ok(t, err)
+	dbUpdater.DB = boltDB
+	applyCommandRunner.DB = boltDB
+	autoMerger.GlobalAutomerge = true
+	defer func() { autoMerger.GlobalAutomerge = false }()
+
+	When(projectCommandBuilder.BuildAutoplanCommands(matchers.AnyPtrToEventsCommandContext())).
+		ThenReturn([]models.ProjectCommandContext{
+			{
+				CommandName: models.PlanCommand,
+			},
+			{
+				CommandName: models.PlanCommand,
+			},
+		}, nil)
+	When(projectCommandRunner.Plan(matchers.AnyModelsProjectCommandContext())).ThenReturn(models.ProjectResult{PlanSuccess: &models.PlanSuccess{}})
+	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)
+	deleteLockCommand.VerifyWasCalledOnce().DeleteLocksByPull(fixtures.Pull.BaseRepo.FullName, fixtures.Pull.Num)
+}
+
+func TestRunGenericPlanCommand_DeleteLocksByPull(t *testing.T) {
+	setup(t)
+	tmp, cleanup := TempDir(t)
+	defer cleanup()
+	boltDB, err := db.New(tmp)
+	Ok(t, err)
+	dbUpdater.DB = boltDB
+	applyCommandRunner.DB = boltDB
+	autoMerger.GlobalAutomerge = true
+	defer func() { autoMerger.GlobalAutomerge = false }()
+
+	When(projectCommandRunner.Plan(matchers.AnyModelsProjectCommandContext())).ThenReturn(models.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: models.PlanCommand})
+	deleteLockCommand.VerifyWasCalledOnce().DeleteLocksByPull(fixtures.Pull.BaseRepo.FullName, fixtures.Pull.Num)
+}
+
+func TestRunSpecificPlanCommandDoesnt_DeleteLocksByPull(t *testing.T) {
+	setup(t)
+	tmp, cleanup := TempDir(t)
+	defer cleanup()
+	boltDB, err := db.New(tmp)
+	Ok(t, err)
+	dbUpdater.DB = boltDB
+	applyCommandRunner.DB = boltDB
+	autoMerger.GlobalAutomerge = true
+	defer func() { autoMerger.GlobalAutomerge = false }()
+
+	When(projectCommandRunner.Plan(matchers.AnyModelsProjectCommandContext())).ThenReturn(models.ProjectResult{PlanSuccess: &models.PlanSuccess{}})
+	When(workingDir.GetPullDir(matchers.AnyModelsRepo(), matchers.AnyModelsPullRequest())).ThenReturn(tmp, nil)
+	fixtures.Pull.BaseRepo = fixtures.GithubRepo
+	ch.RunCommentCommand(fixtures.GithubRepo, nil, nil, fixtures.User, fixtures.Pull.Num, &events.CommentCommand{Name: models.PlanCommand, ProjectName: "default"})
+	deleteLockCommand.VerifyWasCalled(Never()).DeleteLocksByPull(fixtures.Pull.BaseRepo.FullName, fixtures.Pull.Num)
+}
+
 // Test that if one plan fails and we are using automerge, that
 // we delete the plans.
-func TestRunAutoplanCommand_DeletePlans(t *testing.T) {
+func TestRunAutoplanCommandWithError_DeletePlans(t *testing.T) {
 	setup(t)
 	tmp, cleanup := TempDir(t)
 	defer cleanup()
@@ -572,6 +641,7 @@ func TestRunAutoplanCommand_DeletePlans(t *testing.T) {
 		ThenReturn(tmp, nil)
 	fixtures.Pull.BaseRepo = fixtures.GithubRepo
 	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.VerifyWasCalledOnce().DeletePlans(tmp)
 }
 

server/events/plan_command_runner.go

@@ -22,6 +22,7 @@ func NewPlanCommandRunner(
 	parallelPoolSize int,
 	SilenceNoProjects bool,
 	pullStatusFetcher PullStatusFetcher,
+	deleteLockCommand DeleteLockCommand,
 ) *PlanCommandRunner {
 	return &PlanCommandRunner{
 		silenceVCSStatusNoPlans:    silenceVCSStatusNoPlans,
@@ -39,6 +40,7 @@ func NewPlanCommandRunner(
 		parallelPoolSize:           parallelPoolSize,
 		SilenceNoProjects:          SilenceNoProjects,
 		pullStatusFetcher:          pullStatusFetcher,
+		deleteLockCommand:          deleteLockCommand,
 	}
 }
 
@@ -64,6 +66,7 @@ type PlanCommandRunner struct {
 	autoMerger                 *AutoMerger
 	parallelPoolSize           int
 	pullStatusFetcher          PullStatusFetcher
+	deleteLockCommand          DeleteLockCommand
 }
 
 func (p *PlanCommandRunner) runAutoplan(ctx *command.Context) {
@@ -106,6 +109,13 @@ func (p *PlanCommandRunner) runAutoplan(ctx *command.Context) {
 		ctx.Log.Warn("unable to update plan commit status: %s", err)
 	}
 
+	// discard previous plans that might not be relevant anymore
+	ctx.Log.Debug("deleting locks and workdir(s) with previous plans")
+	_, err = p.deleteLockCommand.DeleteLocksByPull(ctx.Pull.BaseRepo.FullName, ctx.Pull.Num)
+	if err != nil {
+		ctx.Log.Err("deleting locks: %s", err)
+	}
+
 	// Only run commands in parallel if enabled
 	var result command.Result
 	if p.isParallelEnabled(projectCmds) {
@@ -180,6 +190,16 @@ func (p *PlanCommandRunner) run(ctx *command.Context, cmd *CommentCommand) {
 
 	projectCmds, policyCheckCmds := p.partitionProjectCmds(ctx, projectCmds)
 
+	// if the plan is generic, new plans will be generated based on changes
+	// discard previous plans that might not be relevant anymore
+	if !cmd.IsForSpecificProject() {
+		ctx.Log.Debug("deleting locks and workdir(s) with previous plans")
+		_, err = p.deleteLockCommand.DeleteLocksByPull(ctx.Pull.BaseRepo.FullName, ctx.Pull.Num)
+		if err != nil {
+			ctx.Log.Err("deleting locks: %s", err)
+		}
+	}
+
 	// Only run commands in parallel if enabled
 	var result command.Result
 	if p.isParallelEnabled(projectCmds) {

server/server.go

@@ -613,6 +613,7 @@ func NewServer(userConfig UserConfig, config Config) (*Server, error) {
 		userConfig.ParallelPoolSize,
 		userConfig.SilenceNoProjects,
 		boltdb,
+		deleteLockCommand,
 	)
 
 	pullReqStatusFetcher := vcs.NewPullReqStatusFetcher(vcsClient)