diff --git a/pkg/tfc_trigger/tfc_trigger.go b/pkg/tfc_trigger/tfc_trigger.go index ffe3279..d1d5304 100644 --- a/pkg/tfc_trigger/tfc_trigger.go +++ b/pkg/tfc_trigger/tfc_trigger.go @@ -33,9 +33,9 @@ const ( InvalidAction ) -const tfPrefix = "tfbuddylock" +const TFLockTagPrefix = "tfbuddylock" -var tagRegex = regexp.MustCompile(fmt.Sprintf("%s\\-(\\d+)", tfPrefix)) +var tagRegex = regexp.MustCompile(fmt.Sprintf("%s\\-(\\d+)", TFLockTagPrefix)) func (a TriggerAction) String() string { switch a { @@ -225,6 +225,23 @@ func FindLockingMR(ctx context.Context, tags []string, thisMR string) string { return "" } +// buildLockingMRURL constructs the full MR URL for a locking MR by replacing +// the IID at the end of the current MR's URL with the locking MR's IID. +// e.g. currentMRWebURL="https://gitlab.com/org/repo/-/merge_requests/100", +// +// lockingMRIID="999" → "https://gitlab.com/org/repo/-/merge_requests/999" +func buildLockingMRURL(currentMRWebURL, lockingMRIID string) string { + if currentMRWebURL == "" { + log.Warn().Str("lockingMRIID", lockingMRIID).Msg("buildLockingMRURL: currentMRWebURL is empty, returning bare MR IID") + return lockingMRIID + } + idx := strings.LastIndex(currentMRWebURL, "/") + if idx < 0 { + return lockingMRIID + } + return currentMRWebURL[:idx+1] + lockingMRIID +} + // handleError both logs an error and reports it back to the Merge Request via an MR comment. // the returned error is identical to the input parameter as a convenience func (t *TFCTrigger) handleError(ctx context.Context, err error, msg string) error { @@ -249,7 +266,7 @@ func (t *TFCTrigger) getLockingMR(ctx context.Context, workspace string) string ctx, span := otel.Tracer("TFC").Start(ctx, "getLockingMR") defer span.End() - tags, err := t.tfc.GetTagsByQuery(ctx, workspace, tfPrefix) + tags, err := t.tfc.GetTagsByQuery(ctx, workspace, TFLockTagPrefix) if err != nil { log.Error().Err(err) } @@ -458,7 +475,7 @@ func (t *TFCTrigger) TriggerCleanupEvent(ctx context.Context) error { if err != nil { return fmt.Errorf("ignoring cleanup trigger for project, missing .tfbuddy.yaml. %w", err) } - tag := fmt.Sprintf("%s-%d", tfPrefix, mr.GetInternalID()) + tag := fmt.Sprintf("%s-%d", TFLockTagPrefix, mr.GetInternalID()) for _, cfgWS := range cfg.Workspaces { ws, err := t.tfc.GetWorkspaceByName(ctx, cfgWS.Organization, @@ -563,18 +580,23 @@ func (t *TFCTrigger) triggerRunForWorkspace(ctx context.Context, cfgWS *TFCWorks } else if t.GetAction() != PlanAction { return fmt.Errorf("run action was not apply or plan. %w", err) } - // If the workspace is locked tell the user and don't queue a run - // Otherwise, TFC wil queue an apply, which might put them out of order + // If the workspace is locked tell the user and don't queue a run. + // Otherwise, TFC will queue an apply, which might put them out of order. if isApply { lockingMR := t.getLockingMR(ctx, ws.ID) if ws.Locked { - return fmt.Errorf("refusing to Apply changes to a locked workspace. %w", err) + if lockingMR != "" { + lockingMRURL := buildLockingMRURL(mr.GetWebURL(), lockingMR) + return fmt.Errorf("refusing to Apply changes to a locked workspace. Lock is held by MR: %s", lockingMRURL) + } + return fmt.Errorf("refusing to Apply changes to a locked workspace. Check the TFC workspace UI for lock details.") } else if lockingMR != "" { - return fmt.Errorf("workspace is locked by another MR! %s", lockingMR) + lockingMRURL := buildLockingMRURL(mr.GetWebURL(), lockingMR) + return fmt.Errorf("refusing to Apply changes to a locked workspace. Lock is held by another MR: %s", lockingMRURL) } else { err = t.tfc.AddTags(ctx, ws.ID, - tfPrefix, + TFLockTagPrefix, fmt.Sprintf("%d", t.GetMergeRequestIID()), ) if err != nil { diff --git a/pkg/tfc_trigger/tfc_trigger_test.go b/pkg/tfc_trigger/tfc_trigger_test.go index 91dea9a..da8e0b0 100644 --- a/pkg/tfc_trigger/tfc_trigger_test.go +++ b/pkg/tfc_trigger/tfc_trigger_test.go @@ -807,6 +807,157 @@ func TestAutoMerge_Globally_Disabled(t *testing.T) { } } +func TestTFCEvents_ApplyBlockedByTFCLockWithLockingMR(t *testing.T) { + ws := &tfc_trigger.ProjectConfig{ + Workspaces: []*tfc_trigger.TFCWorkspace{{ + Name: "service-tfbuddy", + Organization: "zapier-test", + Mode: "apply-before-merge", + }}} + + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + testSuite := mocks.CreateTestSuite(mockCtrl, mocks.TestOverrides{ProjectConfig: ws}, t) + + // Register BEFORE InitTestSuite — first AnyTimes() wins in FIFO order. + testSuite.MockApiClient.EXPECT(). + GetWorkspaceByName(gomock.Any(), gomock.Any(), gomock.Any()). + DoAndReturn(func(ctx context.Context, org, name string) (*tfe.Workspace, error) { + return &tfe.Workspace{ID: name, Locked: true}, nil + }).AnyTimes() + testSuite.MockApiClient.EXPECT(). + GetTagsByQuery(gomock.Any(), gomock.Any(), "tfbuddylock"). + Return([]string{"tfbuddylock-999"}, nil).AnyTimes() + testSuite.MockGitMR.EXPECT(). + GetWebURL(). + Return("https://gitlab.com/zapier/tfbuddy/-/merge_requests/101").AnyTimes() + + testSuite.InitTestSuite() + + 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("expected no fatal error, got:", err) + } + if len(triggeredWS.Errored) == 0 { + t.Fatal("expected workspace to be in errored list") + } + wantURL := "https://gitlab.com/zapier/tfbuddy/-/merge_requests/999" + if !strings.Contains(triggeredWS.Errored[0].Error, wantURL) { + t.Fatalf("expected error to contain %q, got: %s", wantURL, triggeredWS.Errored[0].Error) + } +} + +func TestTFCEvents_ApplyBlockedByTFCLockNoTag(t *testing.T) { + ws := &tfc_trigger.ProjectConfig{ + Workspaces: []*tfc_trigger.TFCWorkspace{{ + Name: "service-tfbuddy", + Organization: "zapier-test", + Mode: "apply-before-merge", + }}} + + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + testSuite := mocks.CreateTestSuite(mockCtrl, mocks.TestOverrides{ProjectConfig: ws}, t) + + testSuite.MockApiClient.EXPECT(). + GetWorkspaceByName(gomock.Any(), gomock.Any(), gomock.Any()). + DoAndReturn(func(ctx context.Context, org, name string) (*tfe.Workspace, error) { + return &tfe.Workspace{ID: name, Locked: true}, nil + }).AnyTimes() + testSuite.MockApiClient.EXPECT(). + GetTagsByQuery(gomock.Any(), gomock.Any(), "tfbuddylock"). + Return([]string{}, nil).AnyTimes() + + testSuite.InitTestSuite() + + 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("expected no fatal error, got:", err) + } + if len(triggeredWS.Errored) == 0 { + t.Fatal("expected workspace to be in errored list") + } + wantSubstr := "refusing to Apply changes to a locked workspace" + if !strings.Contains(triggeredWS.Errored[0].Error, wantSubstr) { + t.Fatalf("expected error to contain %q, got: %s", wantSubstr, triggeredWS.Errored[0].Error) + } + if strings.Contains(triggeredWS.Errored[0].Error, "merge_requests") { + t.Fatalf("error should not reference merge_requests when no tag exists, got: %s", triggeredWS.Errored[0].Error) + } +} + +func TestTFCEvents_ApplyBlockedByTagLock(t *testing.T) { + ws := &tfc_trigger.ProjectConfig{ + Workspaces: []*tfc_trigger.TFCWorkspace{{ + Name: "service-tfbuddy", + Organization: "zapier-test", + Mode: "apply-before-merge", + }}} + + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + testSuite := mocks.CreateTestSuite(mockCtrl, mocks.TestOverrides{ProjectConfig: ws}, t) + + testSuite.MockApiClient.EXPECT(). + GetWorkspaceByName(gomock.Any(), gomock.Any(), gomock.Any()). + DoAndReturn(func(ctx context.Context, org, name string) (*tfe.Workspace, error) { + return &tfe.Workspace{ID: name, Locked: false}, nil + }).AnyTimes() + testSuite.MockApiClient.EXPECT(). + GetTagsByQuery(gomock.Any(), gomock.Any(), "tfbuddylock"). + Return([]string{"tfbuddylock-999"}, nil).AnyTimes() + testSuite.MockGitMR.EXPECT(). + GetWebURL(). + Return("https://gitlab.com/zapier/tfbuddy/-/merge_requests/101").AnyTimes() + + testSuite.InitTestSuite() + + 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("expected no fatal error, got:", err) + } + if len(triggeredWS.Errored) == 0 { + t.Fatal("expected workspace to be in errored list") + } + wantURL := "https://gitlab.com/zapier/tfbuddy/-/merge_requests/999" + if !strings.Contains(triggeredWS.Errored[0].Error, wantURL) { + t.Fatalf("expected error to contain %q, got: %s", wantURL, triggeredWS.Errored[0].Error) + } +} + // TestTFCEvents_ApplyNotBlockedByDifferentServiceChanges verifies that changes // on the target branch in a different service directory do not block applies for // unrelated workspaces. diff --git a/pkg/vcs/gitlab/mr_comment_updater.go b/pkg/vcs/gitlab/mr_comment_updater.go index d9b1bee..69e5846 100644 --- a/pkg/vcs/gitlab/mr_comment_updater.go +++ b/pkg/vcs/gitlab/mr_comment_updater.go @@ -11,12 +11,42 @@ import ( "github.com/rs/zerolog/log" "github.com/zapier/tfbuddy/pkg/runstream" + "github.com/zapier/tfbuddy/pkg/tfc_trigger" ) +// releaseWorkspaceLockTag removes the TFBuddy tag-based workspace lock when an +// apply-triggered run reaches a terminal state. This prevents stale lock tags +// from blocking future applies after a run finishes while the MR stays open. +func (p *RunStatusUpdater) releaseWorkspaceLockTag(ctx context.Context, run *tfe.Run, rmd runstream.RunMetadata) { + if rmd.GetAction() != runstream.ApplyAction { + return + } + // run.Workspace is always populated by GetRun (includes tfe.RunWorkspace). + switch run.Status { + case tfe.RunApplied, tfe.RunErrored, tfe.RunCanceled, tfe.RunDiscarded: + tag := fmt.Sprintf("%s-%d", tfc_trigger.TFLockTagPrefix, rmd.GetMRInternalID()) + if err := p.tfc.RemoveTagsByQuery(ctx, run.Workspace.ID, tag); err != nil { + log.Error().Err(err). + Str("workspace", run.Workspace.Name). + Str("workspaceID", run.Workspace.ID). + Int("mrIID", rmd.GetMRInternalID()). + Msg("could not remove workspace lock tag after apply completion") + } else { + log.Info(). + Str("workspace", run.Workspace.Name). + Int("mrIID", rmd.GetMRInternalID()). + Str("runStatus", string(run.Status)). + Msg("released workspace lock tag after apply reached terminal state") + } + } +} + func (p *RunStatusUpdater) postRunStatusComment(ctx context.Context, run *tfe.Run, rmd runstream.RunMetadata) { ctx, span := otel.Tracer("TFC").Start(ctx, "postRunStatusComment") defer span.End() + p.releaseWorkspaceLockTag(ctx, run, rmd) + commentBody, topLevelNoteBody, resolveDiscussion := comment_formatter.FormatRunStatusCommentBody(p.tfc, run, rmd) var oldUrls string diff --git a/pkg/vcs/gitlab/mr_status_updater_test.go b/pkg/vcs/gitlab/mr_status_updater_test.go index 04e3066..2ea43eb 100644 --- a/pkg/vcs/gitlab/mr_status_updater_test.go +++ b/pkg/vcs/gitlab/mr_status_updater_test.go @@ -3,6 +3,7 @@ package gitlab import ( "context" "errors" + "fmt" "os" "testing" @@ -171,3 +172,152 @@ func TestPolicySoftFailPlanFailsPipelineWhenEnvTrue(t *testing.T) { // Clean up env var for safety (though t.Setenv handles this) os.Unsetenv("TFBUDDY_FAIL_CI_ON_SENTINEL_SOFT_FAIL") } + +func TestPostRunStatusComment_RemovesLockTagOnApplyApplied(t *testing.T) { + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + testSuite := mocks.CreateTestSuite(mockCtrl, mocks.TestOverrides{}, t) + + wsID := "ws-abc123" + mrIID := testSuite.MetaData.MRIID // 101 + expectedTag := fmt.Sprintf("tfbuddylock-%d", mrIID) + + // Core assertion: tag must be removed when apply succeeds + testSuite.MockApiClient.EXPECT(). + RemoveTagsByQuery(gomock.Any(), wsID, expectedTag). + Return(nil). + Times(1) + + // RunApplied triggers UpdateMergeRequestDiscussionNote (topLevelNoteBody always non-empty) + testSuite.MockGitClient.EXPECT(). + UpdateMergeRequestDiscussionNote(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Return(mocks.NewMockMRNote(mockCtrl), nil).AnyTimes() + + // apply summary extraInfo → postComment → AddMergeRequestDiscussionReply + testSuite.MockGitClient.EXPECT(). + AddMergeRequestDiscussionReply(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Return(mocks.NewMockMRNote(mockCtrl), nil).AnyTimes() + + // resolveDiscussion=true for RunApplied without TargetAddrs + testSuite.MockGitClient.EXPECT(). + ResolveMergeRequestDiscussion(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Return(nil).AnyTimes() + + // GetOldRunUrls is NOT called for RunApplied + + r := &RunStatusUpdater{ + tfc: testSuite.MockApiClient, + client: testSuite.MockGitClient, + rs: testSuite.MockStreamClient, + } + r.postRunStatusComment(context.Background(), &tfe.Run{ + ID: "run-123", + Status: tfe.RunApplied, + Workspace: &tfe.Workspace{ + ID: wsID, + Name: "service-tfbuddy", + Organization: &tfe.Organization{Name: "zapier-test"}, + }, + Apply: &tfe.Apply{}, + }, &runstream.TFRunMetadata{ + Action: runstream.ApplyAction, + MergeRequestIID: mrIID, + MergeRequestProjectNameWithNamespace: testSuite.MetaData.ProjectNameNS, + DiscussionID: "disc-1", + RootNoteID: 301, + }) +} + +func TestPostRunStatusComment_RemovesLockTagOnApplyErrored(t *testing.T) { + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + testSuite := mocks.CreateTestSuite(mockCtrl, mocks.TestOverrides{}, t) + + wsID := "ws-abc123" + mrIID := testSuite.MetaData.MRIID + expectedTag := fmt.Sprintf("tfbuddylock-%d", mrIID) + + testSuite.MockApiClient.EXPECT(). + RemoveTagsByQuery(gomock.Any(), wsID, expectedTag). + Return(nil). + Times(1) + + // For RunErrored, GetOldRunUrls is called + testSuite.MockGitClient.EXPECT(). + GetOldRunUrls(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Return("", nil).AnyTimes() + + // topLevelNoteBody non-empty → UpdateMergeRequestDiscussionNote + testSuite.MockGitClient.EXPECT(). + UpdateMergeRequestDiscussionNote(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Return(mocks.NewMockMRNote(mockCtrl), nil).AnyTimes() + + // RunErrored with action=apply: extraInfo="" → no AddMergeRequestDiscussionReply + // resolveDiscussion=false → no ResolveMergeRequestDiscussion + + r := &RunStatusUpdater{ + tfc: testSuite.MockApiClient, + client: testSuite.MockGitClient, + rs: testSuite.MockStreamClient, + } + r.postRunStatusComment(context.Background(), &tfe.Run{ + ID: "run-456", + Status: tfe.RunErrored, + Workspace: &tfe.Workspace{ + ID: wsID, + Name: "service-tfbuddy", + Organization: &tfe.Organization{Name: "zapier-test"}, + }, + }, &runstream.TFRunMetadata{ + Action: runstream.ApplyAction, + MergeRequestIID: mrIID, + MergeRequestProjectNameWithNamespace: testSuite.MetaData.ProjectNameNS, + DiscussionID: "disc-1", + RootNoteID: 301, + }) +} + +func TestPostRunStatusComment_DoesNotRemoveLockTagForPlan(t *testing.T) { + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + testSuite := mocks.CreateTestSuite(mockCtrl, mocks.TestOverrides{}, t) + + // RemoveTagsByQuery must NOT be called for plan actions + testSuite.MockApiClient.EXPECT(). + RemoveTagsByQuery(gomock.Any(), gomock.Any(), gomock.Any()). + Times(0) + + // For RunErrored, GetOldRunUrls is called + testSuite.MockGitClient.EXPECT(). + GetOldRunUrls(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Return("", nil).AnyTimes() + + testSuite.MockGitClient.EXPECT(). + UpdateMergeRequestDiscussionNote(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Return(mocks.NewMockMRNote(mockCtrl), nil).AnyTimes() + + testSuite.MockGitClient.EXPECT(). + AddMergeRequestDiscussionReply(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Return(mocks.NewMockMRNote(mockCtrl), nil).AnyTimes() + + r := &RunStatusUpdater{ + tfc: testSuite.MockApiClient, + client: testSuite.MockGitClient, + rs: testSuite.MockStreamClient, + } + r.postRunStatusComment(context.Background(), &tfe.Run{ + ID: "run-789", + Status: tfe.RunErrored, + Workspace: &tfe.Workspace{ + ID: "ws-plan", + Name: "service-tfbuddy", + Organization: &tfe.Organization{Name: "zapier-test"}, + }, + }, &runstream.TFRunMetadata{ + Action: runstream.PlanAction, + MergeRequestIID: testSuite.MetaData.MRIID, + MergeRequestProjectNameWithNamespace: testSuite.MetaData.ProjectNameNS, + DiscussionID: "disc-1", + RootNoteID: 301, + }) +}