fix: scope merge cleanup to touched workspaces (#59)

Author: MeNsaaH

pkg/mocks/mock_tfc_api.go

@@ -22,6 +22,7 @@ type ApiClient interface {
 	LockUnlockWorkspace(ctx context.Context, workspace string, reason string, tag string, lock bool) error
 	AddTags(ctx context.Context, workspace string, prefix string, value string) error
 	RemoveTagsByQuery(ctx context.Context, workspace string, query string) error
+	RemoveTagsByName(ctx context.Context, workspace string, names []string) error
 	GetTagsByQuery(ctx context.Context, workspace string, query string) ([]string, error)
 }
 
@@ -201,6 +202,25 @@ func (t *TFCClient) RemoveTagsByQuery(ctx context.Context, workspace string, que
 	return nil
 }
 
+// RemoveTagsByName removes tags from a workspace by exact name. Unlike RemoveTagsByQuery,
+// it does not perform substring matching and is safe to use with values that share common
+// substrings (e.g. "tfbuddylock-5" vs "tfbuddylock-50"). Empty input is a no-op.
+func (t *TFCClient) RemoveTagsByName(ctx context.Context, workspace string, names []string) error {
+	ctx, span := otel.Tracer("TFC").Start(ctx, "RemoveTagsByName", trace.WithAttributes(
+		attribute.String("workspace", workspace),
+	))
+	defer span.End()
+
+	if len(names) == 0 {
+		return nil
+	}
+	tags := make([]*tfe.Tag, 0, len(names))
+	for _, name := range names {
+		tags = append(tags, &tfe.Tag{Name: name})
+	}
+	return t.Client.Workspaces.RemoveTags(ctx, workspace, tfe.WorkspaceRemoveTagsOptions{Tags: tags})
+}
+
 // GetTagsByQuery returns a list of values of tags on a terraform workspace matching the query string.
 // It operates on strings reporesenting the value of the tag and internally converts it to and from the upstreams tag struct as needed.  Attempting to query tags based on their tag ID will not match the tag.
 func (t *TFCClient) GetTagsByQuery(ctx context.Context, workspace string, query string) ([]string, error) {

pkg/tfc_api/api_client.go

@@ -454,13 +454,16 @@ func (t *TFCTrigger) TriggerCleanupEvent(ctx context.Context) error {
 	if err != nil {
 		return fmt.Errorf("could not read MergeRequest data from Gitlab API. %w", err)
 	}
-	var wsNames []string
-	cfg, err := getProjectConfigFile(ctx, t.gl, t)
+	triggeredWorkspaces, err := t.getTriggeredWorkspacesForRequest(ctx, mr)
 	if err != nil {
-		return fmt.Errorf("ignoring cleanup trigger for project, missing .tfbuddy.yaml. %w", err)
+		return fmt.Errorf("could not determine workspaces for merge cleanup. %w", err)
 	}
+	if len(triggeredWorkspaces) == 0 {
+		return nil
+	}
+	var wsNames []string
 	tag := fmt.Sprintf("%s-%d", tfPrefix, mr.GetInternalID())
-	for _, cfgWS := range cfg.Workspaces {
+	for _, cfgWS := range triggeredWorkspaces {
 		ws, err := t.tfc.GetWorkspaceByName(ctx,
 			cfgWS.Organization,
 			cfgWS.Name)
@@ -476,14 +479,27 @@ func (t *TFCTrigger) TriggerCleanupEvent(ctx context.Context) error {
 			t.handleError(ctx, err, "error getting tags")
 			continue
 		}
-		if len(tags) != 0 {
-			err = t.tfc.RemoveTagsByQuery(ctx, ws.ID, tag)
-			if err != nil {
-				t.handleError(ctx, err, "Error removing locking tag from workspace")
-				continue
+		// TFC's tag query is a substring match, so a query for "tfbuddylock-5"
+		// also returns "tfbuddylock-50" (locking MR #50). Filter for an exact
+		// match before treating the workspace as one this MR locked.
+		hasOurTag := false
+		for _, name := range tags {
+			if name == tag {
+				hasOurTag = true
+				break
 			}
-			wsNames = append(wsNames, cfgWS.Name)
 		}
+		if !hasOurTag {
+			continue
+		}
+		if err := t.tfc.RemoveTagsByName(ctx, ws.ID, []string{tag}); err != nil {
+			t.handleError(ctx, err, "Error removing locking tag from workspace")
+			continue
+		}
+		wsNames = append(wsNames, cfgWS.Name)
+	}
+	if len(wsNames) == 0 {
+		return nil
 	}
 	_, err = t.gl.CreateMergeRequestDiscussion(ctx, mr.GetInternalID(),
 		t.GetProjectNameWithNamespace(),

pkg/tfc_trigger/tfc_trigger.go

@@ -1104,44 +1104,87 @@ func TestTFCEvents_UnlockRemovesTags(t *testing.T) {
 	}
 }
 
-// TestTriggerCleanupEvent_ContinuesOnMissingWorkspace verifies that if one
-// workspace in the .tfbuddy.yaml config no longer exists in TFC, the cleanup
-// loop skips it and still removes the lock tag from the remaining workspaces
-// (rather than crashing with a nil pointer dereference on the missing ws).
-func TestTriggerCleanupEvent_ContinuesOnMissingWorkspace(t *testing.T) {
+// TestTriggerCleanupEvent_ScopesCleanupToTriggeredWorkspaces verifies that
+// merge cleanup only inspects workspaces touched by the MR. Unrelated
+// workspaces in .tfbuddy.yaml must not be looked up or produce MR noise.
+func TestTriggerCleanupEvent_ScopesCleanupToTriggeredWorkspaces(t *testing.T) {
 	ws := &tfc_trigger.ProjectConfig{
 		Workspaces: []*tfc_trigger.TFCWorkspace{
-			{Name: "deleted-ws", Organization: "zapier-test", Mode: "apply-before-merge"},
-			{Name: "service-tfbuddy", Organization: "zapier-test", Mode: "apply-before-merge"},
+			{Name: "deleted-ws", Organization: "zapier-test", Mode: "apply-before-merge", Dir: "deleted"},
+			{Name: "service-tfbuddy", Organization: "zapier-test", Mode: "apply-before-merge", Dir: "service"},
 		}}
 
 	mockCtrl := gomock.NewController(t)
 	defer mockCtrl.Finish()
 	testSuite := mocks.CreateTestSuite(mockCtrl, mocks.TestOverrides{ProjectConfig: ws}, t)
 
-	// First workspace is missing from TFC
-	testSuite.MockApiClient.EXPECT().GetWorkspaceByName(gomock.Any(), "zapier-test", "deleted-ws").
-		Return(nil, fmt.Errorf("resource not found"))
-	// Second workspace exists and has the locking tag
+	testSuite.MockGitMR.EXPECT().GetInternalID().Return(testSuite.MetaData.MRIID).AnyTimes()
+	testSuite.MockGitClient.EXPECT().GetMergeRequest(
+		gomock.Any(), testSuite.MetaData.MRIID, testSuite.MetaData.ProjectNameNS,
+	).Return(testSuite.MockGitMR, nil)
+	testSuite.MockGitClient.EXPECT().GetRepoFile(
+		gomock.Any(), testSuite.MetaData.ProjectNameNS, ".tfbuddy.yaml", testSuite.MetaData.SourceBranch,
+	).Return(testSuite.MetaData.TFBuddyConfig, nil)
+	testSuite.MockGitClient.EXPECT().GetMergeRequestModifiedFiles(
+		gomock.Any(), testSuite.MetaData.MRIID, testSuite.MetaData.ProjectNameNS,
+	).Return([]string{"service/main.tf"}, nil)
+
+	// Only the triggered workspace is cleaned up.
 	testSuite.MockApiClient.EXPECT().GetWorkspaceByName(gomock.Any(), "zapier-test", "service-tfbuddy").
 		Return(&tfe.Workspace{ID: "service-tfbuddy"}, nil)
 	testSuite.MockApiClient.EXPECT().GetTagsByQuery(gomock.Any(), "service-tfbuddy", "tfbuddylock-101").
 		Return([]string{"tfbuddylock-101"}, nil)
-	testSuite.MockApiClient.EXPECT().RemoveTagsByQuery(gomock.Any(), "service-tfbuddy", "tfbuddylock-101").
+	testSuite.MockApiClient.EXPECT().RemoveTagsByName(gomock.Any(), "service-tfbuddy", []string{"tfbuddylock-101"}).
 		Return(nil)
 
-	// handleError posts an MR comment when the first workspace fails
-	testSuite.MockGitClient.EXPECT().CreateMergeRequestComment(
-		gomock.Any(), testSuite.MetaData.MRIID, testSuite.MetaData.ProjectNameNS,
-		"Error: error getting workspace: resource not found",
-	).Return(nil)
-
-	// Summary discussion is posted with the successfully cleaned workspace
 	testSuite.MockGitClient.EXPECT().CreateMergeRequestDiscussion(
 		gomock.Any(), testSuite.MetaData.MRIID, testSuite.MetaData.ProjectNameNS,
 		"Released locks for workspaces: service-tfbuddy",
 	).Return(testSuite.MockGitDisc, nil)
 
+	tCfg, _ := tfc_trigger.NewTFCTriggerConfig(&tfc_trigger.TFCTriggerOptions{
+		Action:                   tfc_trigger.PlanAction,
+		Branch:                   testSuite.MetaData.SourceBranch,
+		CommitSHA:                "abcd12233",
+		ProjectNameWithNamespace: testSuite.MetaData.ProjectNameNS,
+		MergeRequestIID:          testSuite.MetaData.MRIID,
+		TriggerSource:            tfc_trigger.MergeRequestEventTrigger,
+	})
+	trigger := tfc_trigger.NewTFCTrigger(testSuite.MockGitClient, testSuite.MockApiClient, testSuite.MockStreamClient, tCfg)
+	ctx, _ := otel.Tracer("FAKE").Start(context.Background(), "TEST")
+	if err := trigger.TriggerCleanupEvent(ctx); err != nil {
+		t.Fatalf("cleanup should not fail when unrelated workspaces exist, got: %v", err)
+	}
+}
+
+// TestTriggerCleanupEvent_DoesNotReleaseOtherMRsLocks verifies that the
+// cleanup does not steal a tag from another MR whose IID happens to contain
+// the merging MR's IID as a substring. The TFC ListTags `q` parameter does
+// partial matching, so a query for "tfbuddylock-101" also returns
+// "tfbuddylock-1010" — cleanup must filter for an exact match before
+// treating the workspace as one this MR locked.
+func TestTriggerCleanupEvent_DoesNotReleaseOtherMRsLocks(t *testing.T) {
+	ws := &tfc_trigger.ProjectConfig{
+		Workspaces: []*tfc_trigger.TFCWorkspace{
+			{Name: "shared-ws", Organization: "zapier-test", Mode: "apply-before-merge"},
+		}}
+
+	mockCtrl := gomock.NewController(t)
+	defer mockCtrl.Finish()
+	testSuite := mocks.CreateTestSuite(mockCtrl, mocks.TestOverrides{ProjectConfig: ws}, t)
+
+	// MR #101 is merging. The workspace is locked by MR #1010 (different MR
+	// whose IID contains "101" as a substring). TFC's partial-match query
+	// returns the foreign tag.
+	testSuite.MockApiClient.EXPECT().GetWorkspaceByName(gomock.Any(), "zapier-test", "shared-ws").
+		Return(&tfe.Workspace{ID: "shared-ws"}, nil)
+	testSuite.MockApiClient.EXPECT().GetTagsByQuery(gomock.Any(), "shared-ws", "tfbuddylock-101").
+		Return([]string{"tfbuddylock-1010"}, nil)
+
+	// Cleanup MUST NOT remove the foreign tag and MUST NOT include the
+	// workspace in the summary. (No RemoveTagsByName / RemoveTagsByQuery
+	// call is expected; gomock fails the test if one is made.)
+
 	testSuite.InitTestSuite()
 
 	tCfg, _ := tfc_trigger.NewTFCTriggerConfig(&tfc_trigger.TFCTriggerOptions{
@@ -1155,7 +1198,7 @@ func TestTriggerCleanupEvent_ContinuesOnMissingWorkspace(t *testing.T) {
 	trigger := tfc_trigger.NewTFCTrigger(testSuite.MockGitClient, testSuite.MockApiClient, testSuite.MockStreamClient, tCfg)
 	ctx, _ := otel.Tracer("FAKE").Start(context.Background(), "TEST")
 	if err := trigger.TriggerCleanupEvent(ctx); err != nil {
-		t.Fatalf("cleanup should not fail when a workspace is missing, got: %v", err)
+		t.Fatalf("cleanup should not fail, got: %v", err)
 	}
 }