Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 617b4a7

Browse filesBrowse files
cbullingercbullinger
committed
fix(copier): only process PRs merged to branches that match the source.branch in workflow
1 parent bb047bf commit 617b4a7
Copy full SHA for 617b4a7

File tree

Expand file treeCollapse file tree

2 files changed

+200
-12
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

2 files changed

+200
-12
lines changed
Open diff view settings
Collapse file

‎examples-copier/services/webhook_handler_new.go‎

Copy file name to clipboardExpand all lines: examples-copier/services/webhook_handler_new.go
+17-12Lines changed: 17 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -166,11 +166,15 @@ func HandleWebhookWithContainer(w http.ResponseWriter, r *http.Request, config *
166166
repoOwner := repo.GetOwner().GetLogin()
167167
repoName := repo.GetName()
168168

169+
// Extract the base branch (the branch the PR was merged into)
170+
baseBranch := prEvt.GetPullRequest().GetBase().GetRef()
171+
169172
LogInfoCtx(ctx, "processing merged PR", map[string]interface{}{
170-
"pr_number": prNumber,
171-
"sha": sourceCommitSHA,
172-
"repo": fmt.Sprintf("%s/%s", repoOwner, repoName),
173-
"elapsed_ms": time.Since(startTime).Milliseconds(),
173+
"pr_number": prNumber,
174+
"sha": sourceCommitSHA,
175+
"repo": fmt.Sprintf("%s/%s", repoOwner, repoName),
176+
"base_branch": baseBranch,
177+
"elapsed_ms": time.Since(startTime).Milliseconds(),
174178
})
175179

176180
// Respond immediately to avoid GitHub webhook timeout
@@ -197,11 +201,11 @@ func HandleWebhookWithContainer(w http.ResponseWriter, r *http.Request, config *
197201
// Process asynchronously in background with a new context
198202
// Don't use the request context as it will be cancelled when the request completes
199203
bgCtx := context.Background()
200-
go handleMergedPRWithContainer(bgCtx, prNumber, sourceCommitSHA, repoOwner, repoName, config, container)
204+
go handleMergedPRWithContainer(bgCtx, prNumber, sourceCommitSHA, repoOwner, repoName, baseBranch, config, container)
201205
}
202206

203207
// handleMergedPRWithContainer processes a merged PR using the new pattern matching system
204-
func handleMergedPRWithContainer(ctx context.Context, prNumber int, sourceCommitSHA string, repoOwner string, repoName string, config *configs.Config, container *ServiceContainer) {
208+
func handleMergedPRWithContainer(ctx context.Context, prNumber int, sourceCommitSHA string, repoOwner string, repoName string, baseBranch string, config *configs.Config, container *ServiceContainer) {
205209
startTime := time.Now()
206210

207211
// Configure GitHub permissions
@@ -227,18 +231,20 @@ func handleMergedPRWithContainer(ctx context.Context, prNumber int, sourceCommit
227231
return
228232
}
229233

230-
// Find workflows matching this source repo
234+
// Find workflows matching this source repo and branch
231235
webhookRepo := fmt.Sprintf("%s/%s", repoOwner, repoName)
232-
matchingWorkflows := []types.Workflow{}
236+
var matchingWorkflows []types.Workflow
233237
for _, workflow := range yamlConfig.Workflows {
234-
if workflow.Source.Repo == webhookRepo {
238+
// Match both repository and branch
239+
if workflow.Source.Repo == webhookRepo && workflow.Source.Branch == baseBranch {
235240
matchingWorkflows = append(matchingWorkflows, workflow)
236241
}
237242
}
238243

239244
if len(matchingWorkflows) == 0 {
240-
LogWarningCtx(ctx, "no workflows configured for source repository", map[string]interface{}{
245+
LogWarningCtx(ctx, "no workflows configured for source repository and branch", map[string]interface{}{
241246
"webhook_repo": webhookRepo,
247+
"base_branch": baseBranch,
242248
"workflow_count": len(yamlConfig.Workflows),
243249
})
244250
container.MetricsCollector.RecordWebhookFailed()
@@ -247,6 +253,7 @@ func handleMergedPRWithContainer(ctx context.Context, prNumber int, sourceCommit
247253

248254
LogInfoCtx(ctx, "found matching workflows", map[string]interface{}{
249255
"webhook_repo": webhookRepo,
256+
"base_branch": baseBranch,
250257
"matching_count": len(matchingWorkflows),
251258
})
252259

@@ -361,5 +368,3 @@ func processFilesWithWorkflows(ctx context.Context, prNumber int, sourceCommitSH
361368
"workflow_count": len(yamlConfig.Workflows),
362369
})
363370
}
364-
365-
Collapse file

‎examples-copier/services/webhook_handler_new_test.go‎

Copy file name to clipboardExpand all lines: examples-copier/services/webhook_handler_new_test.go
+183Lines changed: 183 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -301,6 +301,9 @@ func TestHandleWebhookWithContainer_MergedPR(t *testing.T) {
301301
Number: github.Int(123),
302302
Merged: github.Bool(true),
303303
MergeCommitSHA: github.String("abc123"),
304+
Base: &github.PullRequestBranch{
305+
Ref: github.String("main"),
306+
},
304307
},
305308
Repo: &github.Repository{
306309
Name: github.String("test-repo"),
@@ -334,6 +337,186 @@ func TestHandleWebhookWithContainer_MergedPR(t *testing.T) {
334337
// when trying to access GitHub APIs. This is expected and doesn't affect the test result.
335338
}
336339

340+
func TestHandleWebhookWithContainer_MergedPRToDevelopmentBranch(t *testing.T) {
341+
// This test verifies that PRs merged to non-main branches (like development)
342+
// are accepted by the webhook handler but won't match any workflows
343+
// (assuming workflows are configured for main branch only)
344+
345+
// Set up environment variables
346+
os.Setenv(configs.AppId, "123456")
347+
os.Setenv(configs.InstallationId, "789012")
348+
os.Setenv(configs.ConfigRepoOwner, "test-owner")
349+
os.Setenv(configs.ConfigRepoName, "test-repo")
350+
os.Setenv("SKIP_SECRET_MANAGER", "true")
351+
352+
// Generate a valid RSA private key for testing
353+
key, _ := rsa.GenerateKey(rand.Reader, 1024)
354+
der := x509.MarshalPKCS1PrivateKey(key)
355+
pemBytes := pem.EncodeToMemory(&pem.Block{Type: "RSA PRIVATE KEY", Bytes: der})
356+
os.Setenv("GITHUB_APP_PRIVATE_KEY", string(pemBytes))
357+
os.Setenv("GITHUB_APP_PRIVATE_KEY_B64", base64.StdEncoding.EncodeToString(pemBytes))
358+
359+
InstallationAccessToken = "test-token"
360+
361+
config := &configs.Config{
362+
ConfigRepoOwner: "test-owner",
363+
ConfigRepoName: "test-repo",
364+
ConfigFile: "nonexistent-config.yaml",
365+
AuditEnabled: false,
366+
}
367+
368+
container, err := NewServiceContainer(config)
369+
if err != nil {
370+
t.Fatalf("NewServiceContainer() error = %v", err)
371+
}
372+
373+
// Create a merged PR event to development branch
374+
prEvent := &github.PullRequestEvent{
375+
Action: github.String("closed"),
376+
PullRequest: &github.PullRequest{
377+
Number: github.Int(456),
378+
Merged: github.Bool(true),
379+
MergeCommitSHA: github.String("def456"),
380+
Base: &github.PullRequestBranch{
381+
Ref: github.String("development"),
382+
},
383+
},
384+
Repo: &github.Repository{
385+
Name: github.String("test-repo"),
386+
Owner: &github.User{
387+
Login: github.String("test-owner"),
388+
},
389+
},
390+
}
391+
payload, _ := json.Marshal(prEvent)
392+
393+
req := httptest.NewRequest("POST", "/webhook", bytes.NewReader(payload))
394+
req.Header.Set("X-GitHub-Event", "pull_request")
395+
396+
w := httptest.NewRecorder()
397+
398+
HandleWebhookWithContainer(w, req, config, container)
399+
400+
// Should still return 202 Accepted (webhook accepts the event)
401+
if w.Code != http.StatusAccepted {
402+
t.Errorf("Status code = %d, want %d", w.Code, http.StatusAccepted)
403+
}
404+
405+
// Check response body
406+
var response map[string]string
407+
json.Unmarshal(w.Body.Bytes(), &response)
408+
if response["status"] != "accepted" {
409+
t.Errorf("Response status = %v, want accepted", response["status"])
410+
}
411+
412+
// Note: The background goroutine will fail to find matching workflows
413+
// because the workflow config specifies main branch, not development.
414+
// This is the expected behavior - the webhook accepts the event but
415+
// no workflows will be processed.
416+
}
417+
418+
func TestHandleWebhookWithContainer_MergedPRWithDifferentBranches(t *testing.T) {
419+
// This test verifies that the base branch is correctly extracted
420+
// from different PR events
421+
422+
testCases := []struct {
423+
name string
424+
baseBranch string
425+
prNumber int
426+
}{
427+
{
428+
name: "main branch",
429+
baseBranch: "main",
430+
prNumber: 100,
431+
},
432+
{
433+
name: "development branch",
434+
baseBranch: "development",
435+
prNumber: 101,
436+
},
437+
{
438+
name: "feature branch",
439+
baseBranch: "feature/new-feature",
440+
prNumber: 102,
441+
},
442+
{
443+
name: "release branch",
444+
baseBranch: "release/v1.0",
445+
prNumber: 103,
446+
},
447+
}
448+
449+
// Set up environment variables
450+
os.Setenv(configs.AppId, "123456")
451+
os.Setenv(configs.InstallationId, "789012")
452+
os.Setenv(configs.ConfigRepoOwner, "test-owner")
453+
os.Setenv(configs.ConfigRepoName, "test-repo")
454+
os.Setenv("SKIP_SECRET_MANAGER", "true")
455+
456+
key, _ := rsa.GenerateKey(rand.Reader, 1024)
457+
der := x509.MarshalPKCS1PrivateKey(key)
458+
pemBytes := pem.EncodeToMemory(&pem.Block{Type: "RSA PRIVATE KEY", Bytes: der})
459+
os.Setenv("GITHUB_APP_PRIVATE_KEY", string(pemBytes))
460+
os.Setenv("GITHUB_APP_PRIVATE_KEY_B64", base64.StdEncoding.EncodeToString(pemBytes))
461+
462+
InstallationAccessToken = "test-token"
463+
464+
for _, tc := range testCases {
465+
t.Run(tc.name, func(t *testing.T) {
466+
config := &configs.Config{
467+
ConfigRepoOwner: "test-owner",
468+
ConfigRepoName: "test-repo",
469+
ConfigFile: "nonexistent-config.yaml",
470+
AuditEnabled: false,
471+
}
472+
473+
container, err := NewServiceContainer(config)
474+
if err != nil {
475+
t.Fatalf("NewServiceContainer() error = %v", err)
476+
}
477+
478+
// Create a merged PR event with specific base branch
479+
prEvent := &github.PullRequestEvent{
480+
Action: github.String("closed"),
481+
PullRequest: &github.PullRequest{
482+
Number: github.Int(tc.prNumber),
483+
Merged: github.Bool(true),
484+
MergeCommitSHA: github.String("abc123"),
485+
Base: &github.PullRequestBranch{
486+
Ref: github.String(tc.baseBranch),
487+
},
488+
},
489+
Repo: &github.Repository{
490+
Name: github.String("test-repo"),
491+
Owner: &github.User{
492+
Login: github.String("test-owner"),
493+
},
494+
},
495+
}
496+
payload, _ := json.Marshal(prEvent)
497+
498+
req := httptest.NewRequest("POST", "/webhook", bytes.NewReader(payload))
499+
req.Header.Set("X-GitHub-Event", "pull_request")
500+
501+
w := httptest.NewRecorder()
502+
503+
HandleWebhookWithContainer(w, req, config, container)
504+
505+
// Should return 202 Accepted for all merged PRs
506+
if w.Code != http.StatusAccepted {
507+
t.Errorf("Status code = %d, want %d", w.Code, http.StatusAccepted)
508+
}
509+
510+
// Check response body
511+
var response map[string]string
512+
json.Unmarshal(w.Body.Bytes(), &response)
513+
if response["status"] != "accepted" {
514+
t.Errorf("Response status = %v, want accepted", response["status"])
515+
}
516+
})
517+
}
518+
}
519+
337520
func TestRetrieveFileContentsWithConfigAndBranch(t *testing.T) {
338521
// This test would require mocking the GitHub client
339522
// For now, we document the expected behavior

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.