From f8f4f4619ec8e493af757b50217af3e9de0fb671 Mon Sep 17 00:00:00 2001 From: Traian Schiau Date: Wed, 14 Feb 2024 17:57:45 +0200 Subject: [PATCH] [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. --- go.mod | 2 +- pkg/notes/notes.go | 92 ++++++++++++++++++++------------ pkg/notes/notes_gatherer_test.go | 81 +++++++++++++++++++++++++--- pkg/notes/notes_test.go | 6 +-- pkg/notes/notes_v2.go | 6 +-- 5 files changed, 140 insertions(+), 47 deletions(-) diff --git a/go.mod b/go.mod index 1bd803929824..6eecb75d8017 100644 --- a/go.mod +++ b/go.mod @@ -38,6 +38,7 @@ require ( google.golang.org/api v0.152.0 gopkg.in/yaml.v2 v2.4.0 k8s.io/apimachinery v0.29.1 + k8s.io/utils v0.0.0-20240102154912-e7106e64919e sigs.k8s.io/bom v0.5.2-0.20230519223618-1ebaa9ce375f sigs.k8s.io/mdtoc v1.1.0 sigs.k8s.io/promo-tools/v3 v3.6.0 @@ -290,7 +291,6 @@ require ( k8s.io/client-go v0.28.4 // indirect k8s.io/klog/v2 v2.110.1 // indirect k8s.io/kube-openapi v0.0.0-20231010175941-2dd684a91f00 // indirect - k8s.io/utils v0.0.0-20240102154912-e7106e64919e // indirect sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd // indirect sigs.k8s.io/structured-merge-diff/v4 v4.4.1 // indirect ) diff --git a/pkg/notes/notes.go b/pkg/notes/notes.go index 8d97f92960cd..80cfb3b10f52 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,84 @@ 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 (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 + } + } +} -func prsNumForCommitFromMessage(commitMessage string) (prs []int, err error) { +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..8c3b1ca9bc56 100644 --- a/pkg/notes/notes_gatherer_test.go +++ b/pkg/notes/notes_gatherer_test.go @@ -29,6 +29,7 @@ import ( "github.com/google/go-github/v58/github" "github.com/sirupsen/logrus" + "k8s.io/utils/ptr" "sigs.k8s.io/release-sdk/git" "sigs.k8s.io/release-sdk/github/githubfakes" ) @@ -289,15 +290,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 +303,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: ptr.To[int](122), + Body: ptr.To("122\n```release-note\nfrom 122\n```\n"), + }, + 123: { + Number: ptr.To[int](123), + Body: ptr.To("123\n```release-note\nfrom 123\n```\n"), + }, + 124: { + Number: ptr.To[int](124), + Body: ptr.To("124\n```release-note\nfrom 124\n```\n"), + }, + 125: { + Number: ptr.To[int](125), + Body: ptr.To("No release note"), + Head: &github.PullRequestBranch{ + Label: ptr.To("k8s-infra-cherrypick-robot:cherry-pick-122-to-release-0.x"), + }, + }, + 126: { + Number: ptr.To[int](126), + Body: ptr.To("Empty release note\n```release-note\n\n```\n"), + Head: &github.PullRequestBranch{ + Label: ptr.To("fork:automated-cherry-pick-of-#123-upstream-release-0.x"), + }, + }, + 127: { + Number: ptr.To[int](127), + Body: ptr.To("127\n```release-note\nfrom 127\n```\n"), + Head: &github.PullRequestBranch{ + Label: ptr.To("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]