WIP: Pass a list of NoteCategory to template instead of map of kind t…

go.mod

@@ -11,6 +11,7 @@ require (
 	github.com/google/go-github/v29 v29.0.3
 	github.com/google/uuid v1.1.1
 	github.com/kr/pretty v0.1.0
+	github.com/kylelemons/godebug v1.1.0
 	github.com/maxbrunsfeld/counterfeiter/v6 v6.2.2
 	github.com/nozzle/throttler v0.0.0-20180817012639-2ea982251481
 	github.com/pkg/errors v0.9.1

go.sum

@@ -226,6 +226,8 @@ github.com/kr/pty v1.1.1/go.mod h1:pFQYn66WHrOpPYNljwOMqo10TkYh1fy3cYio2l3bCsQ=
 github.com/kr/pty v1.1.8/go.mod h1:O1sed60cT9XZ5uDucP5qwvh+TE3NnUj51EiZO/lmSfw=
 github.com/kr/text v0.1.0 h1:45sCR5RtlFHMR4UwH9sdQ5TC8v0qDQCHnXt+kaKSTVE=
 github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI=
+github.com/kylelemons/godebug v1.1.0 h1:RPNrshWIDI6G2gRW9EHilWtl7Z6Sb1BR0xunSBf0SNc=
+github.com/kylelemons/godebug v1.1.0/go.mod h1:9/0rRGxNHcop5bhtWyNeEfOS8JIWk580+fNqagV/RAw=
 github.com/lib/pq v1.0.0/go.mod h1:5WUZQaWbwv1U+lTReE5YruASi9Al49XbQIvNi/34Woo=
 github.com/lib/pq v1.2.0/go.mod h1:5WUZQaWbwv1U+lTReE5YruASi9Al49XbQIvNi/34Woo=
 github.com/logrusorgru/aurora v0.0.0-20181002194514-a7b3b318ed4e/go.mod h1:7rIyQOR62GCctdiQpZ/zOJlFyk6y+94wXzv6RNZgaR4=

pkg/notes/document/BUILD.bazel

@@ -21,6 +21,7 @@ go_test(
         "//pkg/notes/internal:go_default_library",
         "//pkg/release:go_default_library",
         "@com_github_kr_pretty//:go_default_library",
+        "@com_github_kylelemons_godebug//diff:go_default_library",
         "@com_github_stretchr_testify//require:go_default_library",
         "@io_bazel_rules_go//go/tools/bazel:go_default_library",
     ],

pkg/notes/document/document.go

@@ -34,13 +34,58 @@ import (
 // Document represents the underlying structure of a release notes document.
 type Document struct {
 	NotesWithActionRequired Notes         `json:"action_required"`
-	NotesUncategorized      Notes         `json:"uncategorized"`
 	NotesByKind             NotesByKind   `json:"kinds"`
 	Downloads               *FileMetadata `json:"downloads"`
 	CurrentRevision         string        `json:"release_tag"`
 	PreviousRevision        string
 }
 
+// REVIEWER: This really should be just `Notes` but I want to get some feedback
+// on this idea before doing more refactoring along those lines. This container
+// is useful for several reasons:
+// (1) Its a single container to pass to the template which keesp the template simple.
+// (2) We can sort it easily before rendering.
+// (3) Because we use NoteCategory and a collection rendering in the template is simpler:
+//
+// {{range .NoteCollection}}
+// 	### {{.Kind | prettyKind}}
+//		{{- range $note := .Notes}} - $note {{-end}}
+// {{end}}
+//
+// Of course this assumes the note collection is sorted already which we'll do before rendering.
+
+// NoteCollection is a collection of note categories and implements sorted.Interface
+type NoteCollection []NoteCategory
+
+// Sort sorts selection by sorted by priority order.
+func (n *NoteCollection) Sort(kindPriority []Kind) error {
+	// MAYBE? Implement sorted.Interface instead?
+	if len(kindPriority) == 0 {
+		return errors.New("invalid argument kindPriority cannot be nil")
+	}
+
+	indexOf := func(kind Kind) int {
+		for i, prioKind := range kindPriority {
+			if kind == prioKind {
+				return i
+			}
+		}
+		return -1
+	}
+
+	noteSlice := (*n)
+	sort.Slice(noteSlice, func(i, j int) bool {
+		return indexOf(noteSlice[i].Kind) < indexOf(noteSlice[j].Kind)
+	})
+	return nil
+}
+
+// NoteCategory is a collection for a given kind.
+type NoteCategory struct {
+	Kind  Kind
+	Notes Notes
+}
+
 // FileMetadata contains metadata about files associated with the release.
 type FileMetadata struct {
 	// Files containing source code.
@@ -135,16 +180,18 @@ type NotesByKind map[Kind]Notes
 type Notes []string
 
 const (
-	KindAPIChange       Kind = "api-change"
-	KindBug             Kind = "bug"
-	KindCleanup         Kind = "cleanup"
-	KindDeprecation     Kind = "deprecation"
-	KindDesign          Kind = "design"
-	KindDocumentation   Kind = "documentation"
-	KindFailingTest     Kind = "failing-test"
-	KindFeature         Kind = "feature"
-	KindFlake           Kind = "flake"
+	KindAPIChange     Kind = "api-change"
+	KindBug           Kind = "bug"
+	KindCleanup       Kind = "cleanup"
+	KindDeprecation   Kind = "deprecation"
+	KindDesign        Kind = "design"
+	KindDocumentation Kind = "documentation"
+	KindFailingTest   Kind = "failing-test"
+	KindFeature       Kind = "feature"
+	KindFlake         Kind = "flake"
+	// TODO: These should be same case as the others. Probably fix up prettyKind()??
 	KindBugCleanupFlake Kind = "Other (Bug, Cleanup or Flake)"
+	KindUncategorized   Kind = "Uncategorized"
 )
 
 var kindPriority = []Kind{
@@ -158,6 +205,7 @@ var kindPriority = []Kind{
 	KindCleanup,
 	KindFlake,
 	KindBugCleanupFlake,
+	KindUncategorized,
 }
 
 var kindMap = map[Kind]Kind{
@@ -171,7 +219,6 @@ var kindMap = map[Kind]Kind{
 func CreateDocument(releaseNotes notes.ReleaseNotes, history notes.ReleaseNotesHistory) (*Document, error) {
 	doc := &Document{
 		NotesWithActionRequired: Notes{},
-		NotesUncategorized:      Notes{},
 		NotesByKind:             NotesByKind{},
 	}
 
@@ -201,12 +248,18 @@ func CreateDocument(releaseNotes notes.ReleaseNotes, history notes.ReleaseNotesH
 
 			if len(note.Kinds) == 0 {
 				// the note has not been categorized so far
-				doc.NotesUncategorized = append(doc.NotesUncategorized, note.Markdown)
+				kind := KindUncategorized
+				if existingNotes, ok := doc.NotesByKind[kind]; ok {
+					if ok {
+						doc.NotesByKind[kind] = append(existingNotes, note.Markdown)
+					} else {
+						doc.NotesByKind[kind] = []string{note.Markdown}
+					}
+				}
 			}
 		}
 	}
 
-	sort.Strings(doc.NotesUncategorized)
 	sort.Strings(doc.NotesWithActionRequired)
 	return doc, nil
 }
@@ -292,17 +345,6 @@ func (d *Document) RenderMarkdown(bucket, tars, prevTag, newTag string) (string,
 		nlnl()
 	}
 
-	// We call the uncategorized notes "Other Changes". These are changes
-	// without any kind
-	if len(d.NotesUncategorized) > 0 {
-		o.WriteString("## Other Changes")
-		nlnl()
-		for _, note := range d.NotesUncategorized {
-			writeNote(note)
-		}
-		nlnl()
-	}
-
 	return strings.TrimSpace(o.String()), nil
 }
 

pkg/notes/document/document_test.go

@@ -22,9 +22,11 @@ import (
 	"path/filepath"
 	"strings"
 	"testing"
+	"text/template"
 
 	"github.com/bazelbuild/rules_go/go/tools/bazel"
 	"github.com/kr/pretty"
+	"github.com/kylelemons/godebug/diff"
 	"github.com/stretchr/testify/require"
 
 	"k8s.io/release/pkg/notes/internal"
@@ -144,7 +146,6 @@ func TestRenderMarkdownTemplate(t *testing.T) {
 
 	doc := Document{
 		NotesWithActionRequired: []string{"If an API changes and no one documented it, did it really happen?"},
-		NotesUncategorized:      []string{"Someone somewhere did the world a great justice."},
 		NotesByKind: NotesByKind{
 			KindAPIChange:       []string{"This might make people sad...or happy."},
 			KindBug:             []string{"This will likely get you promoted."},
@@ -156,6 +157,7 @@ func TestRenderMarkdownTemplate(t *testing.T) {
 			KindFeature:         []string{"This will get you promoted."},
 			KindFlake:           []string{"This *should* get you promoted."},
 			KindBugCleanupFlake: []string{"This should definitely get you promoted."},
+			KindUncategorized:   []string{"Someone somewhere did the world a great justice."},
 		},
 		PreviousRevision: "v1.16.0",
 		CurrentRevision:  "v1.16.1",
@@ -283,7 +285,81 @@ func TestSortKinds(t *testing.T) {
 		"flake":                         nil,
 		"bug":                           nil,
 		"feature":                       nil,
+		"Uncategorized":                 nil,
 	}
 	res := sortKinds(input)
 	require.Equal(t, res, kindPriority)
 }
+
+func TestNoteCollection(t *testing.T) {
+	tests := []struct {
+		name       string
+		collection NoteCollection
+		wantOutput string
+	}{
+		{
+			"render collection with higher priority kind appearing first",
+			NoteCollection{
+				NoteCategory{
+					Kind:  KindDeprecation,
+					Notes: []string{"change 1"},
+				},
+				NoteCategory{
+					Kind:  KindAPIChange,
+					Notes: []string{"change 2"},
+				},
+			},
+			"\n# Deprecation\n  * change 1\n# API Change\n  * change 2",
+		},
+		{
+			"render collection with lower priority kind appearing first",
+			NoteCollection{
+				NoteCategory{
+					Kind:  KindAPIChange,
+					Notes: []string{"change 2"},
+				},
+				NoteCategory{
+					Kind:  KindDeprecation,
+					Notes: []string{"change 1"},
+				},
+			},
+			"\n# Deprecation\n  * change 1\n# API Change\n  * change 2",
+		},
+		{
+			"render with equal priority kinds",
+			NoteCollection{
+				NoteCategory{
+					Kind:  KindDeprecation,
+					Notes: []string{"change 1"},
+				},
+				NoteCategory{
+					Kind:  KindDeprecation,
+					Notes: []string{"change 2"},
+				},
+			},
+			"\n# Deprecation\n  * change 1\n# Deprecation\n  * change 2",
+		},
+	}
+
+	const goTemplate = `
+{{- range . }}
+# {{ .Kind | prettyKind}}
+{{range $note := .Notes}}  * {{$note}}{{end -}}
+{{end -}}
+`
+
+	tmpl, err := template.New("markdown").
+		Funcs(template.FuncMap{"prettyKind": prettyKind}).
+		Parse(goTemplate)
+	require.NoError(t, err, "parsing template")
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			var got strings.Builder
+			require.NoError(t, tt.collection.Sort(kindPriority))
+			require.NoError(t, tmpl.Execute(&got, tt.collection), "rendering template")
+
+			require.Empty(t, diff.Diff(tt.wantOutput, got.String()))
+		})
+	}
+}

pkg/notes/document/testdata/document.md.golden

@@ -64,6 +64,9 @@ filename | sha512 hash
 ### Other (Bug, Cleanup or Flake)
  - This should definitely get you promoted.
 
+### Uncategorized
+ - Someone somewhere did the world a great justice.
+
 ### API Change
  - This might make people sad...or happy.
 

repos.bzl

@@ -2053,3 +2053,11 @@ def go_repositories():
         sum = "h1:GRtyhztX3PNl4lhPhhn+eORpNfrFvygcVCQKgMv8lG8=",
         version = "v0.22.1",
     )
+    go_repository(
+        name = "com_github_kylelemons_godebug",
+        build_file_generation = "on",
+        build_file_proto_mode = "disable",
+        importpath = "github.com/kylelemons/godebug",
+        sum = "h1:RPNrshWIDI6G2gRW9EHilWtl7Z6Sb1BR0xunSBf0SNc=",
+        version = "v1.1.0",
+    )