Atlantis unlock and discard all plans via VCS comment #1003

parmouraly · 2020-04-22T12:29:27Z

Objective
In order to allow someone to discard a plans and Atlantis locks without having to close/reopen their PR, we would like to add upstream functionality that enables devs to discard plans via VCS comments in an analogous way to planning/applying.

Context
The ability to discard an atlantis plan on demand is useful particularly for long lived PRs after a successful plan, as they would tend to block other people from performing Atlantis plan on the same project(s) for as long as the original PR is still open and successfully planned, which is not optimal for parallel working.

Atlantis provides a UI action that allows you to discard a plan and remove the atlantis lock.
However for security reasons because of the access Atlantis agents have to modify infrastructure, the UI might not always be reachable due to firewall restrictions.

lkysow · 2020-04-22T23:49:16Z

Hi @parmouraly thanks for the PR!
I don't think you need to follow the pattern for the other commands with the command_builder. Instead I think we should take the code from the DeleteLock method locks_controller and factor it out into a new struct DeleteLockCommand. Then both the DeleteLock and command_runner can call this command struct.

parmouraly · 2020-04-23T08:12:18Z

Hi @lkysow thanks for the quick feedback!
That makes sense, I will work on that and push a follow up between today and tomorrow.

On a more philosophical level one thing I was wondering about is the usage of the keyword discard vs smth more light like unlock.
I did debate which of the two would be better. Unlock feels a bit more to the point and less scary to me but I used discard be following the convention in the LockController.

parmouraly · 2020-04-23T16:31:36Z

Got side-tracked with looking into a bug for which I've put a PR up.
Is the refactor in my last commit moving in the right direction?
Hoping to wrap up the final bits tomorrow.

matthewellis-pp · 2020-05-18T15:37:58Z

Would love to see some movement on this as its a very important feature for me, please reach out if i can be of any assistance on this.

parmouraly · 2020-05-18T16:22:29Z

hey @matthewellis-pp 👋
I've been caught up with some work. I think @lkysow is onboard with that one but he's also been busy.
I'm going to try and wrap this up this week code wise, there is not much left to do. And hopefully we can get this reviewed and merged soon.

lkysow · 2020-05-25T23:02:08Z

Hi @parmouraly so you've got closer to what I was asking for.

I don't think we need to change project_command_builder. I think we should be able to call DeleteLockCommand directly from command_runner:

	if !c.validateCtxAndComment(ctx) {
		return
	}

