Repository: scylladb/scylla-operator
Pull request #753 (2021-08-11 16:59:45)

Handle PRs with multiple "kind/" labels in release notes
**Description of your changes:**
When publishing https://github.com/scylladb/scylla-operator/releases/tag/v1.4.0 the release notes wrongly assigned PR #668 as a feature and it wasn't listed under API changes. This shows that our kinds are not mutually exclusive sets and we have to prioritize it. Like in this case API changes are subset of features but we want the PR listed under API changes.

With this change, the order of sections determines the priority and the first match wins.



Patch:
diff --git a/pkg/cmd/releasenotes/releasenotes.go b/pkg/cmd/releasenotes/releasenotes.go
index 1c046c0c8..bfccd26b7 100644
--- a/pkg/cmd/releasenotes/releasenotes.go
+++ b/pkg/cmd/releasenotes/releasenotes.go
@@ -54,7 +54,7 @@ func (o *GenerateOptions) Run(ctx context.Context) error {
 		numberToPR[int(prs[i].Number)] = &prs[i]
 	}
 
-	var filteredPRs []PullRequest
+	var filteredPRs []*PullRequest
 	var errs []error
 	for _, prNumber := range requiredPullRequestsNumbers {
 		pr, found := numberToPR[prNumber]
@@ -63,7 +63,7 @@ func (o *GenerateOptions) Run(ctx context.Context) error {
 			continue
 		}
 
-		filteredPRs = append(filteredPRs, *pr)
+		filteredPRs = append(filteredPRs, pr)
 	}
 	if errs != nil {
 		return errors.NewAggregate(errs)
diff --git a/pkg/cmd/releasenotes/render.go b/pkg/cmd/releasenotes/render.go
index edca0de92..702626adf 100644
--- a/pkg/cmd/releasenotes/render.go
+++ b/pkg/cmd/releasenotes/render.go
@@ -9,6 +9,8 @@ import (
 	"regexp"
 	"sort"
 	"strings"
+
+	"k8s.io/klog/v2"
 )
 
 const releaseNotesTemplate = `
@@ -63,7 +65,40 @@ func formatPR(pr PullRequest) (string, error) {
 
 type section struct {
 	Title        string
-	PullRequests []PullRequest
+	PullRequests []*PullRequest
+}
+
+func categorizePullRequests(prs []*PullRequest) []section {
+	remaining := map[int]*PullRequest{}
+	for i, pr := range prs {
+		remaining[i] = pr
+	}
+
+	// Given the sections are extracted from the map, the following list has to be ordered by priority
+	// in case there is more than one "kind/" label present.
+	sections := []section{
+		{"API Change", extract(remaining, byKinds("api-change"))},
+		{"Feature", extract(remaining, byKinds("feature"))},
+		{"Bug", extract(remaining, byKinds("bug"))},
+		{"Documentation", extract(remaining, byKinds("documentation"))},
+		{"Flaky/Failing Test", extract(remaining, byKinds("failing-test", "flake"))},
+		{"Other", extract(remaining, anyKind)},
+		{"Uncategorized", extract(remaining, missingKind)},
+	}
+
+	// The sets are mutually exclusive and there shall be no PR left.
+	if len(remaining) != 0 {
+		klog.Fatalf("Internal error: %d unmatched PRs: %#v", len(remaining), remaining)
+	}
+
+	for _, section := range sections {
+		// Sort sections to have a deterministic output.
+		sort.Slice(section.PullRequests, func(i, j int) bool {
+			return section.PullRequests[i].Number < section.PullRequests[j].Number
+		})
+	}
+
+	return sections
 }
 
 type releaseNoteData struct {
@@ -72,23 +107,13 @@ type releaseNoteData struct {
 	PreviousRelease string
 }
 
-func renderReleaseNotes(out io.Writer, release, previousRelease string, pullRequests []PullRequest) error {
+func renderReleaseNotes(out io.Writer, release, previousRelease string, pullRequests []*PullRequest) error {
 	// Render in the order of merge date.
 	sort.Slice(pullRequests, func(i, j int) bool {
 		return pullRequests[i].MergedAt.Before(pullRequests[j].MergedAt.Time)
 	})
 
-	prsByKind, uncategorized := splitByKind(pullRequests)
-
-	sections := []section{
-		{"API Change", extractByKind(prsByKind, "api-change")},
-		{"Feature", extractByKind(prsByKind, "feature")},
-		{"Bug", extractByKind(prsByKind, "bug")},
-		{"Documentation", extractByKind(prsByKind, "documentation")},
-		{"Flaky/Failing Test", extractByKind(prsByKind, "failing-test", "flake")},
-		{"Other", flatten(prsByKind)},
-		{"Uncategorized", uncategorized},
-	}
+	sections := categorizePullRequests(pullRequests)
 
 	data := releaseNoteData{
 		Release:         release,
@@ -111,44 +136,41 @@ func renderReleaseNotes(out io.Writer, release, previousRelease string, pullRequ
 	return nil
 }
 
-func flatten(m map[string][]PullRequest) []PullRequest {
-	var prs []PullRequest
-	for _, v := range m {
-		prs = append(prs, v...)
+func extract(prs map[int]*PullRequest, match func(*PullRequest) bool) []*PullRequest {
+	var res []*PullRequest
+
+	for key, pr := range prs {
+		if match(pr) {
+			res = append(res, pr)
+			delete(prs, key)
+		}
 	}
-	return prs
+
+	return res
 }
 
-func extractByKind(prsByKind map[string][]PullRequest, kinds ...string) []PullRequest {
-	var prs []PullRequest
-	for _, kind := range kinds {
-		prs = append(prs, prsByKind[kind]...)
-		delete(prsByKind, kind)
+func byKinds(kinds ...string) func(*PullRequest) bool {
+	return func(pr *PullRequest) bool {
+		for _, label := range pr.Labels.Nodes {
+			for _, kind := range kinds {
+				if string(label.Name) == "kind/"+kind {
+					return true
+				}
+			}
+		}
+		return false
 	}
-	return prs
 }
 
-func prKind(pr PullRequest) (string, bool) {
+func missingKind(pr *PullRequest) bool {
 	for _, label := range pr.Labels.Nodes {
-		parts := strings.Split(string(label.Name), "/")
-		if len(parts) == 2 && parts[0] == "kind" {
-			return parts[1], true
+		if strings.HasPrefix(string(label.Name), "kind/") {
+			return false
 		}
 	}
-	return "", false
+	return true
 }
 
-func splitByKind(prs []PullRequest) (map[string][]PullRequest, []PullRequest) {
-	prsByKind := make(map[string][]PullRequest)
-	var uncategorized []PullRequest
-
-	for _, pr := range prs {
-		kind, hasKind := prKind(pr)
-		if hasKind {
-			prsByKind[kind] = append(prsByKind[kind], pr)
-		} else {
-			uncategorized = append(uncategorized, pr)
-		}
-	}
-	return prsByKind, uncategorized
+func anyKind(pr *PullRequest) bool {
+	return !missingKind(pr)
 }


Test patch:
diff --git a/pkg/cmd/releasenotes/render_test.go b/pkg/cmd/releasenotes/render_test.go
index 7ecb964be..e1bad776a 100644
--- a/pkg/cmd/releasenotes/render_test.go
+++ b/pkg/cmd/releasenotes/render_test.go
@@ -3,7 +3,11 @@
 package releasenotes
 
 import (
+	"reflect"
 	"testing"
+
+	"github.com/google/go-cmp/cmp"
+	githubql "github.com/shurcooL/githubv4"
 )
 
 func TestCleanupPRTitle(t *testing.T) {
@@ -45,3 +49,283 @@ func TestCleanupPRTitle(t *testing.T) {
 		})
 	}
 }
+
+func makePR(number int, labels []string) *PullRequest {
+	pr := &PullRequest{
+		Number: githubql.Int(number),
+	}
+
+	for _, label := range labels {
+		pr.Labels.Nodes = append(pr.Labels.Nodes, struct {
+			Name githubql.String
+		}{
+			Name: githubql.String(label),
+		})
+	}
+
+	return pr
+}
+
+func TestCategorizePullRequests(t *testing.T) {
+	tt := []struct {
+		name             string
+		prs              []*PullRequest
+		expectedSections []section
+	}{
+		{
+			name: "no PRs return empty sections",
+			prs:  nil,
+			expectedSections: []section{
+				{
+					Title: "API Change",
+				},
+				{
+					Title: "Feature",
+				},
+				{
+					Title: "Bug",
+				},
+				{
+					Title: "Documentation",
+				},
+				{
+					Title: "Flaky/Failing Test",
+				},
+				{
+					Title: "Other",
+				},
+				{
+					Title: "Uncategorized",
+				},
+			},
+		},
+		{
+			name: "multiple PRs with the same kind lands in the same category",
+			prs: []*PullRequest{
+				makePR(1, nil),
+				makePR(2, []string{"priority/important-soon"}),
+				makePR(3, []string{"kind/feature"}),
+				makePR(4, []string{"kind/feature"}),
+				makePR(5, []string{"kind/bug"}),
+				makePR(6, []string{"kind/bug"}),
+				makePR(7, []string{"kind/api-change"}),
+				makePR(8, []string{"kind/api-change"}),
+				makePR(9, []string{"kind/documentation"}),
+				makePR(10, []string{"kind/documentation"}),
+				makePR(11, []string{"kind/failing-test"}),
+				makePR(12, []string{"kind/flake"}),
+				makePR(13, []string{"kind/foo"}),
+				makePR(14, []string{"kind/foo"}),
+			},
+			expectedSections: []section{
+				{
+					Title: "API Change",
+					PullRequests: []*PullRequest{
+						makePR(7, []string{"kind/api-change"}),
+						makePR(8, []string{"kind/api-change"}),
+					},
+				},
+				{
+					Title: "Feature",
+					PullRequests: []*PullRequest{
+						makePR(3, []string{"kind/feature"}),
+						makePR(4, []string{"kind/feature"}),
+					},
+				},
+				{
+					Title: "Bug",
+					PullRequests: []*PullRequest{
+						makePR(5, []string{"kind/bug"}),
+						makePR(6, []string{"kind/bug"}),
+					},
+				},
+				{
+					Title: "Documentation",
+					PullRequests: []*PullRequest{
+						makePR(9, []string{"kind/documentation"}),
+						makePR(10, []string{"kind/documentation"}),
+					},
+				},
+				{
+					Title: "Flaky/Failing Test",
+					PullRequests: []*PullRequest{
+						makePR(11, []string{"kind/failing-test"}),
+						makePR(12, []string{"kind/flake"}),
+					},
+				},
+				{
+					Title: "Other",
+					PullRequests: []*PullRequest{
+						makePR(13, []string{"kind/foo"}),
+						makePR(14, []string{"kind/foo"}),
+					},
+				},
+				{
+					Title: "Uncategorized",
+					PullRequests: []*PullRequest{
+						makePR(1, nil),
+						makePR(2, []string{"priority/important-soon"}),
+					},
+				},
+			},
+		},
+		{
+			name: "precedences",
+			prs: []*PullRequest{
+				makePR(1, []string{
+					"priority/important-soon",
+					"kind/foo",
+					"kind/flake",
+					"kind/failing-test",
+					"kind/documentation",
+					"kind/bug",
+					"kind/feature",
+					"kind/api-change",
+				}),
+				makePR(2, []string{
+					"priority/important-soon",
+					"kind/foo",
+					"kind/flake",
+					"kind/failing-test",
+					"kind/documentation",
+					"kind/bug",
+					"kind/feature",
+				}),
+				makePR(3, []string{
+					"priority/important-soon",
+					"kind/foo",
+					"kind/flake",
+					"kind/failing-test",
+					"kind/documentation",
+					"kind/bug",
+				}),
+				makePR(4, []string{
+					"priority/important-soon",
+					"kind/foo",
+					"kind/flake",
+					"kind/failing-test",
+					"kind/documentation",
+				}),
+				makePR(5, []string{
+					"priority/important-soon",
+					"kind/foo",
+					"kind/flake",
+					"kind/failing-test",
+				}),
+				makePR(6, []string{
+					"priority/important-soon",
+					"kind/foo",
+					"kind/flake",
+				}),
+				makePR(7, []string{
+					"priority/important-soon",
+					"kind/foo",
+				}),
+				makePR(8, []string{
+					"priority/important-soon",
+				}),
+				makePR(9, nil),
+			},
+			expectedSections: []section{
+				{
+					Title: "API Change",
+					PullRequests: []*PullRequest{
+						makePR(1, []string{
+							"priority/important-soon",
+							"kind/foo",
+							"kind/flake",
+							"kind/failing-test",
+							"kind/documentation",
+							"kind/bug",
+							"kind/feature",
+							"kind/api-change",
+						}),
+					},
+				},
+				{
+					Title: "Feature",
+					PullRequests: []*PullRequest{
+						makePR(2, []string{
+							"priority/important-soon",
+							"kind/foo",
+							"kind/flake",
+							"kind/failing-test",
+							"kind/documentation",
+							"kind/bug",
+							"kind/feature",
+						}),
+					},
+				},
+				{
+					Title: "Bug",
+					PullRequests: []*PullRequest{
+						makePR(3, []string{
+							"priority/important-soon",
+							"kind/foo",
+							"kind/flake",
+							"kind/failing-test",
+							"kind/documentation",
+							"kind/bug",
+						}),
+					},
+				},
+				{
+					Title: "Documentation",
+					PullRequests: []*PullRequest{
+						makePR(4, []string{
+							"priority/important-soon",
+							"kind/foo",
+							"kind/flake",
+							"kind/failing-test",
+							"kind/documentation",
+						}),
+					},
+				},
+				{
+					Title: "Flaky/Failing Test",
+					PullRequests: []*PullRequest{
+						makePR(5, []string{
+							"priority/important-soon",
+							"kind/foo",
+							"kind/flake",
+							"kind/failing-test",
+						}),
+						makePR(6, []string{
+							"priority/important-soon",
+							"kind/foo",
+							"kind/flake",
+						}),
+					},
+				},
+				{
+					Title: "Other",
+					PullRequests: []*PullRequest{
+						makePR(7, []string{
+							"priority/important-soon",
+							"kind/foo",
+						}),
+					},
+				},
+				{
+					Title: "Uncategorized",
+					PullRequests: []*PullRequest{
+						makePR(8, []string{
+							"priority/important-soon",
+						}),
+						makePR(9, nil),
+					},
+				},
+			},
+		},
+	}
+
+	for _, tc := range tt {
+		t.Run(tc.name, func(t *testing.T) {
+			got := categorizePullRequests(tc.prs)
+
+			if !reflect.DeepEqual(got, tc.expectedSections) {
+				t.Errorf("expected and got sections differ: %s", cmp.Diff(tc.expectedSections, got))
+			}
+		})
+	}
+}