Skip to content

Commit c2383d9

Browse files
cbullingercbullinger
committed
fix: add warning for conflicting auto_merge settings in batched workflows
When multiple workflows target the same repo with different auto_merge settings, log a warning and use AND logic (any false wins) for safety.
1 parent ccb54c8 commit c2383d9

5 files changed

Lines changed: 202 additions & 21 deletions

File tree

‎services/github_write_to_source.go‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,14 +74,42 @@ func UpdateDeprecationFile(ctx context.Context, config *configs.Config, filesToD
7474
}
7575
}
7676

77+
// Build a set of existing entries for duplicate detection
78+
// Key format: "filename|repo|branch" to identify unique entries
79+
existingEntries := make(map[string]bool)
80+
for _, entry := range deprecationFile {
81+
key := entry.FileName + "|" + entry.Repo + "|" + entry.Branch
82+
existingEntries[key] = true
83+
}
84+
85+
entriesAdded := 0
7786
for key, value := range filesToDeprecate {
87+
// Check for duplicates before appending (prevents issues with webhook redelivery)
88+
entryKey := key + "|" + value.TargetRepo + "|" + value.TargetBranch
89+
if existingEntries[entryKey] {
90+
LogInfo("Skipping duplicate deprecation entry",
91+
"filename", key,
92+
"repo", value.TargetRepo,
93+
"branch", value.TargetBranch,
94+
)
95+
continue
96+
}
97+
7898
newDeprecatedFileEntry := types.DeprecatedFileEntry{
7999
FileName: key,
80100
Repo: value.TargetRepo,
81101
Branch: value.TargetBranch,
82102
DeletedOn: time.Now().Format(time.RFC3339),
83103
}
84104
deprecationFile = append(deprecationFile, newDeprecatedFileEntry)
105+
existingEntries[entryKey] = true // Mark as added to prevent duplicates within current batch
106+
entriesAdded++
107+
}
108+
109+
// Early return if all entries were duplicates
110+
if entriesAdded == 0 {
111+
LogInfo("All deprecation entries already exist; skipping update")
112+
return
85113
}
86114

87115
updatedJSON, err := json.MarshalIndent(deprecationFile, "", " ")