+	if cmd.CommandName() == models.DiscardCommand {
+		c.DeleteLockCommand.Delete(cmd)
+		l.VCSClient.CreateComment(lock.Pull.BaseRepo, lock.Pull.Num, comment)
+		return
+	}

	if cmd.CommandName() == models.ApplyCommand {

lkysow

I think unlock is better than discard like you mentioned in another one of your comments.

lkysow · 2020-05-25T23:02:18Z

.gitignore

+atlantis
+temp.sh


oops that was unintentional. Removed

CONTRIBUTING.md

lkysow · 2020-05-25T23:02:52Z

server/events/comment_parser.go

@@ -61,6 +61,8 @@ type CommentBuilder interface {
 	BuildPlanComment(repoRelDir string, workspace string, project string, commentArgs []string) string
 	// BuildApplyComment builds an apply comment for the specified args.
 	BuildApplyComment(repoRelDir string, workspace string, project string) string
+	// BuildDiscardComment builds a discard comment for the specified args.
+	BuildDiscardComment(repoRelDir string, workspace string, project string, commentArgs []string) string


Shouldn't need commentArgs

server/events/comment_parser.go

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

server/events/comment_parser.go

@@ -264,6 +274,22 @@ func (e *CommentParser) BuildApplyComment(repoRelDir string, workspace string, p
 	return fmt.Sprintf("%s %s%s", atlantisExecutable, models.ApplyCommand.String(), flags)
 }

+// BuildDiscardComment builds discard comment for the specified args.
+func (e *CommentParser) BuildDiscardComment(repoRelDir string, workspace string, project string, commentArgs []string) string {


won't need this

lkysow · 2020-05-25T23:07:52Z

server/events/comment_parser.go

+           To plan a specific project, use the -d, -w and -p flags.
+  apply    Runs 'terraform apply' on all unapplied plans from this pull request.
+           To only apply a specific plan, use the -d, -w and -p flags.
+  discard  Discards all plans in this PR as well as the atlantis locks.


If you support -d, -w and -p then this should read.

Discards all plans in this PR and deletes all locks held by this PR. To discard a specific project, use the -d, -w and -p flags.

I was thinking, for simplicity, to only support general discard/unlock for the whole PR in this version. Would that be OK?

Yes that sounds good.

lkysow · 2020-05-25T23:08:19Z

server/events/delete_lock_command.go

@@ -0,0 +1,84 @@
+// Copyright 2017 HootSuite Media Inc.


Don't need this boilerplate because this is a new file. That's only for old files.

server/events/delete_lock_command.go

lkysow · 2020-05-25T23:09:02Z

server/events/markdown_renderer.go

-	applyCommandTitle = "Apply"
+	planCommandTitle    = "Plan"
+	applyCommandTitle   = "Apply"
+	discardCommandTitle = "Discard"


I don't think you need to modify this renderer. Should be able to construct this comment in command_runner.

Done. I left the bit that shows the option for discarding plans

I mean after a successful plan, you get options to click to unlock, apply etc, so I added the unlock option

This successfully deletes the atlantis lock and allows a competing PR to plan, but doesn not delete the actual plans, so a user can still apply after the lock is discarded. This will be investigated in a follow up commit Also fixed the basic mark down rendering for the discard command

Specifically deleting plans via VCS commenting now reuses the same logic as the LockController which is used by the UI delete plan action. Also via VCS commenting it's only possible atm to delete all the plans involved in the PR, as well as release all related Atlantis locks

Co-authored-by: Luke Kysow <[email protected]>

parmouraly · 2020-06-08T15:01:08Z

Just rebased to master ^

server/events/delete_lock_command_test.go

parmouraly · 2020-06-08T16:13:55Z

server/testfixtures/test-repos/simple/exp-output-autoplan.txt.act

+* :arrow_forward: To **apply** this plan, comment:
+    * `atlantis apply -d .`
+* :put_litter_in_its_place: To **delete** this plan click [here](lock-url), or to delete all plans comment and atlantis locks:
+    * `atlantis unlock`


How does this look @lkysow . Are you happy with this keyword? I can adapt the comment parser, model etc if so
PS: oops just spotted a typo in the above comment will fix with next commit

parmouraly · 2020-06-08T16:16:01Z

server/locks_controller_test.go

@@ -140,129 +135,23 @@ func TestDeleteLock_InvalidLockID(t *testing.T) {
 	responseContains(t, w, http.StatusBadRequest, "Invalid lock id \"%A@\"")
 }

+// TODO: I'm going to adapt the next 5 test cases to use some mock of DeleteLockCommand


Am I OK to generate mocks for DeleteLockCommand so I can refactor these tests @lkysow ?

Yes, looks like that will be too hard to instantiate a concrete instance of.

Just trying to keep this as simple as possible

parmouraly · 2020-06-09T16:14:39Z

@lkysow I think so far I've addressed all your comments.

I think the three main outstanding points are:

Reach an agreement on providing only a general atlantis unlock at this first iteration, for the whole PR. Just to keep things simple and take it step by step. Are we good with unlock?
Some test additions/adaptations to maintain or improve coverage
Deciding on whether to generate mocks for the new DeleteLockCommand in order to restore the tests in the locks_controller_test. Should I go for it, or do you prefer to avoid mocks here too?

Is there anything else I'm forgetting? Do you think we are close to merging this one?
If not what can I do to get this in.

lkysow · 2020-06-09T21:18:43Z

server/events/comment_parser.go

+		name = models.DiscardCommand
+		flagSet = pflag.NewFlagSet(models.DiscardCommand.String(), pflag.ContinueOnError)
+		flagSet.SetOutput(ioutil.Discard)
+		flagSet.StringVarP(&workspace, workspaceFlagLong, workspaceFlagShort, "", "Discard the plan for this workspace.")


you'll need to remove the flags -p, etc. here if you're only doing the whole PR

Yup good spot 👍 Been a long time I edited this file and totally forgot about it...

parmouraly · 2020-06-14T14:25:23Z

I've converted this back to draft because I'm missing quite a few tests still... Coming soon

parmouraly · 2020-06-19T14:57:58Z

@lkysow this is a working version 🎉
As in it seems to work fine when manually testing on my machine
I think I've addressed all your comments so far. Please let me know if I can cover anything else.

In the mean time I'll see if I can push a couple more unit tests for models and comment_parser

Also supply usage tip when unlock is used incorrectly

lkysow · 2020-06-20T05:34:54Z

server/events/delete_lock_command.go

+			l.Logger.Err("unable to delete project status: %s", err)
+		}
+	}
+}


