From 9c5556edacf6229898df10af754bb82a74fc4246 Mon Sep 17 00:00:00 2001 From: Traian Schiau Date: Wed, 14 Feb 2024 17:57:45 +0200 Subject: [PATCH 1/2] [notes] Get the number of the origin PR for cherry-picks Get the number of the origin PR for cherry-picks from the PR's branch name instead of the commit message. This way we get the same behavior, using the origin release note if the current one is empty, for the cherry-picks created by the `hack/..` script and those created by prow, regardless of the merge strategy used in the project. --- pkg/notes/notes.go | 93 ++++++++++++++++++++------------ pkg/notes/notes_gatherer_test.go | 80 ++++++++++++++++++++++++--- pkg/notes/notes_test.go | 6 +-- pkg/notes/notes_v2.go | 6 +-- 4 files changed, 139 insertions(+), 46 deletions(-) diff --git a/pkg/notes/notes.go b/pkg/notes/notes.go index b0782563076..54a6cb046b1 100644 --- a/pkg/notes/notes.go +++ b/pkg/notes/notes.go @@ -50,6 +50,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 ) @@ -1030,59 +1031,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 99fd8486ca4..5f42628a50a 100644 --- a/pkg/notes/notes_gatherer_test.go +++ b/pkg/notes/notes_gatherer_test.go @@ -291,15 +291,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) @@ -309,6 +304,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 f514d5d1784..56ae939cc23 100644 --- a/pkg/notes/notes_test.go +++ b/pkg/notes/notes_test.go @@ -223,12 +223,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 c99d6cd66d3..c6ac8ede875 100644 --- a/pkg/notes/notes_v2.go +++ b/pkg/notes/notes_v2.go @@ -280,7 +280,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, @@ -301,11 +301,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] From 35981e7f310c99a8fc07fcb4a8b93c35aebf0a89 Mon Sep 17 00:00:00 2001 From: Traian Schiau Date: Wed, 11 Sep 2024 17:17:35 +0300 Subject: [PATCH 2/2] Preserve old behavior. --- pkg/notes/notes.go | 53 +++++++++++++++++++------------- pkg/notes/notes_gatherer_test.go | 11 +++++-- pkg/notes/notes_test.go | 6 ++-- pkg/notes/notes_v2.go | 6 ++-- 4 files changed, 46 insertions(+), 30 deletions(-) diff --git a/pkg/notes/notes.go b/pkg/notes/notes.go index 54a6cb046b1..fc78defa485 100644 --- a/pkg/notes/notes.go +++ b/pkg/notes/notes.go @@ -28,6 +28,7 @@ import ( "net/http" "net/url" "regexp" + "slices" "sort" "strconv" "strings" @@ -1031,25 +1032,26 @@ func (g *Gatherer) prsForCommitFromSHA(sha string) (prs []*gogithub.PullRequest, } func (g *Gatherer) prsForCommitFromMessage(commitMessage string) (prs []*gogithub.PullRequest, err error) { - mainPrNum, err := prNumForCommitFromMessage(commitMessage) + prsNum, err := prsNumForCommitFromMessage(commitMessage) if err != nil { return nil, err } - // 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) + for _, prNum := range prsNum { + // Given the PR number that we've now converted to an integer, get the PR from + // the API + pr, err := g.getPr(prNum) + if err != nil { + return prs, err + } + prs = append(prs, pr) - originPrNum, origPrErr := originPrNumFromPr(mainPr) - if origPrErr == nil { - originPr, err := g.getPr(originPrNum) - if err == nil { - prs = append(prs, originPr) + originPrNum, origPrErr := originPrNumFromPr(pr) + if origPrErr == nil && slices.Index(prsNum, originPrNum) == -1 { + originPr, err := g.getPr(originPrNum) + if err == nil { + prs = append(prs, originPr) + } } } @@ -1074,25 +1076,34 @@ var ( // stops being true, this definitely won't work anymore. regexMergedPR = regexp.MustCompile(`Merge pull request #(?P\d+)`) - // If the PR was squash merged, the regexp is different + // 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" + // 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 + // 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) { +func prsNumForCommitFromMessage(commitMessage string) (prs []int, err error) { if pr := prForRegex(regexMergedPR, commitMessage); pr != 0 { - return pr, nil + prs = append(prs, pr) } if pr := prForRegex(regexSquashPR, commitMessage); pr != 0 { - return pr, nil + prs = append(prs, pr) + } + + if pr := prForRegex(regexHackBranch, commitMessage); pr != 0 { + prs = append(prs, pr) + } + + if len(prs) == 0 { + return nil, errNoPRIDFoundInCommitMessage + } else { + return prs, nil } - return 0, errNoPRIDFoundInCommitMessage } func originPrNumFromPr(pr *gogithub.PullRequest) (origPRNum int, err error) { diff --git a/pkg/notes/notes_gatherer_test.go b/pkg/notes/notes_gatherer_test.go index 5f42628a50a..d06abafdb2a 100644 --- a/pkg/notes/notes_gatherer_test.go +++ b/pkg/notes/notes_gatherer_test.go @@ -291,10 +291,15 @@ 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", "and lastly in parens (#124) yeah"), + 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`), }, getPullRequestStubber: func(t *testing.T) getPullRequestStub { - seenPRs := newIntsRecorder(123, 124) + seenPRs := newIntsRecorder(123, 124, 125, 126, 127, 128) return func(_ context.Context, org, repo string, prID int) (*github.PullRequest, *github.Response, error) { checkOrgRepo(t, org, repo) @@ -304,7 +309,7 @@ func TestGatherNotes(t *testing.T) { return nil, nil, nil } }, - expectedGetPullRequestCallCount: 2, + expectedGetPullRequestCallCount: 6, }, "when the PR is a cherry pick": { diff --git a/pkg/notes/notes_test.go b/pkg/notes/notes_test.go index 56ae939cc23..f514d5d1784 100644 --- a/pkg/notes/notes_test.go +++ b/pkg/notes/notes_test.go @@ -223,12 +223,12 @@ func TestGetPRNumberFromCommitMessage(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - pr, err := prNumForCommitFromMessage(tc.commitMessage) + prs, err := prsNumForCommitFromMessage(tc.commitMessage) if err != nil { t.Fatalf("Expected no error to occur but got %v", err) } - if pr != tc.expectedPRNumber { - t.Errorf("Expected PR number to be %d but was %d", tc.expectedPRNumber, pr) + if prs[0] != tc.expectedPRNumber { + t.Errorf("Expected PR number to be %d but was %d", tc.expectedPRNumber, prs[0]) } }) } diff --git a/pkg/notes/notes_v2.go b/pkg/notes/notes_v2.go index c6ac8ede875..c99d6cd66d3 100644 --- a/pkg/notes/notes_v2.go +++ b/pkg/notes/notes_v2.go @@ -280,7 +280,7 @@ func (g *Gatherer) listLeftParentCommits(opts *options.Options) ([]*commitPrPair } // Find and collect PR number from commit message - prNum, err := prNumForCommitFromMessage(commitPointer.Message) + prNums, err := prsNumForCommitFromMessage(commitPointer.Message) if err == errNoPRIDFoundInCommitMessage { logrus.WithFields(logrus.Fields{ "sha": hashString, @@ -301,11 +301,11 @@ func (g *Gatherer) listLeftParentCommits(opts *options.Options) ([]*commitPrPair } logrus.WithFields(logrus.Fields{ "sha": hashString, - "pr": prNum, + "prs": prNums, }).Debug("found PR from commit") // Only taking the first one, assuming they are merged by Prow - pairs = append(pairs, &commitPrPair{Commit: commitPointer, PrNum: prNum}) + pairs = append(pairs, &commitPrPair{Commit: commitPointer, PrNum: prNums[0]}) // Advance pointer based on left parent hashPointer = commitPointer.ParentHashes[0]