Improve log messages, commets and variable names

Author: g-gaston

hack/tools/release/notes/list.go

@@ -32,7 +32,9 @@ import (
 type githubFromToPRLister struct {
 	client         *githubClient
 	fromRef, toRef ref
-	branch         string
+	// branch is optional. It helps optimize the PR query by restricting
+	// the results to PRs merged in the selected branch and in main
+	branch string
 }
 
 func newGithubFromToPRLister(repo string, fromRef, toRef ref, branch string) *githubFromToPRLister {
@@ -44,8 +46,7 @@ func newGithubFromToPRLister(repo string, fromRef, toRef ref, branch string) *gi
 	}
 }
 
-// listPRs queries the PRs merged in a branch after
-// the `fromRef` reference and before `toRef` (included).
+// listPRs returns the PRs merged between `fromRef` and `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)
@@ -83,19 +84,19 @@ func (l *githubFromToPRLister) listPRs() ([]pr, error) {
 	// 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)
+	log.Printf("Listing PRs from %s to %s", fromDate, toDate)
 	gPRs, err := l.client.listMergedPRs(fromDate, toDate, l.branch, "main")
 	if err != nil {
 		return nil, err
 	}
 
 	log.Printf("Found %d PRs in github", len(gPRs))
 
-	prIDs := buildSetOfPrIDs(diff.Commits)
+	selectedPRNumbers := buildSetOfPRNumbers(diff.Commits)
 
 	prs := make([]pr, 0, len(gPRs))
 	for _, p := range gPRs {
-		if _, ok := prIDs[fmt.Sprintf("%d", p.Number)]; !ok {
+		if _, ok := selectedPRNumbers[fmt.Sprintf("%d", p.Number)]; !ok {
 			continue
 		}
 		labels := make([]string, 0, len(p.Labels))
@@ -111,24 +112,24 @@ func (l *githubFromToPRLister) 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))
+	if len(prs) != len(selectedPRNumbers) {
+		return nil, errors.Errorf("expected %d PRs from commit list but only found %d", len(selectedPRNumbers), 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{})
+func buildSetOfPRNumbers(commits []githubCommitNode) map[string]struct{} {
+	prNumbers := make(map[string]struct{})
 	for _, commit := range commits {
 		match := mergeCommitMessage.FindStringSubmatch(commit.Commit.Message)
 		if len(match) != 2 {
 			continue
 		}
-		prIDs[match[1]] = struct{}{}
+		prNumbers[match[1]] = struct{}{}
 	}
 
-	return prIDs
+	return prNumbers
 }

hack/tools/release/notes/main.go

@@ -187,6 +187,8 @@ func computeConfigDefaults(config *notesCmdConfig) error {
 	return nil
 }
 
+// defaultBranchForNewTag calculates the branch to cut a release from based on
+// on the new release tag.
 func defaultBranchForNewTag(newTag semver.Version) string {
 	if newTag.Patch == 0 {
 		if len(newTag.Pre) == 0 {

hack/tools/release/notes/ref.go

@@ -25,8 +25,11 @@ import (
 	"github.com/pkg/errors"
 )
 
+// ref represents a git reference.
 type ref struct {
-	reType, value string
+	// reType is the ref type: tags for a tag, head for a branch, commit for a commit.
+	reType string
+	value  string
 }
 
 func (r ref) String() string {

hack/tools/release/notes/release_notes_integration_test.go

@@ -44,12 +44,12 @@ func TestReleaseNotesIntegration(t *testing.T) {
 			expected: "test/golden/v1.3.10.md",
 		},
 		{
-			// The release notes command computes everything from last tag
-			// to HEAD. Hence if we use the head of release-1.5, this test will
-			// become invalid every time we backport some PR to release branch release-1.5.
-			// Here we cheat a little by poiting to worktree to v1.5.0, which is the
-			// release that this test is simulating, so it should not exist yet. But
-			// it represents accurately the HEAD of release-1.5 when we released v1.5.0.
+			// By default when using the `--release` options, notes command computes
+			// everything from last tag to HEAD. Hence if we use the head of release-1.5,
+			// this test will become invalid every time we backport some PR to release
+			// branch release-1.5.
+			// Instead, to simulate the v1.5.0 release, we manually set the `--to` flag,
+			// which sets the upper boundary for the PR search.
 			name:     "new minor",
 			args:     []string{"--from", "tags/v1.4.0", "--to", "tags/v1.5.0", "--branch", "release-1.5"},
 			expected: "test/golden/v1.5.0.md",