Skip to content

Commit 8a5873e

Browse files
committed
fix(github): recover fork PR re-runs when GitHub omits PR metadata
When someone clicks Re-run on a Pipelines as Code check for a pull request coming from a fork, GitHub can leave out the usual pull request details in the webhook payload. That left PAC with no obvious way to tell which pull request the check belonged to, so the re-run stopped with an error instead of starting again. This change teaches PAC to keep looking in a more practical way. If the first GitHub API says it cannot find the pull request for the commit, PAC now lists open pull requests in the repository and matches the one whose head commit SHA is the same. In simple terms: when GitHub does not hand us the answer directly, we now look through the open pull requests and find the one built from that exact commit. The change also checks pull request data attached directly to the check_run event before falling back to SHA-based lookup, and the tests now cover both the new fork pull request path and the empty-result error cases. Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>
1 parent 69c1197 commit 8a5873e

File tree

2 files changed

+249
-1
lines changed

2 files changed

+249
-1
lines changed

pkg/provider/github/parse_payload.go

Lines changed: 49 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -314,13 +314,55 @@ func selectSingleOpenPullRequest(prs []*github.PullRequest) (*github.PullRequest
314314
}
315315
}
316316

317+
func (v *Provider) findOpenPullRequestBySHA(ctx context.Context, org, repo, sha string) (*github.PullRequest, error) {
318+
const maxPages = 10
319+
opts := &github.PullRequestListOptions{
320+
State: "open",
321+
Sort: "updated",
322+
ListOptions: github.ListOptions{PerPage: 100},
323+
}
324+
var matches []*github.PullRequest
325+
326+
for page := 0; page < maxPages; page++ {
327+
prs, resp, err := wrapAPI(v, "list_pull_requests", func() ([]*github.PullRequest, *github.Response, error) {
328+
return v.Client().PullRequests.List(ctx, org, repo, opts)
329+
})
330+
if err != nil {
331+
return nil, fmt.Errorf("failed to list open pull requests in %s/%s: %w", org, repo, err)
332+
}
333+
334+
for _, pr := range prs {
335+
if pr.GetHead().GetSHA() == sha {
336+
matches = append(matches, pr)
337+
}
338+
}
339+
340+
if resp.NextPage == 0 {
341+
break
342+
}
343+
opts.Page = resp.NextPage
344+
}
345+
346+
return selectSingleOpenPullRequest(matches)
347+
}
348+
317349
func (v *Provider) resolveReRequestPullRequest(ctx context.Context, runevent *info.Event) (*github.PullRequest, error) {
318350
prs, err := v.getPullRequestsWithCommit(ctx, runevent.SHA, runevent.Organization, runevent.Repository, false)
319351
if err != nil {
320352
return nil, err
321353
}
322354

323-
return selectSingleOpenPullRequest(prs)
355+
pr, err := selectSingleOpenPullRequest(prs)
356+
if err != nil {
357+
return nil, err
358+
}
359+
if pr != nil {
360+
return pr, nil
361+
}
362+
363+
// ListPullRequestsWithCommit may return no matches for fork PR commits.
364+
v.Logger.Infof("No PR found via commits API for SHA %s, falling back to open PR listing", runevent.SHA)
365+
return v.findOpenPullRequestBySHA(ctx, runevent.Organization, runevent.Repository, runevent.SHA)
324366
}
325367

