Merge on successful apply #39

davidwin93 · 2023-10-13T16:50:30Z

Resolves issue #19

Remaining work:

add config option at global and project level to allow disabling this feature [x]
update comments posted to Github/Gitlab that indicate if auto merge is enabled [x]

This will only merge after a full apply. This allows multiple targeted applies to be used without concern that the MR will be auto merged.

Tested the following situations:

autoMerge disabled and enabled in .tfbuddy config
TFBUDDY_ALLOW_AUTO_MERGE in deployment set to false and not present
targeted plans and applies to ensure that a MR is only merged after a full apply

pkg/vcs/github/run_events_worker.go

pkg/vcs/gitlab/mr_status_updater.go

Signed-off-by: davidwin93 <dave.winiarski@zapier.com>

djeebus · 2023-10-17T17:39:25Z

pkg/tfc_trigger/tfc_trigger.go

@@ -639,6 +639,8 @@ func (t *TFCTrigger) publishRunToStream(ctx context.Context, run *tfe.Run) error
 		DiscussionID:                         t.GetMergeRequestDiscussionID(),
 		RootNoteID:                           t.GetMergeRequestRootNoteID(),
 		VcsProvider:                          t.GetVcsProvider(),
+		//set Auto Merge if both conditions are met.
+		AutoMerge: cfgWS.AutoMerge && cfgWS.Mode == "apply-before-merge",


do we want to surface some kind of error message if auto merge is configured for "merge-before-apply" or "tfc-vcs-repo"?

oh, similar question for "automerge=true" but global auto merge is disabled.

We now log if either condition occurs. Plus the help text in the MR will indicate that a manual merge is required.

djeebus · 2023-10-17T17:41:44Z

pkg/comment_formatter/tfc_status_update.go

 )