‎services/webhook_handler_new.go‎

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -382,7 +382,7 @@ func handleMergedPRWithContainer(ctx context.Context, prNumber int, sourceCommit
382382

383383
// 5. Process workflows independently, collecting per-workflow errors (#6)
384384
workflowErrors := processFilesWithWorkflows(ctx, prNumber, sourceCommitSHA, changedFiles, yamlConfig, config, container)
385-
uploadAndDeprecateFiles(ctx, config, container, repoOwner, repoName, baseBranch)
385+
uploadAndDeprecateFiles(ctx, config, container, repoOwner, repoName, baseBranch, prNumber)
386386

387387
// 6. Collect unique target repos for notification
388388
targetRepos := collectTargetRepos(yamlConfig)
@@ -459,7 +459,7 @@ func fetchChangedFiles(ctx context.Context, config *configs.Config, container *S
459459

460460
// uploadAndDeprecateFiles drains the file-state queues, uploading files to target
461461
// repos and updating the deprecation file in the source repo.
462-
func uploadAndDeprecateFiles(ctx context.Context, config *configs.Config, container *ServiceContainer, sourceRepoOwner, sourceRepoName, sourceBranch string) {
462+
func uploadAndDeprecateFiles(ctx context.Context, config *configs.Config, container *ServiceContainer, sourceRepoOwner, sourceRepoName, sourceBranch string, prNumber int) {
463463
// Upload queued files
464464
filesToUpload := container.FileStateService.GetFilesToUpload()
465465
AddFilesToTargetRepos(ctx, config, filesToUpload, container.PRTemplateFetcher, container.MetricsCollector)
@@ -468,16 +468,31 @@ func uploadAndDeprecateFiles(ctx context.Context, config *configs.Config, contai
468468
// Build deprecation map and update file in the source repo
469469
deprecationMap := container.FileStateService.GetFilesToDeprecate()
470470
filesToDeprecate := make(map[string]types.Configs)
471+
var deprecatedFiles []string
471472
for _, entries := range deprecationMap {
472473
for _, entry := range entries {
473474
filesToDeprecate[entry.FileName] = types.Configs{
474475
TargetRepo: entry.Repo,
475476
TargetBranch: entry.Branch,
476477
}
478+
deprecatedFiles = append(deprecatedFiles, entry.FileName)
477479
}
478480
}
479481
UpdateDeprecationFile(ctx, config, filesToDeprecate, sourceRepoOwner, sourceRepoName, sourceBranch)
480482
container.FileStateService.ClearFilesToDeprecate()
483+
484+
// Send Slack notification if files were deprecated
485+
if len(deprecatedFiles) > 0 {
486+
sourceRepo := fmt.Sprintf("%s/%s", sourceRepoOwner, sourceRepoName)
487+
if notifyErr := container.SlackNotifier.NotifyDeprecation(ctx, &DeprecationEvent{
488+
PRNumber: prNumber,
489+
SourceRepo: sourceRepo,
490+
FileCount: len(deprecatedFiles),
491+
Files: deprecatedFiles,
492+
}); notifyErr != nil {
493+
LogWarningCtx(ctx, "failed to send Slack deprecation notification", map[string]interface{}{"error": notifyErr.Error()})
494+
}
495+
}
481496
}
482497

483498
// reportCompletion calculates processing metrics and sends a Slack notification.

‎services/workflow_processor.go‎

Lines changed: 33 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -142,7 +142,7 @@ func (wp *workflowProcessor) ProcessWorkflow(
142142
for i := range matches {
143143
mr := &matches[i]
144144
if mr.isDelete {
145-
wp.addToDeprecationMap(mr.workflow, mr.targetPath)
145+
wp.addToDeprecationMap(mr.workflow, mr.targetPath, mr.file.Path, mr.prNumber)
146146
filesMatched++
147147
continue
148148
}
@@ -379,17 +379,24 @@ func (wp *workflowProcessor) isExcluded(path string, excludePatterns []string) b
379379
return false
380380
}
381381

382-
// addToDeprecationMap adds a file to the deprecation map
383-
func (wp *workflowProcessor) addToDeprecationMap(workflow types.Workflow, targetPath string) {
382+
// addToDeprecationMap adds a file to the deprecation map if deprecation tracking is enabled
383+
func (wp *workflowProcessor) addToDeprecationMap(workflow types.Workflow, targetPath string, sourcePath string, prNumber int) {
384+
// Only track deprecations if explicitly enabled
385+
if workflow.DeprecationCheck == nil || !workflow.DeprecationCheck.Enabled {
386+
return
387+
}
388+
384389
deprecationFile := "deprecated_examples.json"
385-
if workflow.DeprecationCheck != nil && workflow.DeprecationCheck.File != "" {
390+
if workflow.DeprecationCheck.File != "" {
386391
deprecationFile = workflow.DeprecationCheck.File
387392
}
388393

389394
entry := types.DeprecatedFileEntry{
390-
FileName: targetPath,
391-
Repo: workflow.Destination.Repo,
392-
Branch: workflow.Destination.Branch,
395+
FileName: targetPath,
396+
Repo: workflow.Destination.Repo,
397+
Branch: workflow.Destination.Branch,
398+
SourcePath: sourcePath,
399+
PRNumber: prNumber,
393400
}
394401

395402
wp.fileStateService.AddFileToDeprecate(deprecationFile, entry)
@@ -424,6 +431,25 @@ func (wp *workflowProcessor) queueUpload(
424431
UsePRTemplate: getUsePRTemplate(workflow),
425432
AutoMergePR: getAutoMerge(workflow),
426433
}
434+
} else {
435+
// When batching multiple workflows, use AND logic for auto-merge (conservative):
436+
// auto-merge is only enabled if ALL workflows in the batch want it.
437+
// Log a warning when workflows have conflicting auto-merge settings.
438+
workflowAutoMerge := getAutoMerge(workflow)
439+
if workflowAutoMerge != content.AutoMergePR {
440+
LogWarning("Workflows in batch have conflicting auto_merge settings; using AND logic (auto-merge disabled)",
441+
"workflow", workflow.Name,
442+
"target", key.RepoName,
443+
"workflow_auto_merge", workflowAutoMerge,
444+
"batch_auto_merge", content.AutoMergePR,
445+
)
446+
// AND logic: if either is false, result is false
447+
content.AutoMergePR = false
448+
}
449+
// For PR template, use OR logic - if any workflow wants it, use it
450+
if getUsePRTemplate(workflow) && !content.UsePRTemplate {
451+
content.UsePRTemplate = true
452+
}
427453
}
428454

429455
// Add file to content

‎services/workflow_processor_test.go‎

Lines changed: 118 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,13 @@ func createTestWorkflow(name string, transformations []types.Transformation) typ
5959
}
6060
}
6161

62+
// createTestWorkflowWithDeprecation creates a test workflow with deprecation tracking enabled
63+
func createTestWorkflowWithDeprecation(name string, transformations []types.Transformation) types.Workflow {
64+
wf := createTestWorkflow(name, transformations)
65+
wf.DeprecationCheck = &types.DeprecationConfig{Enabled: true}
66+
return wf
67+
}
68+
6269
func createMoveTransformation(from, to string) types.Transformation {
6370
return types.Transformation{
6471
Move: &types.MoveTransform{From: from, To: to},
@@ -159,7 +166,7 @@ func TestWorkflowProcessor_MoveTransformation(t *testing.T) {
159166
test.TestConfig(),
160167
)
161168

162-
workflow := createTestWorkflow("test-workflow", []types.Transformation{
169+
workflow := createTestWorkflowWithDeprecation("test-workflow", []types.Transformation{
163170
createMoveTransformation(tt.from, tt.to),
164171
})
165172

@@ -229,7 +236,7 @@ func TestWorkflowProcessor_CopyTransformation(t *testing.T) {
229236
test.TestConfig(),
230237
)
231238

232-
workflow := createTestWorkflow("test-workflow", []types.Transformation{
239+
workflow := createTestWorkflowWithDeprecation("test-workflow", []types.Transformation{
233240
createCopyTransformation(tt.from, tt.to),
234241
})
235242

@@ -304,7 +311,7 @@ func TestWorkflowProcessor_GlobTransformation(t *testing.T) {
304311
test.TestConfig(),
305312
)
306313

307-
workflow := createTestWorkflow("test-workflow", []types.Transformation{
314+
workflow := createTestWorkflowWithDeprecation("test-workflow", []types.Transformation{
308315
createGlobTransformation(tt.pattern, tt.transform),
309316
})
310317

@@ -372,7 +379,7 @@ func TestWorkflowProcessor_RegexTransformation(t *testing.T) {
372379
test.TestConfig(),
373380
)
374381

375-
workflow := createTestWorkflow("test-workflow", []types.Transformation{
382+
workflow := createTestWorkflowWithDeprecation("test-workflow", []types.Transformation{
376383
createRegexTransformation(tt.pattern, tt.transform),
377384
})
378385

@@ -493,7 +500,8 @@ func TestWorkflowProcessor_ExcludePatterns(t *testing.T) {
493500
createMoveTransformation("src", "dest"),
494501
createMoveTransformation("vendor", "vendor"),
495502
},
496-
Exclude: tt.exclude,
503+
Exclude: tt.exclude,
504+
DeprecationCheck: &types.DeprecationConfig{Enabled: true},
497505
}
498506

499507
changedFiles := []types.ChangedFile{
@@ -546,6 +554,7 @@ func TestWorkflowProcessor_MultipleTransformations(t *testing.T) {
546554
createMoveTransformation("docs", "documentation"),
547555
createCopyTransformation("README.md", "docs/README.md"),
548556
},
557+
DeprecationCheck: &types.DeprecationConfig{Enabled: true},
549558
}
550559

551560
changedFiles := []types.ChangedFile{
@@ -659,7 +668,8 @@ func TestWorkflowProcessor_InvalidExcludePattern(t *testing.T) {
659668
createMoveTransformation("src", "dest"),
660669
},
661670
// Invalid glob pattern - should be handled gracefully
662-
Exclude: []string{"[invalid"},
671+
Exclude: []string{"[invalid"},
672+
DeprecationCheck: &types.DeprecationConfig{Enabled: true},
663673
}
664674

665675
changedFiles := []types.ChangedFile{
@@ -704,7 +714,8 @@ func TestWorkflowProcessor_CustomDeprecationFile(t *testing.T) {
704714
createMoveTransformation("src", "dest"),
705715
},
706716
DeprecationCheck: &types.DeprecationConfig{
707-
File: "custom_deprecation.json",
717+
Enabled: true,
718+
File: "custom_deprecation.json",
708719
},
709720
}
710721

@@ -720,6 +731,8 @@ func TestWorkflowProcessor_CustomDeprecationFile(t *testing.T) {
720731
assert.True(t, exists, "expected custom deprecation file to be used")
721732
require.Len(t, entries, 1, "expected one entry in custom deprecation file")
722733
assert.Equal(t, "dest/main.go", entries[0].FileName)
734+
assert.Equal(t, "src/main.go", entries[0].SourcePath, "expected source path to be recorded")
735+
assert.Equal(t, 1, entries[0].PRNumber, "expected PR number to be recorded")
723736
}
724737

725738
// ============================================================================
@@ -769,6 +782,8 @@ func TestWorkflowProcessor_FileStatusHandling(t *testing.T) {
769782
workflow := createTestWorkflow("test-workflow", []types.Transformation{
770783
createMoveTransformation("src", "dest"),
771784
})
785+
// Enable deprecation tracking for this test
786+
workflow.DeprecationCheck = &types.DeprecationConfig{Enabled: true}
772787

773788
changedFiles := []types.ChangedFile{
774789
{Path: "src/main.go", Status: tt.status},
@@ -790,6 +805,45 @@ func TestWorkflowProcessor_FileStatusHandling(t *testing.T) {
790805
}
791806
}
792807

808+
func TestWorkflowProcessor_DeprecationDisabledByDefault(t *testing.T) {
809+
// Test that deprecation tracking does NOT happen when DeprecationCheck.Enabled is false/unset
810+
fileStateService := services.NewFileStateService()
811+
processor := services.NewWorkflowProcessor(
812+
services.NewPatternMatcher(),
813+
services.NewPathTransformer(),
814+
fileStateService,
815+
nil,
816+
&mockMessageTemplater{},
817+
test.TestConfig(),
818+
)
819+
820+
// Create workflow WITHOUT deprecation enabled
821+
workflow := createTestWorkflow("test-workflow", []types.Transformation{
822+
createMoveTransformation("src", "dest"),
823+
})
824+
// DeprecationCheck is nil - should not track deprecations
825+
826+
changedFiles := []types.ChangedFile{
827+
{Path: "src/main.go", Status: "removed"},
828+
}
829+
830+
err := processor.ProcessWorkflow(context.Background(), workflow, changedFiles, 1, "abc123")
831+
require.NoError(t, err)
832+
833+
deprecated := fileStateService.GetFilesToDeprecate()
834+
assert.Empty(t, deprecated, "expected NO deprecation tracking when DeprecationCheck is nil")
835+
836+
// Also test when DeprecationCheck exists but Enabled is false
837+
workflow.DeprecationCheck = &types.DeprecationConfig{Enabled: false, File: "test.json"}
838+
fileStateService.ClearFilesToDeprecate()
839+
840+
err = processor.ProcessWorkflow(context.Background(), workflow, changedFiles, 1, "abc123")
841+
require.NoError(t, err)
842+
843+
deprecated = fileStateService.GetFilesToDeprecate()
844+
assert.Empty(t, deprecated, "expected NO deprecation tracking when DeprecationCheck.Enabled is false")
845+
}
846+
793847
// ============================================================================
794848
// Tests for Path Transformation Edge Cases
795849
// ============================================================================
@@ -845,7 +899,7 @@ func TestWorkflowProcessor_PathTransformationEdgeCases(t *testing.T) {
845899
test.TestConfig(),
846900
)
847901

848-
workflow := createTestWorkflow("test-workflow", []types.Transformation{tt.transform})
902+
workflow := createTestWorkflowWithDeprecation("test-workflow", []types.Transformation{tt.transform})
849903

850904
changedFiles := []types.ChangedFile{
851905
{Path: tt.sourcePath, Status: "removed"},
@@ -863,3 +917,59 @@ func TestWorkflowProcessor_PathTransformationEdgeCases(t *testing.T) {
863917
})
864918
}
865919
}
920+
921+
// ============================================================================
922+
// Tests for Auto-Merge Batching Behavior
923+
// ============================================================================
924+
925+
// TestFileStateService_AutoMergeBatching_AllTrue tests that when all entries
926+
// in a batch have auto_merge: true, the batch retains auto_merge: true.
927+
func TestFileStateService_AutoMergeBatching_AllTrue(t *testing.T) {
928+
fileStateService := services.NewFileStateService()
929+
930+
key := types.UploadKey{
931+
RepoName: "test-org/dest-repo",
932+
BranchPath: "main",
933+
CommitStrategy: "pull_request",
934+
}
935+
936+
// First upload with auto_merge: true
937+
fileStateService.AddFileToUpload(key, types.UploadFileContent{
938+
TargetBranch: "main",
939+
CommitStrategy: "pull_request",
940+
AutoMergePR: true,
941+
})
942+
943+
uploads := fileStateService.GetFilesToUpload()
944+
require.Len(t, uploads, 1, "expected one upload entry")
945+
assert.True(t, uploads[key].AutoMergePR, "expected auto_merge to be true")
946+
}
947+
948+
// TestFileStateService_AutoMergeBatching_AllFalse tests that when all entries
949+
// in a batch have auto_merge: false, the batch has auto_merge: false.
950+
func TestFileStateService_AutoMergeBatching_AllFalse(t *testing.T) {
951+
fileStateService := services.NewFileStateService()
952+
953+
key := types.UploadKey{
954+
RepoName: "test-org/dest-repo",
955+
BranchPath: "main",
956+
CommitStrategy: "pull_request",
957+
}
958+
959+
// Upload with auto_merge: false
960+
fileStateService.AddFileToUpload(key, types.UploadFileContent{
961+
TargetBranch: "main",
962+
CommitStrategy: "pull_request",
963+
AutoMergePR: false,
964+
})
965+
966+
uploads := fileStateService.GetFilesToUpload()
967+
require.Len(t, uploads, 1, "expected one upload entry")
968+
assert.False(t, uploads[key].AutoMergePR, "expected auto_merge to be false")
969+
}
970+
971+
// Note: The actual conflict detection and AND logic happens in workflow_processor.go's
972+
// queueUpload function when adding files to an existing batch. Testing this requires
973+
// either mocking GitHub API calls or testing at the integration level.
974+
// The conflict warning is logged when workflows with different auto_merge settings
975+
// target the same repo, and AND logic is applied (any false results in false).

‎types/types.go‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -92,10 +92,12 @@ type Configs struct {
9292
}
9393
type DeprecationFile []DeprecatedFileEntry
9494
type DeprecatedFileEntry struct {
95-
FileName string `json:"filename"`
96-
Repo string `json:"repo"`
97-
Branch string `json:"branch"`
98-
DeletedOn string `json:"deleted_on"`
95+
FileName string `json:"filename"`
96+
Repo string `json:"repo"`
97+
Branch string `json:"branch"`
98+
DeletedOn string `json:"deleted_on"`
99+
SourcePath string `json:"source_path,omitempty"` // Original source file path
100+
PRNumber int `json:"pr_number,omitempty"` // PR that caused the deletion
99101
}
100102

101103
// **** UPLOAD TYPES **** //

0 commit comments

Comments
 (0)