great work on this command.

lkysow · 2020-06-20T05:35:52Z

server/events/markdown_renderer.go

@@ -249,7 +248,8 @@ var planSuccessWrappedTmpl = template.Must(template.New("").Parse(
 // to do next.
 var planNextSteps = "{{ if .PlanWasDeleted }}This plan was not saved because one or more projects failed and automerge requires all plans pass.{{ else }}* :arrow_forward: To **apply** this plan, comment:\n" +
 	"    * `{{.ApplyCmd}}`\n" +
-	"* :put_litter_in_its_place: To **delete** this plan click [here]({{.LockURL}})\n" +
+	"* :put_litter_in_its_place: To **delete** this plan click [here]({{.LockURL}}), or to delete all plans and atlantis locks comment:\n" +
+	"    * `atlantis unlock`\n" +


I'm going to move this to the bottom where we talk about running atlantis apply to apply all plans because unlocking all plans makes more sense there than nested with each individual plan

Ah yea good point didn't think about it like that. I just kept it in the already existing delete section to avoid adding a new bullet point but yea sounds good 👍

It's just gonna be a bit a of a pain to update the renderer_tests :-D

lkysow · 2020-06-20T05:36:51Z

server/events/models/models.go

 	}
 	return ""
 }
+
+// DeleteLockCommandResult is the result of attempting to delete an atlantis lock.
+type DeleteLockCommandResult int


I've made some changes that make this unnecessary, mainly following the pattern before where error means error and nil means lock not found.

lkysow · 2020-06-24T04:54:34Z

Merged with #1091! Great work 🎉

parmouraly changed the title ~~Feature discard plan via comment~~ [WIP] Discard plan and Atlantis lock via VCS comment Apr 22, 2020

parmouraly marked this pull request as draft April 22, 2020 17:08

parmouraly force-pushed the feature-discard-plan-via-comment branch from 054c119 to 65923b4 Compare April 22, 2020 17:16

matthewellis-pp mentioned this pull request May 18, 2020

Feature Request: "atlantis unlock" #732

Closed

lkysow reviewed May 25, 2020

View reviewed changes

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

parmouraly changed the title ~~[WIP] Discard plan and Atlantis lock via VCS comment~~ Discard plan and Atlantis lock via VCS comment Jun 8, 2020

parmouraly marked this pull request as ready for review June 8, 2020 14:48

Paris Morali and others added 5 commits June 8, 2020 15:56

Update server/events/comment_parser.go

889d1aa

Co-authored-by: Luke Kysow <[email protected]>

Update server/events/comment_parser.go

b2cf8ff

Co-authored-by: Luke Kysow <[email protected]>

