Fixes #1006: Keep track of project status even if plans have been deleted #1005

parmouraly · 2020-04-23T16:10:45Z

As part of the fix the model is slightly extended to allow for an extra PlanStatus indicating the absence of a plan for a project.
This allows us to unlock a project and delete the plan, without having to completely remove the project status.
That means that atlantis knows there is a project that should normally have a plan and be accounted for, but the plan has been deleted/unlocked.
The reason this is important is so that we can't "trick" atlantis into thinking that the PR can be merged just because we've lost track of a projectstatus as a result of a manual unlock.

An example of things going wrong is when you delete a plan and atlantis lock via the UI and then execute atlantis apply
No projects are applied as a result but the branch is automatically merged which means you can end up with changed infrastructure code on your master without applying any changes

Issue link: #1006

codecov · 2020-04-23T16:14:12Z

Codecov Report

Merging #1005 into master will increase coverage by 0.43%.
The diff coverage is 60.00%.

@@            Coverage Diff             @@
##           master    #1005      +/-   ##
==========================================
+ Coverage   71.49%   71.93%   +0.43%     
==========================================
  Files          67       67              
  Lines        5564     5565       +1     
==========================================
+ Hits         3978     4003      +25     
+ Misses       1272     1238      -34     
- Partials      314      324      +10

