Skip to content

Commit 9d87645

Browse files
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 9d87645

5 files changed

Lines changed: 366 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

0 commit comments

Comments
 (0)