Some polishing up and removing unneeded changes

56fc1a2

parmouraly force-pushed the feature-discard-plan-via-comment branch from 02b1d4b to 56fc1a2 Compare June 8, 2020 15:00

Some more polishing up and made a start with tests

aebb65e

parmouraly commented Jun 8, 2020

View reviewed changes

server/events/delete_lock_command_test.go Show resolved Hide resolved

parmouraly commented Jun 8, 2020

View reviewed changes

parmouraly added 2 commits June 9, 2020 11:07

Adapt renderer tests to new unlock command

60c883f

Remove diff from project command runner

a8793fa

Just trying to keep this as simple as possible

parmouraly force-pushed the feature-discard-plan-via-comment branch from 2f728d1 to a8793fa Compare June 9, 2020 12:35

parmouraly added 2 commits June 9, 2020 13:58

Extend testfixtures with new atlantis unlock comment

4d12e17

Fix linting issues

bba7c06

parmouraly force-pushed the feature-discard-plan-via-comment branch from 4178426 to 58c0c9e Compare June 9, 2020 13:25

parmouraly added 2 commits June 9, 2020 15:42

Add more tests for the new deletelockcommand

0f4d3d2

Fix lint problem

465eb09

lkysow reviewed Jun 9, 2020

View reviewed changes

parmouraly added 3 commits June 10, 2020 21:12

Add some more tests and integrate PR feedback

4c168c1

Just a commit to retrigger the build

40f7778

Fix help doc for new unlock command

1fcea31

parmouraly marked this pull request as draft June 14, 2020 14:24

parmouraly marked this pull request as ready for review June 19, 2020 13:13

Command runner tests

0d5d143

parmouraly marked this pull request as draft June 19, 2020 13:56

Unlock command now works, also rename discard->unlock

8fb4404

parmouraly marked this pull request as ready for review June 19, 2020 14:12

Fix forgotten test command rename

0ecaacc

parmouraly changed the title ~~Discard plan and Atlantis lock via VCS comment~~ Atlantis unlock and discard all plans via VCS comment Jun 19, 2020

parmouraly added 3 commits June 19, 2020 15:26

Re-introduce UnlockCmd

271efd0

Fix markdown renderer tests

66f07ba

Fix comment typo and some more polishing

cb9e349

Add tests for models and command parser

98a5bea

Also supply usage tip when unlock is used incorrectly

lkysow reviewed Jun 20, 2020

View reviewed changes

server/events/delete_lock_command.go

l.Logger.Err("unable to delete project status: %s", err)

}

}

}

Copy link

Member

lkysow Jun 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great work on this command.

lkysow reviewed Jun 20, 2020

View reviewed changes

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

This was referenced Jun 24, 2020

Unlock cmd #1090

Merged

Unlock cmd #1091

Merged

lkysow closed this Jun 24, 2020

		atlantis
		temp.sh

Atlantis unlock and discard all plans via VCS comment #1003

Atlantis unlock and discard all plans via VCS comment #1003

Conversation

parmouraly commented Apr 22, 2020 • edited Loading

lkysow commented Apr 22, 2020

parmouraly commented Apr 23, 2020

parmouraly commented Apr 23, 2020

matthewellis-pp commented May 18, 2020

parmouraly commented May 18, 2020

lkysow commented May 25, 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

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

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

parmouraly Jun 8, 2020 • edited Loading

Choose a reason for hiding this comment

parmouraly Jun 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parmouraly commented Jun 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parmouraly commented Jun 14, 2020

parmouraly commented Jun 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parmouraly Jun 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lkysow commented Jun 24, 2020

parmouraly commented Apr 22, 2020 •

edited

Loading

parmouraly commented Jun 8, 2020 •

edited

Loading

parmouraly Jun 8, 2020 •

edited

Loading

parmouraly Jun 8, 2020 •

edited

Loading

parmouraly Jun 20, 2020 •

edited

Loading