Impacted Files	Coverage Δ
server/events/models/models.go	`73.84% <0.00%> (-1.16%)`	⬇️
server/locks_controller.go	`87.14% <0.00%> (ø)`
server/events/db/boltdb.go	`74.16% <100.00%> (-0.13%)`	⬇️
server/events/command_runner.go	`52.72% <0.00%> (+8.84%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 25cd66d...abd2161. Read the comment docs.

Extra guards have been added to preven automatically merging a branch upstream when automerge is enabled but errors exist or if no commands where executed at all. An example of this bug in action is when you delete a plan and atlantis lock via the UI and then execute atlantis apply No projects are applied but the branch is automerged

As part of the fix the model is slightly extended to allow for an extra PlanStatus indicating the absence of a plan for a project. This allows us to unlock a project and delete the plan, without having to completely remove the project status. That means that atlantis knows there is a project that should normally have a plan and be accounted for, but the plan has been deleted/unlocked. The reason this is important is so that we can't "trick" atlantis into thinking that the PR can be merged just because we've lost track of a projectstatus as a result of a manual unlock.

parmouraly · 2020-04-26T13:04:33Z

Hi 👋
I will try to provide some tests and refactor the DB to improve reusing code for the bit I added.
Please let me know if you don't agree with this change so in case I haven't got around to writing tests yet I can save some time.
Or if you agree and can let me know I'll be more motivated to wrap this up :-)
Thanks!

marsavela · 2020-05-05T13:53:05Z

@lkysow I believe this deserves a look. This is an important bug IMO. Would we be able to go forward? @parmouraly are there any updates?

I don't want code to be merged without being applied. I know my devs. 😂

lkysow

Looks good, needs tests please.

lkysow · 2020-05-05T23:55:37Z

server/events/models/models.go

 	// error while applying it.
 	ErroredApplyStatus
 	// AppliedPlanStatus means that a plan has been generated and applied
 	// successfully.
 	AppliedPlanStatus
+	// NotPlannedPlanStatus means that either no plan has been generated yet or
+	// a plan has been deleted - eg after using the UI to unlock the project
+	NotPlannedPlanStatus


This is only set when the project is unlocked, not when no plan has been generated yet, so maybe we can call this DiscardedPlanStatus and update the description to say:

means that there was an unapplied plan that was discarded due to a project being unlocked

lkysow · 2020-05-05T23:56:10Z

server/locks_controller.go

@@ -108,8 +108,8 @@ func (l *LocksController) DeleteLock(w http.ResponseWriter, r *http.Request) {
 				l.Logger.Err("unable to delete workspace: %s", err)
 			}
 		}
-		if err := l.DB.DeleteProjectStatus(lock.Pull, lock.Workspace, lock.Project.Path); err != nil {


We should delete the DeleteProjectStatus method since it's not used anymore

With BoltDB interface we are able to mock/stub and improve test coverage. Some other refactoring was required to make code aware and compatible with the new interface

parmouraly · 2020-05-11T14:21:25Z

server/events/command_runner_test.go

+	//When(boltDB.UpdatePullWithResults(modelPull, nil)).ThenReturn(pullStatus, nil)
+	When(boltDB.UpdatePullWithResults(dbmatchers.EqModelsPullRequest(modelPull), dbmatchers.EqSliceOfModelsProjectResult(nil))).ThenReturn(pullStatus, nil)
+	// TODO: stubbing here doesn't seem to work? pullStatus defined here is not actually returned and so I cannot uncomment the next two lines which would verify our scenario here
+	//vcsClient.VerifyWasCalledOnce().CreateComment(fixtures.GithubRepo, modelPull.Num, "not automerging because project at dir %q, workspace %q has status %q")


@lkysow this tests the meat of this PR but I've been having some trouble with stubbing. Line 287 (or 286) doesn't seem to have any effect. What am I missing?

parmouraly · 2020-05-19T07:59:00Z

This is ready to be merged with the exception of one new testcase I've added in command_runner_test.go which I would like to include but haven't managed to get it working. But I can just remove that for now in order to merge if the rest is OK.

lkysow

In addition to my suggestions is it possible to use a real bolt db instead of the mock? That will reduce a lot of the code changes.

server/events/command_runner_test.go

lkysow · 2020-05-25T23:24:21Z

server/events/db/boltdb.go

 // workspace and repoRelDir.
-func (b *BoltDB) DeleteProjectStatus(pull models.PullRequest, workspace string, repoRelDir string) error {
+func (b *DefaultBoltDB) UpdateProjectStatus(pull models.PullRequest, workspace string, repoRelDir string, targetStatus models.ProjectPlanStatus) error {


Suggested rewrite.

// UpdateProjectStatus updates project status. func (b *DefaultBoltDB) UpdateProjectStatus(pull models.PullRequest, workspace string, repoRelDir string, newStatus models.ProjectPlanStatus) error { key, err := b.pullKey(pull) if err != nil { return err } err = b.db.Update(func(tx *bolt.Tx) error { bucket := tx.Bucket(b.pullsBucketName) currStatusPtr, err := b.getPullFromBucket(bucket, key) if err != nil { return err } if currStatusPtr == nil { return nil } currStatus := *currStatusPtr // Update the status. for i := range currStatus.Projects { // NOTE: We're using a reference here because we are // in-place updating its Status field. proj := &currStatus.Projects[i] if proj.Workspace == workspace && proj.RepoRelDir == repoRelDir { proj.Status = newStatus break } } return b.writePullToBucket(bucket, key, currStatus) }) return errors.Wrap(err, "DB transaction failed") }

lkysow · 2020-05-25T23:27:00Z

server/locks_controller_test.go

+			RepoFullName: repoName,
+		},
+	}, nil)
+	db := dbmocks.NewMockBoltDB()


Can you use a real boltdb instead?

tmp, cleanup := TempDir(t) defer os.Remove(tmp) db, err := db.New(tmp) Ok(t, err)

Sorry I don't follow.
The value of this test is to verify that db.UpdateProjectStatus is actually called when someone uses the deleteLock endpoint.
Using mock allows me to verify this (see line 234) in a simple way.

If I use a real DB like you suggest, how can I do this?
Am I missing something?

Commented above.

parmouraly · 2020-05-26T13:58:18Z

Hey Luke, thanks for your comments. I have replied to all of them, please if you can send me some clarifications so I can push the last commit and we can wrap this up.

lkysow · 2020-05-27T16:17:57Z

server/events/command_runner_test.go

@@ -234,3 +238,20 @@ func TestRunAutoplanCommand_DeletePlans(t *testing.T) {
 	ch.RunAutoplanCommand(fixtures.GithubRepo, fixtures.GithubRepo, fixtures.Pull, fixtures.User)
 	pendingPlanFinder.VerifyWasCalledOnce().DeletePlans(tmp)
 }
+
+func TestApplyWithAutoMerge_VSCMerge(t *testing.T) {
+	t.Log("if \"atlantis apply\" is run with automerge and at least one project" +


comment here is not describing the test

Good spot, fixed

server/events/command_runner_test.go

lkysow · 2020-06-03T23:15:11Z

server/locks_controller_test.go

@@ -192,6 +194,46 @@ func TestDeleteLock_OldFormat(t *testing.T) {
 	cp.VerifyWasCalled(Never()).CreateComment(AnyRepo(), AnyInt(), AnyString())
 }

+func TestDeleteLock_UpdateProjectStatus(t *testing.T) {


If you update to the following the you don't need the db mock:

func TestDeleteLock_UpdateProjectStatus(t *testing.T) { t.Log("When deleting a lock, pull status has to be updated to reflect discarded plan") RegisterMockTestingT(t) repoName := "owner/repo" projectPath := "path" workspaceName := "workspace" cp := vcsmocks.NewMockClient() l := mocks.NewMockLocker() workingDir := mocks2.NewMockWorkingDir() workingDirLocker := events.NewDefaultWorkingDirLocker() pull := models.PullRequest{ BaseRepo: models.Repo{FullName: repoName}, } When(l.Unlock("id")).ThenReturn(&models.ProjectLock{ Pull: pull, Workspace: workspaceName, Project: models.Project{ Path: projectPath, RepoFullName: repoName, }, }, nil) tmp, cleanup := TempDir(t) defer cleanup() db, err := db.New(tmp) Ok(t, err) // Seed the DB with a successful plan for that project (that is later discarded). _, err = db.UpdatePullWithResults(pull, []models.ProjectResult{ { Command: models.PlanCommand, RepoRelDir: projectPath, Workspace: workspaceName, PlanSuccess: &models.PlanSuccess{ TerraformOutput: "tf-output", LockURL: "lock-url", }, }, }) Ok(t, err) lc := server.LocksController{ Locker: l, Logger: logging.NewNoopLogger(), VCSClient: cp, WorkingDirLocker: workingDirLocker, WorkingDir: workingDir, DB: db, } req, _ := http.NewRequest("GET", "", bytes.NewBuffer(nil)) req = mux.SetURLVars(req, map[string]string{"id": "id"}) w := httptest.NewRecorder() lc.DeleteLock(w, req) responseContains(t, w, http.StatusOK, "Deleted lock id \"id\"") status, err := db.GetPullStatus(pull) Ok(t, err) Assert(t, status != nil, "status was nil") Equals(t, []models.ProjectStatus{ { Workspace: workspaceName, RepoRelDir: projectPath, Status: models.DiscardedPlanStatus, }, }, status.Projects) }

Thanks, replaced 👍

parmouraly · 2020-06-08T14:35:25Z

@lkysow I think I've address all comments here, is there anything else you want me to look at or can we merge?

lkysow

Looks good! I need to pull the code and test it which I'll do just before including it in the next release.

parmouraly force-pushed the fix-automerge-on-noapply branch from 462e45e to c85155e Compare April 23, 2020 16:18

parmouraly mentioned this pull request Apr 24, 2020

Auto-merge could merge unapplied infra changes #1006

Closed

parmouraly changed the title ~~Fix: Do not automerge with errors or noop~~ Fixes #1006: Do not automerge with errors or noop Apr 24, 2020

parmouraly marked this pull request as draft April 26, 2020 11:19

parmouraly changed the title ~~Fixes #1006: Do not automerge with errors or noop~~ Fixes #1006: Do not automerge when plans have been deleted Apr 26, 2020

parmouraly marked this pull request as ready for review April 26, 2020 13:02

parmouraly changed the title ~~Fixes #1006: Do not automerge when plans have been deleted~~ Fixes #1006: Keep track of project status even if plans have been deleted Apr 26, 2020

lkysow reviewed May 5, 2020

View reviewed changes

Paris Morali added 2 commits May 9, 2020 11:42

Integrate PR feedback and tests adapted

ce1d577

Add BoltDB interface and some relevant tests

ade9b31

With BoltDB interface we are able to mock/stub and improve test coverage. Some other refactoring was required to make code aware and compatible with the new interface

parmouraly commented May 11, 2020

View reviewed changes

Add the rest of the BoltDB interface methods

cb26355

lkysow requested changes May 25, 2020

View reviewed changes

lkysow added the waiting-on-response Waiting for a response from the user label May 25, 2020

lkysow reviewed May 27, 2020

View reviewed changes

server/events/command_runner_test.go Show resolved Hide resolved

parmouraly added 2 commits June 3, 2020 17:00

Integrate some more PR feedback including a test

421b247

Fix lint error

9689dbb

parmouraly requested a review from lkysow June 3, 2020 16:12

lkysow reviewed Jun 3, 2020

View reviewed changes

parmouraly added 3 commits June 4, 2020 16:12

Remove BoltDB interface and mock as not needed

f546b69

Fix typo on boltDB reference

036e1f6

Typo in BoltDB test

8a905f9

parmouraly added 2 commits June 4, 2020 16:32

Merge branch 'master' into fix-automerge-on-noapply

ad512f1

Fix typo from master merge conflict

abd2161

parmouraly requested a review from lkysow June 4, 2020 15:50

lkysow approved these changes Jun 9, 2020

View reviewed changes

lkysow mentioned this pull request Jun 24, 2020

Fixes #1006: Keep track of project status even if plans have been deleted #1092

Merged

lkysow removed the waiting-on-response Waiting for a response from the user label Jun 24, 2020

lkysow merged commit d3a6d18 into runatlantis:master Jun 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #1006: Keep track of project status even if plans have been deleted #1005

Fixes #1006: Keep track of project status even if plans have been deleted #1005

parmouraly commented Apr 23, 2020 •

edited

Loading

codecov bot commented Apr 23, 2020 •

edited

Loading

parmouraly commented Apr 26, 2020 •

edited

Loading

marsavela commented May 5, 2020

lkysow left a comment

lkysow May 5, 2020

lkysow May 5, 2020

parmouraly May 11, 2020

parmouraly commented May 19, 2020

lkysow left a comment

lkysow May 25, 2020

parmouraly May 26, 2020

lkysow May 25, 2020

parmouraly May 26, 2020

lkysow Jun 3, 2020

parmouraly commented May 26, 2020

lkysow May 27, 2020 •

edited

Loading

parmouraly Jun 3, 2020

lkysow Jun 3, 2020

parmouraly Jun 4, 2020

parmouraly commented Jun 8, 2020 •

edited

Loading

lkysow left a comment

Fixes #1006: Keep track of project status even if plans have been deleted #1005

Fixes #1006: Keep track of project status even if plans have been deleted #1005

Conversation

parmouraly commented Apr 23, 2020 • edited Loading

codecov bot commented Apr 23, 2020 • edited Loading

Codecov Report

parmouraly commented Apr 26, 2020 • edited Loading

marsavela commented May 5, 2020

lkysow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parmouraly commented May 19, 2020

lkysow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parmouraly commented May 26, 2020

lkysow May 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parmouraly commented Jun 8, 2020 • edited Loading

lkysow left a comment

Choose a reason for hiding this comment

parmouraly commented Apr 23, 2020 •

edited

Loading

codecov bot commented Apr 23, 2020 •

edited

Loading

parmouraly commented Apr 26, 2020 •

edited

Loading

lkysow May 27, 2020 •

edited

Loading

parmouraly commented Jun 8, 2020 •

edited

Loading