[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.
kubernetes · Feb 14, 2024 · e39476e · e39476e
1 parent d1687a2
commit e39476e
Show file tree

Hide file tree

Showing 5 changed files with 141 additions and 47 deletions.
diff --git 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
@@ -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<number>\d+)`)
-	pr := prForRegex(regex, commitMessage)
-	if pr != 0 {
-		prs = append(prs, pr)
+	regexMergedPR = regexp.MustCompile(`Merge pull request #(?P<number>\d+)`)
+
+	// If the PR was squash merged, the regexp is different
+	regexSquashPR = regexp.MustCompile(`\(#(?P<number>\d+)\)`)
+
+	// The branch name created by "hack/cherry_pick_pull.sh"
+	regexHackBranch = regexp.MustCompile(`automated-cherry-pick-of-#(?P<number>\d+)`)
+
+	// The branch name created by k8s-infra-cherrypick-robot
+	regexProwBranch = regexp.MustCompile(`cherry-pick-(?P<number>\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<number>\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<number>\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
@@ -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
@@ -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
@@ -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]