+func getProperApplyText(rmd runstream.RunMetadata, wsName string) string {
+	if rmd.GetAutoMerge() && vcs.IsGlobalAutoMergeEnabled() {


rather than check both rmd.GetAutoMerge() && vcs.IsGlobalAutoMergeEnabled() in 5 different places, any reason to either put the global check into GetAutoMerge or check it once before creating the runstream.TFRunMetadata in tfc_trigger.go?

I like the idea of putting it in GetAutoMerge. That way if there's a long queued run and the global setting changes it would be respected.

djeebus · 2023-10-18T11:06:48Z

docs/usage.md

@@ -50,6 +50,8 @@ env:
  TFBUDDY_PROJECT_ALLOW_LIST: tfc-project/
  TFBUDDY_WORKSPACE_ALLOW_LIST: tfc-workspace
  TFBUDDY_DEFAULT_TFC_ORGANIZATION: companyX
+  # Optional setting to disable auto merging MRs after a successful apply. This is enabled by default.
+  TFBUDDY_ENABLE_AUTO_MERGE: "false"


sorry, one last thing. since this doesn't enable it, but rather prevents it from being enabled per repo, can we rename this "ALLOW_AUTO_MERGE", or something similar?

Signed-off-by: davidwin93 <dave.winiarski@zapier.com>

davidwin93 · 2023-10-19T16:24:38Z

If an apply returns no changes we will still auto merge

Gitlab:

Github:

Signed-off-by: davidwin93 <dave.winiarski@zapier.com>

…for nats dedupe timeout Signed-off-by: davidwin93 <dave.winiarski@zapier.com>

djeebus

looks good to me! only thing i can see in your most recent commits is the rmd.GetAction() == "apply" is kind of scary, it'd be nice to have an enum or something there, but go kind of suck w/ enums. maybe just a constant to avoid the strings? not important though, still approved!

Signed-off-by: davidwin93 <dave.winiarski@zapier.com>

davidwin93 force-pushed the Merge-On-Apply branch 3 times, most recently from 0d98b79 to 0779625 Compare October 13, 2023 17:22

djeebus reviewed Oct 14, 2023

View reviewed changes

pkg/vcs/github/run_events_worker.go Outdated Show resolved Hide resolved

djeebus reviewed Oct 14, 2023

View reviewed changes

pkg/vcs/gitlab/mr_status_updater.go Outdated Show resolved Hide resolved

djeebus approved these changes Oct 14, 2023

View reviewed changes

davidwin93 marked this pull request as draft October 14, 2023 03:39

davidwin93 added 4 commits October 16, 2023 10:34

migrate to go uber mock

bdf19d0

Signed-off-by: davidwin93 <dave.winiarski@zapier.com>

add merge PR support to Github

9800c3f

Signed-off-by: davidwin93 <dave.winiarski@zapier.com>

add merge MR support to Gitlab

d69cab3

Signed-off-by: davidwin93 <dave.winiarski@zapier.com>

update dependencies

45797cf

Signed-off-by: davidwin93 <dave.winiarski@zapier.com>

davidwin93 force-pushed the Merge-On-Apply branch from 0779625 to 9f7e0e2 Compare October 16, 2023 16:34

davidwin93 added 2 commits October 16, 2023 11:26

add check on project and global levels if auto merge is enabled

8071bdd

Signed-off-by: davidwin93 <dave.winiarski@zapier.com>

update MR comments to indicate if auto merge is enabled

66c18e4

Signed-off-by: davidwin93 <dave.winiarski@zapier.com>

davidwin93 force-pushed the Merge-On-Apply branch from 874a425 to 66c18e4 Compare October 16, 2023 17:26

davidwin93 added 2 commits October 17, 2023 10:02

add test coverage for project level auto merge

e1c394b

Signed-off-by: davidwin93 <dave.winiarski@zapier.com>

refactor help text for MR comments

2438df8

Signed-off-by: davidwin93 <dave.winiarski@zapier.com>

davidwin93 force-pushed the Merge-On-Apply branch from 2424d9a to 2438df8 Compare October 17, 2023 16:02

update docs

3073ecb

Signed-off-by: davidwin93 <dave.winiarski@zapier.com>

davidwin93 marked this pull request as ready for review October 17, 2023 16:07

davidwin93 requested a review from djeebus October 17, 2023 16:07

djeebus reviewed Oct 17, 2023

View reviewed changes

davidwin93 force-pushed the Merge-On-Apply branch 2 times, most recently from bdca8a3 to 58bf1cb Compare October 18, 2023 02:07

djeebus reviewed Oct 18, 2023

View reviewed changes

address code review comments

eaed084

Signed-off-by: davidwin93 <dave.winiarski@zapier.com>

davidwin93 force-pushed the Merge-On-Apply branch from 58bf1cb to eaed084 Compare October 18, 2023 13:06

update env var that allows auto merge

9afdb2d

Signed-off-by: davidwin93 <dave.winiarski@zapier.com>

davidwin93 requested a review from djeebus October 18, 2023 15:03

auto merge if apply has no changes

6495533

Signed-off-by: davidwin93 <dave.winiarski@zapier.com>

davidwin93 added 3 commits October 19, 2023 12:28

check if apply was targeted before merging even if empty.

aed8c9e

Signed-off-by: davidwin93 <dave.winiarski@zapier.com>

add additional unit tests for auto merge

7b842ef

Signed-off-by: davidwin93 <dave.winiarski@zapier.com>

add commit sha to msg id to allow new commits to run without waiting …

cb5681c

…for nats dedupe timeout Signed-off-by: davidwin93 <dave.winiarski@zapier.com>

djeebus approved these changes Oct 23, 2023

View reviewed changes

use constants instead of hardcoded values

b2892d0

Signed-off-by: davidwin93 <dave.winiarski@zapier.com>

djeebus approved these changes Oct 24, 2023

View reviewed changes

davidwin93 merged commit 5c713dc into main Oct 24, 2023
4 checks passed

davidwin93 deleted the Merge-On-Apply branch October 24, 2023 16:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge on successful apply #39

Merge on successful apply #39

davidwin93 commented Oct 13, 2023 •

edited

Loading

djeebus Oct 17, 2023

djeebus Oct 17, 2023

davidwin93 Oct 18, 2023

djeebus Oct 17, 2023

davidwin93 Oct 17, 2023

djeebus Oct 18, 2023

davidwin93 commented Oct 19, 2023

djeebus left a comment

Merge on successful apply #39

Merge on successful apply #39

Conversation

davidwin93 commented Oct 13, 2023 • edited Loading

djeebus Oct 17, 2023

Choose a reason for hiding this comment

djeebus Oct 17, 2023

Choose a reason for hiding this comment

davidwin93 Oct 18, 2023

Choose a reason for hiding this comment

djeebus Oct 17, 2023

Choose a reason for hiding this comment

davidwin93 Oct 17, 2023

Choose a reason for hiding this comment

djeebus Oct 18, 2023

Choose a reason for hiding this comment

davidwin93 commented Oct 19, 2023

djeebus left a comment

Choose a reason for hiding this comment

davidwin93 commented Oct 13, 2023 •

edited

Loading