Skip to content

Commit

Permalink
address code review comments
Browse files Browse the repository at this point in the history 
Signed-off-by: davidwin93 <dave.winiarski@zapier.com>
  • Loading branch information
@davidwin93
davidwin93 committed Oct 18, 2023
1 parent 3073ecb commit eaed084
Show file tree
Hide file tree
Showing 6 changed files with 246 additions and 9 deletions.
5 changes: 2 additions & 3 deletions pkg/comment_formatter/tfc_status_update.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,19 +9,18 @@ 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)
}
}
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)
Expand Down
3 changes: 2 additions & 1 deletion pkg/runstream/run_metadata.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"time"

"github.com/nats-io/nats.go"
"github.com/zapier/tfbuddy/pkg/vcs"
)

// ensure type complies with interface
Expand Down Expand Up @@ -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)
Expand Down
16 changes: 14 additions & 2 deletions pkg/tfc_trigger/tfc_trigger.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -639,8 +639,20 @@ 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("auto-merge cannot be enabled because the 'apply-before-merge' mode is not in use")
rmd.AutoMerge = false
}
if cfgWS.AutoMerge && !vcs.IsGlobalAutoMergeEnabled() {
log.Info().Str("RunID", run.ID).
Str("Org", run.Workspace.Organization.Name).
Str("WS", run.Workspace.Name).Msg("auto-merge cannot be enabled since the feature is globally disabled")
rmd.AutoMerge = false
}
err := t.runstream.AddRunMeta(rmd)
if err != nil {
Expand Down
226 changes: 226 additions & 0 deletions pkg/tfc_trigger/tfc_trigger_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,18 @@ package tfc_trigger_test
import (
"context"
"fmt"
"os"
"testing"
"time"

"github.com/hashicorp/go-tfe"
"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"
"github.com/zapier/tfbuddy/pkg/vcs"
"go.opentelemetry.io/otel"
"go.uber.org/mock/gomock"
)
Expand Down Expand Up @@ -579,3 +582,226 @@ 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("auto-merge cannot be enabled because the 'apply-before-merge' mode is not in use")

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)
}
}

func TestAutoMerge_Globally_Disabled(t *testing.T) {
originalVal := os.Getenv(vcs.TF_BUDDY_AUTO_MERGE)
os.Setenv(vcs.TF_BUDDY_AUTO_MERGE, "false")
defer func() { os.Setenv(vcs.TF_BUDDY_AUTO_MERGE, originalVal) }()

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: 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("auto-merge cannot be enabled since the feature is globally disabled")

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)
}
}
2 changes: 1 addition & 1 deletion pkg/vcs/github/run_events_worker.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand Down
3 changes: 1 addition & 2 deletions pkg/vcs/gitlab/mr_status_updater.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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
}

Expand Down

0 comments on commit eaed084

Please sign in to comment.