326368
func (v *Provider) processEvent(ctx context.Context, event *info.Event, eventInt any) (*info.Event, error) {
@@ -503,6 +545,12 @@ func (v *Provider) handleReRequestEvent(ctx context.Context, event *github.Check
503545
runevent.HeadURL = event.GetCheckRun().GetCheckSuite().GetRepository().GetHTMLURL()
504546
// If we don't have a pull_request in this it probably mean a push
505547
if len(event.GetCheckRun().GetCheckSuite().PullRequests) == 0 {
548+
if len(event.GetCheckRun().PullRequests) > 0 {
549+
runevent.PullRequestNumber = event.GetCheckRun().PullRequests[0].GetNumber()
550+
runevent.TriggerTarget = triggertype.PullRequest
551+
v.Logger.Infof("Recheck of PR %s/%s#%d has been requested (from check_run)", runevent.Organization, runevent.Repository, runevent.PullRequestNumber)
552+
return v.getPullRequest(ctx, runevent)
553+
}
506554
// If head_branch is null, try to find a PR by SHA before assuming push
507555
if runevent.HeadBranch == "" && runevent.SHA != "" {
508556
pr, err := v.resolveReRequestPullRequest(ctx, runevent)

pkg/provider/github/parse_payload_test.go

Lines changed: 200 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -336,6 +336,76 @@ func TestGetPullRequestsWithCommit(t *testing.T) {
336336
}
337337
}
338338

339+
func TestFindOpenPullRequestBySHA(t *testing.T) {
340+
tests := []struct {
341+
name string
342+
sha string
343+
setup func(t *testing.T, mux *http.ServeMux)
344+
wantPRNumber int
345+
wantErr string
346+
}{
347+
{
348+
name: "single matching open PR",
349+
sha: "forkPRsha",
350+
setup: func(t *testing.T, mux *http.ServeMux) {
351+
t.Helper()
352+
mux.HandleFunc("/repos/owner/reponame/pulls", func(rw http.ResponseWriter, _ *http.Request) {
353+
fmt.Fprint(rw, `[
354+
{"number": 101, "state": "open", "head": {"sha": "otherSHA"}},
355+
{"number": 202, "state": "open", "head": {"sha": "forkPRsha"}}
356+
]`)
357+
})
358+
},
359+
wantPRNumber: 202,
360+
},
361+
{
362+
name: "multiple matching open PRs across pages returns ambiguity error",
363+
sha: "ambiguousSHA",
364+
setup: func(t *testing.T, mux *http.ServeMux) {
365+
t.Helper()
366+
mux.HandleFunc("/repos/owner/reponame/pulls", func(rw http.ResponseWriter, r *http.Request) {
367+
switch r.URL.Query().Get("page") {
368+
case "", "1":
369+
rw.Header().Set("Link", `<https://api.github.com/repos/owner/reponame/pulls?page=2>; rel="next"`)
370+
fmt.Fprint(rw, `[{"number": 101, "state": "open", "head": {"sha": "ambiguousSHA"}}]`)
371+
case "2":
372+
fmt.Fprint(rw, `[{"number": 202, "state": "open", "head": {"sha": "ambiguousSHA"}}]`)
373+
default:
374+
t.Fatalf("unexpected page %q", r.URL.Query().Get("page"))
375+
}
376+
})
377+
},
378+
wantErr: "found 2 open pull requests associated with the commit",
379+
},
380+
}
381+
382+
for _, tt := range tests {
383+
t.Run(tt.name, func(t *testing.T) {
384+
ctx, _ := rtesting.SetupFakeContext(t)
385+
fakeclient, mux, _, teardown := ghtesthelper.SetupGH()
386+
defer teardown()
387+
388+
tt.setup(t, mux)
389+
390+
logger, _ := logger.GetLogger()
391+
provider := &Provider{
392+
ghClient: fakeclient,
393+
Logger: logger,
394+
}
395+
396+
pr, err := provider.findOpenPullRequestBySHA(ctx, "owner", "reponame", tt.sha)
397+
if tt.wantErr != "" {
398+
assert.ErrorContains(t, err, tt.wantErr)
399+
return
400+
}
401+
402+
assert.NilError(t, err)
403+
assert.Assert(t, pr != nil)
404+
assert.Equal(t, tt.wantPRNumber, pr.GetNumber())
405+
})
406+
}
407+
}
408+
339409
func TestIsCommitPartOfPullRequest(t *testing.T) {
340410
tests := []struct {
341411
name string
@@ -644,6 +714,133 @@ func TestParsePayLoad(t *testing.T) {
644714
shaRet: "samplePRsha",
645715
wantedPullRequestNumber: 54321,
646716
},
717+
{
718+
name: "good/rerequest check_run resolves PR from check_run pull requests",
719+
eventType: "check_run",
720+
githubClient: true,
721+
triggerTarget: string(triggertype.PullRequest),
722+
payloadEventStruct: github.CheckRunEvent{
723+
Action: github.Ptr("rerequested"),
724+
Repo: sampleRepo,
725+
CheckRun: &github.CheckRun{
726+
PullRequests: []*github.PullRequest{&samplePR},
727+
CheckSuite: &github.CheckSuite{
728+
HeadSHA: github.Ptr("samplePRsha"),
729+
},
730+
},
731+
},
732+
muxReplies: map[string]any{
733+
"/repos/owner/reponame/pulls/54321": samplePR,
734+
},
735+
shaRet: "samplePRsha",
736+
wantedPullRequestNumber: 54321,
737+
},
738+
{
739+
name: "good/rerequest check_run null head_branch resolves fork PR from open PR list",
740+
eventType: "check_run",
741+
githubClient: true,
742+
triggerTarget: string(triggertype.PullRequest),
743+
payloadEventStruct: github.CheckRunEvent{
744+
Action: github.Ptr("rerequested"),
745+
Repo: sampleRepo,
746+
CheckRun: &github.CheckRun{
747+
CheckSuite: &github.CheckSuite{
748+
HeadSHA: github.Ptr("forkPRsha"),
749+
},
750+
},
751+
},
752+
muxReplies: map[string]any{
753+
"/repos/owner/reponame/commits/forkPRsha/pulls": []*github.PullRequest{},
754+
"/repos/owner/reponame/pulls": []*github.PullRequest{
755+
{
756+
Number: github.Ptr(987),
757+
State: github.Ptr("open"),
758+
HTMLURL: github.Ptr("https://github.com/owner/reponame/pull/987"),
759+
Head: &github.PullRequestBranch{
760+
SHA: github.Ptr("forkPRsha"),
761+
Ref: github.Ptr("fork-feature"),
762+
Repo: &github.Repository{
763+
Owner: &github.User{
764+
Login: github.Ptr("fork-owner"),
765+
},
766+
Name: github.Ptr("reponame"),
767+
HTMLURL: github.Ptr("https://github.com/fork-owner/reponame"),
768+
},
769+
},
770+
Base: &github.PullRequestBranch{
771+
Ref: github.Ptr("main"),
772+
SHA: github.Ptr("basesha"),
773+
Repo: sampleRepo,
774+
},
775+
User: &github.User{
776+
Login: github.Ptr("fork-contributor"),
777+
},
778+
Title: github.Ptr("fork PR"),
779+
},
780+
},
781+
"/repos/owner/reponame/pulls/987": github.PullRequest{
782+
Number: github.Ptr(987),
783+
State: github.Ptr("open"),
784+
HTMLURL: github.Ptr("https://github.com/owner/reponame/pull/987"),
785+
Head: &github.PullRequestBranch{
786+
SHA: github.Ptr("forkPRsha"),
787+
Ref: github.Ptr("fork-feature"),
788+
Repo: &github.Repository{
789+
Owner: &github.User{
790+
Login: github.Ptr("fork-owner"),
791+
},
792+
Name: github.Ptr("reponame"),
793+
HTMLURL: github.Ptr("https://github.com/fork-owner/reponame"),
794+
},
795+
},
796+
Base: &github.PullRequestBranch{
797+
Ref: github.Ptr("main"),
798+
SHA: github.Ptr("basesha"),
799+
Repo: sampleRepo,
800+
},
801+
User: &github.User{
802+
Login: github.Ptr("fork-contributor"),
803+
},
804+
Title: github.Ptr("fork PR"),
805+
},
806+
},
807+
shaRet: "forkPRsha",
808+
wantedPullRequestNumber: 987,
809+
},
810+
{
811+
name: "bad/rerequest check_run null head_branch ambiguous fallback PRs found",
812+
eventType: "check_run",
813+
githubClient: true,
814+
wantErrString: "found 2 open pull requests associated with the commit",
815+
payloadEventStruct: github.CheckRunEvent{
816+
Action: github.Ptr("rerequested"),
817+
Repo: sampleRepo,
818+
CheckRun: &github.CheckRun{
819+
CheckSuite: &github.CheckSuite{
820+
HeadSHA: github.Ptr("ambiguousFallbackSHA"),
821+
},
822+
},
823+
},
824+
muxReplies: map[string]any{
825+
"/repos/owner/reponame/commits/ambiguousFallbackSHA/pulls": []*github.PullRequest{},
826+
"/repos/owner/reponame/pulls": []*github.PullRequest{
827+
{
828+
Number: github.Ptr(301),
829+
State: github.Ptr("open"),
830+
Head: &github.PullRequestBranch{
831+
SHA: github.Ptr("ambiguousFallbackSHA"),
832+
},
833+
},
834+
{
835+
Number: github.Ptr(302),
836+
State: github.Ptr("open"),
837+
Head: &github.PullRequestBranch{
838+
SHA: github.Ptr("ambiguousFallbackSHA"),
839+
},
840+
},
841+
},
842+
},
843+
},
647844
{
648845
name: "good/rerequest check_suite null head_branch resolves PR from SHA",
649846
eventType: "check_suite",
@@ -678,6 +875,7 @@ func TestParsePayLoad(t *testing.T) {
678875
},
679876
muxReplies: map[string]any{
680877
"/repos/owner/reponame/commits/orphanSHA/pulls": []*github.PullRequest{},
878+
"/repos/owner/reponame/pulls": []*github.PullRequest{},
681879
},
682880
},
683881
{
@@ -694,6 +892,7 @@ func TestParsePayLoad(t *testing.T) {
694892
},
695893
muxReplies: map[string]any{
696894
"/repos/owner/reponame/commits/orphanSHA/pulls": []*github.PullRequest{},
895+
"/repos/owner/reponame/pulls": []*github.PullRequest{},
697896
},
698897
},
699898
{
@@ -717,6 +916,7 @@ func TestParsePayLoad(t *testing.T) {
717916
State: github.Ptr("closed"),
718917
},
719918
},
919+
"/repos/owner/reponame/pulls": []*github.PullRequest{},
720920
},
721921
},
722922
{

0 commit comments

Comments
 (0)