diff --git a/pkg/notes/notes.go b/pkg/notes/notes.go index 8d97f92960cd..8bdf939e4e8f 100644 --- a/pkg/notes/notes.go +++ b/pkg/notes/notes.go @@ -48,6 +48,7 @@ import ( var ( errNoPRIDFoundInCommitMessage = errors.New("no PR IDs found in the commit message") + errNoOriginPRIDFoundInPR = errors.New("no origin PR IDs found in the PR") errNoPRFoundForCommitSHA = errors.New("no PR found for this commit") apiSleepTime int64 = 60 ) @@ -1019,59 +1020,85 @@ func (g *Gatherer) prsForCommitFromSHA(sha string) (prs []*gogithub.PullRequest, } func (g *Gatherer) prsForCommitFromMessage(commitMessage string) (prs []*gogithub.PullRequest, err error) { - prsNum, err := prsNumForCommitFromMessage(commitMessage) + mainPrNum, err := prNumForCommitFromMessage(commitMessage) if err != nil { return nil, err } - var res *gogithub.PullRequest - var resp *gogithub.Response - for _, pr := range prsNum { - // Given the PR number that we've now converted to an integer, get the PR from - // the API - for { - res, resp, err = g.client.GetPullRequest(g.context, g.options.GithubOrg, g.options.GithubRepo, pr) - if err != nil { - if !canWaitAndRetry(resp, err) { - return nil, err - } - } else { - break - } + // Given the PR number that we've now converted to an integer, get the PR from + // the API + mainPr, err := g.getPr(mainPrNum) + if err != nil { + return prs, err + } + + prs = append(prs, mainPr) + + originPrNum, origPrErr := originPrNumFromPr(mainPr) + if origPrErr == nil { + originPr, err := g.getPr(originPrNum) + if err == nil { + prs = append(prs, originPr) } - prs = append(prs, res) } return prs, err } -func prsNumForCommitFromMessage(commitMessage string) (prs []int, err error) { +func (g *Gatherer) getPr(prNum int) (*gogithub.PullRequest, error) { + for { + res, resp, err := g.client.GetPullRequest(g.context, g.options.GithubOrg, g.options.GithubRepo, prNum) + if err != nil { + if !canWaitAndRetry(resp, err) { + return nil, err + } + } else { + return res, err + } + } +} + +var ( // Thankfully k8s-merge-robot commits the PR number consistently. If this ever // stops being true, this definitely won't work anymore. - regex := regexp.MustCompile(`Merge pull request #(?P\d+)`) - pr := prForRegex(regex, commitMessage) - if pr != 0 { - prs = append(prs, pr) + regexMergedPR = regexp.MustCompile(`Merge pull request #(?P\d+)`) + + // If the PR was squash merged, the regexp is different + regexSquashPR = regexp.MustCompile(`\(#(?P\d+)\)`) + + // The branch name created by "hack/cherry_pick_pull.sh" + regexHackBranch = regexp.MustCompile(`automated-cherry-pick-of-#(?P\d+)`) + + // The branch name created by k8s-infra-cherrypick-robot + regexProwBranch = regexp.MustCompile(`cherry-pick-(?P\d+)-to`) +) + +func prNumForCommitFromMessage(commitMessage string) (prs int, err error) { + if pr := prForRegex(regexMergedPR, commitMessage); pr != 0 { + return pr, nil } - regex = regexp.MustCompile(`automated-cherry-pick-of-#(?P\d+)`) - pr = prForRegex(regex, commitMessage) - if pr != 0 { - prs = append(prs, pr) + if pr := prForRegex(regexSquashPR, commitMessage); pr != 0 { + return pr, nil } + return 0, errNoPRIDFoundInCommitMessage +} - // If the PR was squash merged, the regexp is different - regex = regexp.MustCompile(`\(#(?P\d+)\)`) - pr = prForRegex(regex, commitMessage) - if pr != 0 { - prs = append(prs, pr) +func originPrNumFromPr(pr *gogithub.PullRequest) (origPRNum int, err error) { + if pr == nil || pr.Head == nil || pr.Head.Label == nil { + return 0, errNoOriginPRIDFoundInPR + } + origPRNum = prForRegex(regexHackBranch, *pr.Head.Label) + if origPRNum != 0 { + return origPRNum, nil } - if prs == nil { - return nil, errNoPRIDFoundInCommitMessage + origPRNum = prForRegex(regexProwBranch, *pr.Head.Label) + if origPRNum != 0 { + return origPRNum, nil } - return prs, nil + return 0, errNoOriginPRIDFoundInPR } func prForRegex(regex *regexp.Regexp, commitMessage string) int { diff --git a/pkg/notes/notes_gatherer_test.go b/pkg/notes/notes_gatherer_test.go index a75633cf4f81..8074eed39967 100644 --- a/pkg/notes/notes_gatherer_test.go +++ b/pkg/notes/notes_gatherer_test.go @@ -289,15 +289,10 @@ func TestGatherNotes(t *testing.T) { "when commit messages hold PR numbers, we use those and don't query to get a list of PRs for a SHA": { commitList: []*github.RepositoryCommit{ repoCommit("123", "there is the message Merge pull request #123 somewhere in the middle"), - repoCommit("124", "some automated-cherry-pick-of-#124 can be found too"), - repoCommit("125", "and lastly in parens (#125) yeah"), - repoCommit("126", `all three together - some Merge pull request #126 and - another automated-cherry-pick-of-#127 with - a thing (#128) in parens`), + repoCommit("124", "and lastly in parens (#124) yeah"), }, getPullRequestStubber: func(t *testing.T) getPullRequestStub { - seenPRs := newIntsRecorder(123, 124, 125, 126, 127, 128) + seenPRs := newIntsRecorder(123, 124) return func(_ context.Context, org, repo string, prID int) (*github.PullRequest, *github.Response, error) { checkOrgRepo(t, org, repo) @@ -307,6 +302,77 @@ func TestGatherNotes(t *testing.T) { return nil, nil, nil } }, + expectedGetPullRequestCallCount: 2, + }, + + "when the PR is a cherry pick": { + commitList: []*github.RepositoryCommit{ + repoCommit("125", "Merge a prow cherry-pick (#125)"), + repoCommit("126", "Merge hack cherry-pick (#126)"), + repoCommit("127", "Merge hack cherry-pick (#127)"), + }, + getPullRequestStubber: func(t *testing.T) getPullRequestStub { + seenPRs := newIntsRecorder(122, 123, 124, 125, 126, 127) + prsMap := map[int]*github.PullRequest{ + 122: { + Number: intPtr(122), + Body: strPtr("122\n```release-note\nfrom 122\n```\n"), + }, + 123: { + Number: intPtr(123), + Body: strPtr("123\n```release-note\nfrom 123\n```\n"), + }, + 124: { + Number: intPtr(124), + Body: strPtr("124\n```release-note\nfrom 124\n```\n"), + }, + 125: { + Number: intPtr(125), + Body: strPtr("No release note"), + Head: &github.PullRequestBranch{ + Label: strPtr("k8s-infra-cherrypick-robot:cherry-pick-122-to-release-0.x"), + }, + }, + 126: { + Number: intPtr(126), + Body: strPtr("Empty release note\n```release-note\n\n```\n"), + Head: &github.PullRequestBranch{ + Label: strPtr("fork:automated-cherry-pick-of-#123-upstream-release-0.x"), + }, + }, + 127: { + Number: intPtr(127), + Body: strPtr("127\n```release-note\nfrom 127\n```\n"), + Head: &github.PullRequestBranch{ + Label: strPtr("fork:automated-cherry-pick-of-#124-upstream-release-0.x"), + }, + }, + } + return func(_ context.Context, org, repo string, prID int) (*github.PullRequest, *github.Response, error) { + checkOrgRepo(t, org, repo) + if err := seenPRs.Mark(prID); err != nil { + t.Errorf("In GetPullRequest: %v", err) + } + return prsMap[prID], nil, nil + } + }, + resultsChecker: func(t *testing.T, results []*Result) { + expectMap := map[string]int{ + "125": 122, + "126": 123, + "127": 127, + } + + for _, result := range results { + expected, found := expectMap[*result.commit.SHA] + if !found { + t.Errorf("Unexpected SHA %s", *result.commit.SHA) + } + if expected != *result.pullRequest.Number { + t.Errorf("Expecting %d got %d for SHA %s", expected, *result.pullRequest.Number, *result.commit.SHA) + } + } + }, expectedGetPullRequestCallCount: 6, }, "when GetPullRequest(...) returns an error": { diff --git a/pkg/notes/notes_test.go b/pkg/notes/notes_test.go index 58784bb18838..e7efecf679fd 100644 --- a/pkg/notes/notes_test.go +++ b/pkg/notes/notes_test.go @@ -222,12 +222,12 @@ func TestGetPRNumberFromCommitMessage(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - prs, err := prsNumForCommitFromMessage(tc.commitMessage) + pr, err := prNumForCommitFromMessage(tc.commitMessage) if err != nil { t.Fatalf("Expected no error to occur but got %v", err) } - if prs[0] != tc.expectedPRNumber { - t.Errorf("Expected PR number to be %d but was %d", tc.expectedPRNumber, prs[0]) + if pr != tc.expectedPRNumber { + t.Errorf("Expected PR number to be %d but was %d", tc.expectedPRNumber, pr) } }) } diff --git a/pkg/notes/notes_v2.go b/pkg/notes/notes_v2.go index 3b51f2b7d8dd..02e99283336c 100644 --- a/pkg/notes/notes_v2.go +++ b/pkg/notes/notes_v2.go @@ -278,7 +278,7 @@ func (g *Gatherer) listLeftParentCommits(opts *options.Options) ([]*commitPrPair } // Find and collect PR number from commit message - prNums, err := prsNumForCommitFromMessage(commitPointer.Message) + prNum, err := prNumForCommitFromMessage(commitPointer.Message) if err == errNoPRIDFoundInCommitMessage { logrus.WithFields(logrus.Fields{ "sha": hashString, @@ -299,11 +299,11 @@ func (g *Gatherer) listLeftParentCommits(opts *options.Options) ([]*commitPrPair } logrus.WithFields(logrus.Fields{ "sha": hashString, - "prs": prNums, + "pr": prNum, }).Debug("found PR from commit") // Only taking the first one, assuming they are merged by Prow - pairs = append(pairs, &commitPrPair{Commit: commitPointer, PrNum: prNums[0]}) + pairs = append(pairs, &commitPrPair{Commit: commitPointer, PrNum: prNum}) // Advance pointer based on left parent hashPointer = commitPointer.ParentHashes[0]