Fix issues and make test pass

hack/tools/release/notes/github.go

@@ -25,26 +25,111 @@ import (
 	"log"
 	"math"
 	"os/exec"
+	"strings"
 	"time"
 )
 
+// githubClient uses the gh CLI to make API request to GitHub.
+type githubClient struct {
+	// repo is full [org]/[repo_name]
+	repo string
+}
+
 // githubDiff is the API response for the "compare" endpoint.
 type githubDiff struct {
 	// MergeBaseCommit points to most recent common ancestor between two references.
-	MergeBaseCommit githubCommitNode `json:"merge_base_commit"`
+	MergeBaseCommit githubCommitNode   `json:"merge_base_commit"`
+	Commits         []githubCommitNode `json:"commits"`
+	Total           int                `json:"total_commits"`
 }
 
 type githubCommitNode struct {
 	Commit githubCommit `json:"commit"`
 }
 
+type githubCommitter struct {
+	Date time.Time `json:"date"`
+}
+
+// getDiff calls the `compare` endpoint.
+func (c githubClient) getDiff(base, head string) (githubDiff, error) {
+	diff := githubDiff{}
+	if err := c.runGHAPICommand(fmt.Sprintf("repos/%s/compare/%s...%s?per_page=1'", c.repo, base, head), &diff); err != nil {
+		return githubDiff{}, err
+	}
+	return diff, nil
+}
+
+// getDiffAllCommits calls the `compare` endpoint, iterating over all pages and aggregating results.
+func (c githubClient) getDiffAllCommits(base, head string) (*githubDiff, error) {
+	pageSize := 250
+	url := fmt.Sprintf("repos/%s/compare/%s...%s", c.repo, base, head)
+
+	diff, commits, err := iterate(c, url, pageSize, nil, func(new *githubDiff) ([]githubCommitNode, int) {
+		return new.Commits, new.Total
+	})
+	if err != nil {
+		return nil, err
+	}
+
+	diff.Commits = commits
+
+	return diff, nil
+}
+
+// githubRef is the API response for the "ref" endpoint.
+type githubRef struct {
+	Object githubObject `json:"object"`
+}
+
+type objectType string
+
+const (
+	commitType objectType = "commit"
+	tagType    objectType = "tag"
+)
+
+type githubObject struct {
+	ObjectType objectType `json:"type"`
+	SHA        string     `json:"sha"`
+}
+
+// getDiff calls the `compare` endpoint.
+func (c githubClient) getRef(ref string) (githubRef, error) {
+	refResponse := githubRef{}
+	if err := c.runGHAPICommand(fmt.Sprintf("repos/%s/git/ref/%s", c.repo, ref), &refResponse); err != nil {
+		return githubRef{}, err
+	}
+	return refResponse, nil
+}
+
+// githubTag is the API response for the "tags" endpoint.
+type githubTag struct {
+	Object githubObject `json:"object"`
+}
+
+// getTag calls the `tags` endpoint.
+func (c githubClient) getTag(tagSHA string) (githubTag, error) {
+	tagResponse := githubTag{}
+	if err := c.runGHAPICommand(fmt.Sprintf("repos/%s/git/tags/%s", c.repo, tagSHA), &tagResponse); err != nil {
+		return githubTag{}, err
+	}
+	return tagResponse, nil
+}
+
 // githubCommit is the API response for a "git/commits" request.
 type githubCommit struct {
+	Message   string          `json:"message"`
 	Committer githubCommitter `json:"committer"`
 }
 
