From bdca8a35ef3bab090e4287317729eb4ac109244e Mon Sep 17 00:00:00 2001 From: davidwin93 Date: Tue, 17 Oct 2023 14:23:30 -0600 Subject: [PATCH] address code review comments Signed-off-by: davidwin93 --- pkg/comment_formatter/tfc_status_update.go | 5 +- pkg/runstream/run_metadata.go | 3 +- pkg/tfc_trigger/tfc_trigger.go | 10 +- pkg/tfc_trigger/tfc_trigger_test.go | 146 +++++++++++++++++++++ pkg/vcs/github/run_events_worker.go | 2 +- pkg/vcs/gitlab/mr_status_updater.go | 3 +- 6 files changed, 160 insertions(+), 9 deletions(-) diff --git a/pkg/comment_formatter/tfc_status_update.go b/pkg/comment_formatter/tfc_status_update.go index c5fff97..7ae132c 100644 --- a/pkg/comment_formatter/tfc_status_update.go +++ b/pkg/comment_formatter/tfc_status_update.go @@ -9,11 +9,10 @@ import ( "github.com/zapier/tfbuddy/pkg/runstream" "github.com/zapier/tfbuddy/pkg/terraform_plan" "github.com/zapier/tfbuddy/pkg/tfc_api" - "github.com/zapier/tfbuddy/pkg/vcs" ) func getProperApplyText(rmd runstream.RunMetadata, wsName string) string { - if rmd.GetAutoMerge() && vcs.IsGlobalAutoMergeEnabled() { + if rmd.GetAutoMerge() { return fmt.Sprintf(howToApplyFormat, wsName, autoMRMergeSnippet) } else { return fmt.Sprintf(howToApplyFormat, wsName, manualMRMergeSnippet) @@ -21,7 +20,7 @@ func getProperApplyText(rmd runstream.RunMetadata, wsName string) string { } func getProperTargetedApplyText(rmd runstream.RunMetadata, run *tfe.Run, wsName string) string { targets := strings.Join(run.TargetAddrs, ",") - if rmd.GetAutoMerge() && vcs.IsGlobalAutoMergeEnabled() { + if rmd.GetAutoMerge() { return fmt.Sprintf(howToApplyFormatWithTarget, targets, wsName, targets, autoMRMergeSnippet) } else { return fmt.Sprintf(howToApplyFormatWithTarget, targets, wsName, targets, manualMRMergeSnippet) diff --git a/pkg/runstream/run_metadata.go b/pkg/runstream/run_metadata.go index a266fa4..ab35711 100644 --- a/pkg/runstream/run_metadata.go +++ b/pkg/runstream/run_metadata.go @@ -5,6 +5,7 @@ import ( "time" "github.com/nats-io/nats.go" + "github.com/zapier/tfbuddy/pkg/vcs" ) // ensure type complies with interface @@ -78,7 +79,7 @@ func (r *TFRunMetadata) GetVcsProvider() string { return r.VcsProvider } func (r *TFRunMetadata) GetAutoMerge() bool { - return r.AutoMerge + return r.AutoMerge && vcs.IsGlobalAutoMergeEnabled() } func (s *Stream) AddRunMeta(rmd RunMetadata) error { b, err := encodeTFRunMetadata(rmd) diff --git a/pkg/tfc_trigger/tfc_trigger.go b/pkg/tfc_trigger/tfc_trigger.go index b6fc472..2f051b1 100644 --- a/pkg/tfc_trigger/tfc_trigger.go +++ b/pkg/tfc_trigger/tfc_trigger.go @@ -639,8 +639,14 @@ func (t *TFCTrigger) publishRunToStream(ctx context.Context, run *tfe.Run, cfgWS 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", + AutoMerge: cfgWS.AutoMerge, + } + //disable Auto Merge and log if the mode is not apply-before-merge + if cfgWS.Mode != "apply-before-merge" && cfgWS.AutoMerge { + log.Info().Str("RunID", run.ID). + Str("Org", run.Workspace.Organization.Name). + Str("WS", run.Workspace.Name).Msg("cannot enable auto merge since mode is not apply-before-merge") + rmd.AutoMerge = false } err := t.runstream.AddRunMeta(rmd) if err != nil { diff --git a/pkg/tfc_trigger/tfc_trigger_test.go b/pkg/tfc_trigger/tfc_trigger_test.go index 22ede56..d09db7a 100644 --- a/pkg/tfc_trigger/tfc_trigger_test.go +++ b/pkg/tfc_trigger/tfc_trigger_test.go @@ -10,6 +10,7 @@ import ( "github.com/rs/zerolog/log" "github.com/rzajac/zltest" "github.com/zapier/tfbuddy/pkg/mocks" + "github.com/zapier/tfbuddy/pkg/runstream" "github.com/zapier/tfbuddy/pkg/tfc_api" "github.com/zapier/tfbuddy/pkg/tfc_trigger" "go.opentelemetry.io/otel" @@ -579,3 +580,148 @@ func TestTFCEvents_MultiWorkspaceApplyModifiedBothSrcDstBranches(t *testing.T) { } } + +func TestAutoMerge_False_Merge_Before_Apply(t *testing.T) { + ws := &tfc_trigger.ProjectConfig{ + Workspaces: []*tfc_trigger.TFCWorkspace{{ + Name: "service-tfbuddy", + Organization: "zapier-test", + Mode: "merge-before-apply", + AutoMerge: true, + }}} + + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + testSuite := mocks.CreateTestSuite(mockCtrl, mocks.TestOverrides{ProjectConfig: ws}, t) + + testSuite.MockGitRepo.EXPECT().GetModifiedFileNamesBetweenCommits(testSuite.MetaData.CommonSHA, "main").Return([]string{}, nil) + testSuite.MockApiClient.EXPECT().CreateRunFromSource(gomock.Any(), gomock.Any()).Return(&tfe.Run{ + ID: "101", + Workspace: &tfe.Workspace{Name: "service-tfbuddy", + Organization: &tfe.Organization{Name: "zapier-test"}, + }, + ConfigurationVersion: &tfe.ConfigurationVersion{Speculative: false}}, nil) + + testSuite.MockStreamClient.EXPECT().AddRunMeta(&runstream.TFRunMetadata{ + RunID: "101", + Organization: "zapier-test", + Workspace: "service-tfbuddy", + Source: "merge_request", + Action: "apply", + CommitSHA: "abcd12233", + MergeRequestProjectNameWithNamespace: testSuite.MetaData.ProjectNameNS, + MergeRequestIID: testSuite.MetaData.MRIID, + DiscussionID: "201", + RootNoteID: 301, + VcsProvider: "", + AutoMerge: false, + }) + + testSuite.InitTestSuite() + testLogger := zltest.New(t) + log.Logger = log.Logger.Output(testLogger) + + tCfg, _ := tfc_trigger.NewTFCTriggerConfig(&tfc_trigger.TFCTriggerOptions{ + Action: tfc_trigger.ApplyAction, + Branch: "test-branch", + CommitSHA: "abcd12233", + ProjectNameWithNamespace: testSuite.MetaData.ProjectNameNS, + MergeRequestIID: testSuite.MetaData.MRIID, + TriggerSource: tfc_trigger.CommentTrigger, + }) + trigger := tfc_trigger.NewTFCTrigger(testSuite.MockGitClient, testSuite.MockApiClient, testSuite.MockStreamClient, tCfg) + ctx, _ := otel.Tracer("FAKE").Start(context.Background(), "TEST") + triggeredWS, err := trigger.TriggerTFCEvents(ctx) + if err != nil { + t.Fatal(err) + return + } + lastEntry := testLogger.LastEntry() + if lastEntry == nil { + t.Fatal("expected log message not nil") + return + } + lastEntry.ExpMsg("cannot enable auto merge since mode is not apply-before-merge") + + if len(triggeredWS.Errored) != 0 { + t.Fatal("expected no failed workspaces") + } + if len(triggeredWS.Executed) == 0 { + t.Fatal("expected successful triggers") + } + if triggeredWS.Executed[0] != "service-tfbuddy" { + t.Fatal("expected workspace", triggeredWS.Errored[0].Name) + } +} +func TestAutoMerge_True_Apply_Before_Merge(t *testing.T) { + ws := &tfc_trigger.ProjectConfig{ + Workspaces: []*tfc_trigger.TFCWorkspace{{ + Name: "service-tfbuddy", + Organization: "zapier-test", + Mode: "apply-before-merge", + AutoMerge: true, + }}} + + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + testSuite := mocks.CreateTestSuite(mockCtrl, mocks.TestOverrides{ProjectConfig: ws}, t) + + testSuite.MockGitRepo.EXPECT().GetModifiedFileNamesBetweenCommits(testSuite.MetaData.CommonSHA, "main").Return([]string{}, nil) + testSuite.MockApiClient.EXPECT().CreateRunFromSource(gomock.Any(), gomock.Any()).Return(&tfe.Run{ + ID: "101", + Workspace: &tfe.Workspace{Name: "service-tfbuddy", + Organization: &tfe.Organization{Name: "zapier-test"}, + }, + ConfigurationVersion: &tfe.ConfigurationVersion{Speculative: false}}, nil) + + testSuite.MockStreamClient.EXPECT().AddRunMeta(&runstream.TFRunMetadata{ + RunID: "101", + Organization: "zapier-test", + Workspace: "service-tfbuddy", + Source: "merge_request", + Action: "apply", + CommitSHA: "abcd12233", + MergeRequestProjectNameWithNamespace: testSuite.MetaData.ProjectNameNS, + MergeRequestIID: testSuite.MetaData.MRIID, + DiscussionID: "201", + RootNoteID: 301, + VcsProvider: "", + AutoMerge: true, + }) + + testSuite.InitTestSuite() + testLogger := zltest.New(t) + log.Logger = log.Logger.Output(testLogger) + + tCfg, _ := tfc_trigger.NewTFCTriggerConfig(&tfc_trigger.TFCTriggerOptions{ + Action: tfc_trigger.ApplyAction, + Branch: "test-branch", + CommitSHA: "abcd12233", + ProjectNameWithNamespace: testSuite.MetaData.ProjectNameNS, + MergeRequestIID: testSuite.MetaData.MRIID, + TriggerSource: tfc_trigger.CommentTrigger, + }) + trigger := tfc_trigger.NewTFCTrigger(testSuite.MockGitClient, testSuite.MockApiClient, testSuite.MockStreamClient, tCfg) + ctx, _ := otel.Tracer("FAKE").Start(context.Background(), "TEST") + triggeredWS, err := trigger.TriggerTFCEvents(ctx) + if err != nil { + t.Fatal(err) + return + } + lastEntry := testLogger.LastEntry() + if lastEntry == nil { + t.Fatal("expected log message not nil") + return + } + lastEntry.ExpMsg("created TFC run") + + if len(triggeredWS.Errored) != 0 { + t.Fatal("expected no failed workspaces") + } + if len(triggeredWS.Executed) == 0 { + t.Fatal("expected successful triggers") + } + if triggeredWS.Executed[0] != "service-tfbuddy" { + t.Fatal("expected workspace", triggeredWS.Errored[0].Name) + } +} diff --git a/pkg/vcs/github/run_events_worker.go b/pkg/vcs/github/run_events_worker.go index a8bd329..dfb8c5d 100644 --- a/pkg/vcs/github/run_events_worker.go +++ b/pkg/vcs/github/run_events_worker.go @@ -90,7 +90,7 @@ func (w *RunEventsWorker) postRunStatusComment(ctx context.Context, run *tfe.Run } } func (w *RunEventsWorker) mergePRIfPossible(ctx context.Context, rmd runstream.RunMetadata) { - if !rmd.GetAutoMerge() || !vcs.IsGlobalAutoMergeEnabled() { + if !rmd.GetAutoMerge() { return } w.client.MergeMR(ctx, rmd.GetMRInternalID(), rmd.GetMRProjectNameWithNamespace()) diff --git a/pkg/vcs/gitlab/mr_status_updater.go b/pkg/vcs/gitlab/mr_status_updater.go index 488ac7c..1799add 100644 --- a/pkg/vcs/gitlab/mr_status_updater.go +++ b/pkg/vcs/gitlab/mr_status_updater.go @@ -11,7 +11,6 @@ import ( "github.com/rs/zerolog/log" gogitlab "github.com/xanzy/go-gitlab" "github.com/zapier/tfbuddy/pkg/runstream" - "github.com/zapier/tfbuddy/pkg/vcs" "go.opentelemetry.io/otel" ) @@ -202,7 +201,7 @@ func (p *RunStatusUpdater) mergeMRIfPossible(ctx context.Context, rmd runstream. ctx, span := otel.Tracer("TFC").Start(ctx, "mergeMRIfPossible") defer span.End() - if !rmd.GetAutoMerge() || !vcs.IsGlobalAutoMergeEnabled() { + if !rmd.GetAutoMerge() { return }