Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[notes] Rework getting the number of the origin PR for cherry-picks #3468

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 63 additions & 25 deletions pkg/notes/notes.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
"net/http"
"net/url"
"regexp"
"slices"
"sort"
"strconv"
"strings"
Expand All @@ -50,6 +51,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
)
Expand Down Expand Up @@ -1034,55 +1036,91 @@ func (g *Gatherer) prsForCommitFromMessage(commitMessage string) (prs []*gogithu
if err != nil {
return nil, err
}
var res *gogithub.PullRequest
var resp *gogithub.Response

for _, pr := range prsNum {
for _, prNum := 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
pr, err := g.getPr(prNum)
if err != nil {
return prs, err
}
prs = append(prs, pr)

originPrNum, origPrErr := originPrNumFromPr(pr)
if origPrErr == nil && slices.Index(prsNum, originPrNum) == -1 {
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 {
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 prsNumForCommitFromMessage(commitMessage string) (prs []int, err error) {
if pr := prForRegex(regexMergedPR, commitMessage); pr != 0 {
prs = append(prs, pr)
}

regex = regexp.MustCompile(`automated-cherry-pick-of-#(?P<number>\d+)`)
trasc marked this conversation as resolved.
Show resolved Hide resolved
pr = prForRegex(regex, commitMessage)
if pr != 0 {
if pr := prForRegex(regexSquashPR, commitMessage); pr != 0 {
prs = append(prs, pr)
}

// If the PR was squash merged, the regexp is different
regex = regexp.MustCompile(`\(#(?P<number>\d+)\)`)
pr = prForRegex(regex, commitMessage)
if pr != 0 {
if pr := prForRegex(regexHackBranch, commitMessage); pr != 0 {
prs = append(prs, pr)
}

if prs == nil {
if len(prs) == 0 {
return nil, errNoPRIDFoundInCommitMessage
} else {
return prs, nil
}
}

return prs, nil
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
}

origPRNum = prForRegex(regexProwBranch, *pr.Head.Label)
if origPRNum != 0 {
return origPRNum, nil
}

return 0, errNoOriginPRIDFoundInPR
}

func prForRegex(regex *regexp.Regexp, commitMessage string) int {
Expand Down
71 changes: 71 additions & 0 deletions pkg/notes/notes_gatherer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -311,6 +311,77 @@ func TestGatherNotes(t *testing.T) {
},
expectedGetPullRequestCallCount: 6,
},

"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": {
commitList: []*github.RepositoryCommit{repoCommit("some-sha", "some #123 thing")},
listPullRequestsWithCommitStubber: func(t *testing.T) listPullRequestsWithCommitStub {
Expand Down
Loading