-type githubCommitter struct {
-	Date time.Time `json:"date"`
+// getCommit calls the `commits` endpoint.
+func (c githubClient) getCommit(sha string) (githubCommit, error) {
+	commit := githubCommit{}
+	if err := c.runGHAPICommand(fmt.Sprintf("repos/%s/git/commits/%s", c.repo, sha), &commit); err != nil {
+		return githubCommit{}, err
+	}
+	return commit, nil
 }
 
 // githubPRList is the API response for the "search" endpoint.
@@ -65,42 +150,19 @@ type githubLabel struct {
 	Name string `json:"name"`
 }
 
-// githubClient uses the gh CLI to make API request to GitHub.
-type githubClient struct {
-	// repo is full [org]/[repo_name]
-	repo string
-}
-
-// getDiff calls the `compare` endpoint.
-func (c githubClient) getDiff(base, head string) (githubDiff, error) {
-	diff := githubDiff{}
-	if err := c.runGHAPICommand(fmt.Sprintf("repos/%s/compare/%s...%s?per_page=1'", c.repo, base, head), &diff); err != nil {
-		return githubDiff{}, err
-	}
-	return diff, nil
-}
-
 // listMergedPRs calls the `search` endpoint and queries for PRs.
-func (c githubClient) listMergedPRs(baseBranch string, after time.Time) ([]githubPR, error) {
+func (c githubClient) listMergedPRs(baseBranch string, after, before time.Time) ([]githubPR, error) {
 	pageSize := 100
-	page := 0
-	totalPRs := math.MaxInt
-
-	searchQuery := fmt.Sprintf("repo:%s+base:%s+is:pr+is:merged+merged:>%s", c.repo, baseBranch, after.Format(time.RFC3339))
-
-	var prs []githubPR
-
-	for len(prs) < totalPRs {
-		page++
-		url := fmt.Sprintf("search/issues?per_page=%d&page=%d&q=%s", pageSize, page, searchQuery)
-		log.Println("Calling search endpoint: " + url)
-		prList := githubPRList{}
-		if err := c.runGHAPICommand(url, &prList); err != nil {
-			return nil, err
-		}
+	searchQuery := fmt.Sprintf("repo:%s+is:pr+is:merged+merged:%s..%s", c.repo, after.Format(time.RFC3339), before.Format(time.RFC3339))
+	if baseBranch != "" {
+		searchQuery += "+base:" + baseBranch
+	}
 
-		prs = append(prs, prList.Items...)
-		totalPRs = prList.Total
+	_, prs, err := iterate(c, "search/issues", pageSize, []string{"q=" + searchQuery}, func(new *githubPRList) ([]githubPR, int) {
+		return new.Items, new.Total
+	})
+	if err != nil {
+		return nil, err
 	}
 
 	return prs, nil
@@ -116,3 +178,37 @@ func (c githubClient) runGHAPICommand(url string, response any) error {
 
 	return json.Unmarshal(out, response)
 }
+
+type extractFunc[T, C any] func(newPage *T) (pageElements []C, totalElements int)
+
+func iterate[T, C any](client githubClient, url string, pageSize int, extraQueryArgs []string, extract extractFunc[T, C]) (*T, []C, error) {
+	page := 0
+	totalElements := math.MaxInt
+	elementsRead := 0
+
+	var firstPage *T
+	var collection []C
+
+	for elementsRead < totalElements {
+		page++
+		requestURL := fmt.Sprintf("%s?per_page=%d&page=%d&%s", url, pageSize, page, strings.Join(extraQueryArgs, "&"))
+		log.Printf("Calling endpoint %s", requestURL)
+		pageResult := new(T)
+		if err := client.runGHAPICommand(requestURL, pageResult); err != nil {
+			return nil, nil, err
+		}
+
+		pageRead, t := extract(pageResult)
+		collection = append(collection, pageRead...)
+		elementsRead += len(pageRead)
+		totalElements = t
+
+		if firstPage == nil {
+			firstPage = pageResult
+		}
+	}
+
+	log.Printf("Total of %d pages and %d elements read", page, len(collection))
+
+	return firstPage, collection, nil
+}

hack/tools/release/notes/list.go

@@ -20,42 +20,84 @@ limitations under the License.
 package main
 
 import (
+	"fmt"
 	"log"
+	"regexp"
+	"time"
+
+	"github.com/pkg/errors"
 )
 
-// githubPRLister lists PRs from GitHub.
-type githubPRLister struct {
-	client       *githubClient
-	from, branch string
+// githubFromToPRLister lists PRs from GitHub contained between two refs.
+type githubFromToPRLister struct {
+	client         *githubClient
+	fromRef, toRef ref
+	branch         string
 }
 
-func newPRLister(repo, fromTag, branch string) *githubPRLister {
-	return &githubPRLister{
-		client: &githubClient{repo: repo},
-		from:   fromTag,
-		branch: branch,
+func newGithubFromToPRLister(repo string, fromRef, toRef ref, branch string) *githubFromToPRLister {
+	return &githubFromToPRLister{
+		client:  &githubClient{repo: repo},
+		fromRef: fromRef,
+		toRef:   toRef,
+		branch:  branch,
 	}
 }
 
 // listPRs queries the PRs merged in a branch after
-// the `from` reference.
-func (l *githubPRLister) listPRs() ([]pr, error) {
-	log.Printf("Computing diff between %s and %s", l.branch, l.from)
-	diff, err := l.client.getDiff(l.branch, l.from)
+// the `fromRef` reference and before `toRef` (included).
+func (l *githubFromToPRLister) listPRs() ([]pr, error) {
+	log.Printf("Computing diff between %s and %s", l.fromRef, l.toRef)
+	diff, err := l.client.getDiffAllCommits(l.fromRef.value, l.toRef.value)
+	if err != nil {
+		return nil, err
+	}
+
+	log.Printf("Reading ref %s for upper limit", l.toRef)
+	toRef, err := l.client.getRef(l.toRef.String())
+	if err != nil {
+		return nil, err
+	}
+
+	var toCommitSHA string
+	if toRef.Object.ObjectType == tagType {
+		log.Printf("Reading tag info %s for upper limit", toRef.Object.SHA)
+		toTag, err := l.client.getTag(toRef.Object.SHA)
+		if err != nil {
+			return nil, err
+		}
+		toCommitSHA = toTag.Object.SHA
+	} else {
+		toCommitSHA = toRef.Object.SHA
+	}
+
+	log.Printf("Reading commit %s for upper limit", toCommitSHA)
+	toCommit, err := l.client.getCommit(toCommitSHA)
 	if err != nil {
 		return nil, err
 	}
 
-	log.Printf("Listing PRs from branch %s starting after %s", l.branch, diff.MergeBaseCommit.Commit.Committer.Date)
-	gPRs, err := l.client.listMergedPRs(l.branch, diff.MergeBaseCommit.Commit.Committer.Date)
+	fromDate := diff.MergeBaseCommit.Commit.Committer.Date
+	// We add an extra minute to avoid errors by 1 (observed durin testing)
+	// We cross check the list of PRs against the list of commits, so we will filter out
+	// any PRs entries not belonging to the computed diff
+	toDate := toCommit.Committer.Date.Add(1 * time.Minute)
+
+	log.Printf("Listing PRs from branch %s from %s to %s", l.branch, fromDate, toDate)
+	gPRs, err := l.client.listMergedPRs("", fromDate, toDate)
 	if err != nil {
 		return nil, err
 	}
 
 	log.Printf("Found %d PRs in github", len(gPRs))
 
+	prIDs := buildSetOfPrIDs(diff.Commits)
+
 	prs := make([]pr, 0, len(gPRs))
 	for _, p := range gPRs {
+		if _, ok := prIDs[fmt.Sprintf("%d", p.Number)]; !ok {
+			continue
+		}
 		labels := make([]string, 0, len(p.Labels))
 		for _, l := range p.Labels {
 			labels = append(labels, l.Name)
@@ -67,5 +109,26 @@ func (l *githubPRLister) listPRs() ([]pr, error) {
 		})
 	}
 
+	log.Printf("%d PRs match the commits from the git diff", len(prs))
+
+	if len(prs) != len(prIDs) {
+		return nil, errors.Errorf("expected %d PRs from commit list but only found %d", len(prIDs), len(prs))
+	}
+
 	return prs, nil
 }
+
+var mergeCommitMessage = regexp.MustCompile(`(?m)^Merge pull request #(\d+) .*$`)
+
+func buildSetOfPrIDs(commits []githubCommitNode) map[string]struct{} {
+	prIDs := make(map[string]struct{})
+	for _, commit := range commits {
+		match := mergeCommitMessage.FindStringSubmatch(commit.Commit.Message)
+		if len(match) != 2 {
+			continue
+		}
+		prIDs[match[1]] = struct{}{}
+	}
+
+	return prIDs
+}