From 29cadce23722bb6f8d7104131c2af69cfe2fffc8 Mon Sep 17 00:00:00 2001 From: npolshakova Date: Mon, 16 Sep 2024 22:19:26 -0400 Subject: [PATCH 1/2] fix empty release note parsing --- pkg/notes/notes.go | 5 ++- pkg/notes/notes_gatherer_test.go | 8 ++-- pkg/notes/notes_test.go | 68 +++++++++++++++++++++++++------- 3 files changed, 62 insertions(+), 19 deletions(-) diff --git a/pkg/notes/notes.go b/pkg/notes/notes.go index 605b7f5ed08..b1e3731d63d 100644 --- a/pkg/notes/notes.go +++ b/pkg/notes/notes.go @@ -627,13 +627,16 @@ func (l *commitList) List() []*gogithub.RepositoryCommit { // that do NOT contain release notes. Notably, this is all of the variations of // "release note none" that appear in the commit log. var noteExclusionFilters = []*regexp.Regexp{ - // 'none','n/a','na' case insensitive with optional trailing + // 'none','n/a','na' case-insensitive with optional trailing // whitespace, wrapped in ``` with/without release-note identifier // the 'none','n/a','na' can also optionally be wrapped in quotes ' or " regexp.MustCompile("(?i)```release-notes?\\s*('\")?(none|n/a|na)('\")?\\s*```"), // simple '/release-note-none' tag regexp.MustCompile("/release-note-none"), + + // Matches "release-notes" block with no meaningful content (ex. only whitespace, empty, just newlines) + regexp.MustCompile("(?i)```release-notes?\\s*```\\s*"), } // MatchesExcludeFilter returns true if the string matches an excluded release note. diff --git a/pkg/notes/notes_gatherer_test.go b/pkg/notes/notes_gatherer_test.go index 771b2c7b26c..822bd6b1941 100644 --- a/pkg/notes/notes_gatherer_test.go +++ b/pkg/notes/notes_gatherer_test.go @@ -351,9 +351,11 @@ func TestGatherNotes(t *testing.T) { {pullRequest(9, "release-note /release-note-none", "closed")}, // excluded, the exclusion filters take precedence {pullRequest(10, "```release-note\nNAAAAAAAAAA\n```", "closed")}, // included, does not match the N/A filter, but the 'release-note' check {pullRequest(11, "```release-note\nnone something\n```", "closed")}, // included, does not match the N/A filter, but the 'release-note' check - // empty release note block shouldn't be matched + // empty release note block should be excluded {pullRequest(12, "```release-note\n\n```", "closed")}, - {pullRequest(13, "```release-note\n\n```", "open")}, + {pullRequest(13, "```release-note```", "closed")}, + {pullRequest(13, "```release-note ```", "closed")}, + {pullRequest(14, "```release-note\n\n```", "open")}, } var callCount int64 = -1 @@ -369,7 +371,7 @@ func TestGatherNotes(t *testing.T) { resultsChecker: func(t *testing.T, results []*Result) { // there is not much we can check on the Result, as all the fields are // unexported - expectedResultSize := 9 + expectedResultSize := 12 if e, a := expectedResultSize, len(results); e != a { t.Errorf("Expected the result to be of size %d, got %d", e, a) } diff --git a/pkg/notes/notes_test.go b/pkg/notes/notes_test.go index 69c3bb125bc..1ec3774a5c1 100644 --- a/pkg/notes/notes_test.go +++ b/pkg/notes/notes_test.go @@ -328,25 +328,63 @@ func TestMatchesExcludeFilter(t *testing.T) { shouldExclude: false, }, { - input: `@kubernetes/sig-auth-pr-reviews -/milestone v1.19 -/priority important-longterm -/kind cleanup -/kind deprecation - -xref: #81126 -xref: #81152 -xref: https://github.com/kubernetes/website/pull/19630 - + input: `@kubernetes/sig-auth-pr-reviews + /milestone v1.19 + /priority important-longterm + /kind cleanup + /kind deprecation + + xref: #81126 + xref: #81152 + xref: https://github.com/kubernetes/website/pull/19630 + + ` + mdSep + `release-note + Action Required: Support for basic authentication via the --basic-auth-file flag has been removed. Users should migrate to --token-auth-file for similar functionality. + ` + mdSep + ` + + ` + mdSep + `docs + Removed "Static Password File" section from https://kubernetes.io/docs/reference/access-authn-authz/authentication/#static-password-file + https://github.com/kubernetes/website/pull/19630 + ` + mdSep, + shouldExclude: false, + }, + { + input: ` +#### Does this PR introduce a user-facing change? + ` + mdSep + `release-note -Action Required: Support for basic authentication via the --basic-auth-file flag has been removed. Users should migrate to --token-auth-file for similar functionality. + ` + mdSep + ` +#### Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: + + ` + mdSep + `docs -Removed "Static Password File" section from https://kubernetes.io/docs/reference/access-authn-authz/authentication/#static-password-file -https://github.com/kubernetes/website/pull/19630 -` + mdSep, - shouldExclude: false, + +` + mdSep + ` + +`, + shouldExclude: true, }, } { res := MatchesExcludeFilter(tc.input) From d948c1a7245ed72580454eb84dfb3dafad926b83 Mon Sep 17 00:00:00 2001 From: npolshakova Date: Tue, 17 Sep 2024 15:45:29 -0400 Subject: [PATCH 2/2] fix integration test for empty release notes --- pkg/notes/notes.go | 13 ++++++++++--- pkg/notes/notes_gatherer_test.go | 8 ++++---- pkg/notes/notes_test.go | 12 +++++++++++- 3 files changed, 25 insertions(+), 8 deletions(-) diff --git a/pkg/notes/notes.go b/pkg/notes/notes.go index b1e3731d63d..da7ffaa6cac 100644 --- a/pkg/notes/notes.go +++ b/pkg/notes/notes.go @@ -372,6 +372,16 @@ func (g *Gatherer) ListReleaseNotes() (*ReleaseNotes, error) { // may contain the commit message, the PR description, etc. // This is generally the content inside the ```release-note ``` stanza. func noteTextFromString(s string) (string, error) { + // check release note is not empty + // Matches "release-notes" block with no meaningful content (ex. only whitespace, empty, just newlines) + emptyExps := []*regexp.Regexp{ + regexp.MustCompile("(?i)```release-notes?\\s*```\\s*"), + } + + if matchesFilter(s, emptyExps) { + return "", errors.New("empty release note") + } + exps := []*regexp.Regexp{ // (?s) is needed for '.' to be matching on newlines, by default that's disabled // we need to match ungreedy 'U', because after the notes a `docs` block can occur @@ -634,9 +644,6 @@ var noteExclusionFilters = []*regexp.Regexp{ // simple '/release-note-none' tag regexp.MustCompile("/release-note-none"), - - // Matches "release-notes" block with no meaningful content (ex. only whitespace, empty, just newlines) - regexp.MustCompile("(?i)```release-notes?\\s*```\\s*"), } // MatchesExcludeFilter returns true if the string matches an excluded release note. diff --git a/pkg/notes/notes_gatherer_test.go b/pkg/notes/notes_gatherer_test.go index 822bd6b1941..fe5832f5164 100644 --- a/pkg/notes/notes_gatherer_test.go +++ b/pkg/notes/notes_gatherer_test.go @@ -351,11 +351,11 @@ func TestGatherNotes(t *testing.T) { {pullRequest(9, "release-note /release-note-none", "closed")}, // excluded, the exclusion filters take precedence {pullRequest(10, "```release-note\nNAAAAAAAAAA\n```", "closed")}, // included, does not match the N/A filter, but the 'release-note' check {pullRequest(11, "```release-note\nnone something\n```", "closed")}, // included, does not match the N/A filter, but the 'release-note' check - // empty release note block should be excluded + // empty release note block should skipped because noteTextFromString returns an error {pullRequest(12, "```release-note\n\n```", "closed")}, {pullRequest(13, "```release-note```", "closed")}, - {pullRequest(13, "```release-note ```", "closed")}, - {pullRequest(14, "```release-note\n\n```", "open")}, + {pullRequest(14, "```release-note ```", "closed")}, + {pullRequest(15, "```release-note\n\n```", "open")}, } var callCount int64 = -1 @@ -371,7 +371,7 @@ func TestGatherNotes(t *testing.T) { resultsChecker: func(t *testing.T, results []*Result) { // there is not much we can check on the Result, as all the fields are // unexported - expectedResultSize := 12 + expectedResultSize := 9 if e, a := expectedResultSize, len(results); e != a { t.Errorf("Expected the result to be of size %d, got %d", e, a) } diff --git a/pkg/notes/notes_test.go b/pkg/notes/notes_test.go index 1ec3774a5c1..94b06393104 100644 --- a/pkg/notes/notes_test.go +++ b/pkg/notes/notes_test.go @@ -297,6 +297,16 @@ func TestNoteTextFromString(t *testing.T) { require.Equal(t, "item\nitem\n- item\n item", res) }, }, + { + noteBlock( + "", + ), + func(res string, err error) { + require.NotNil(t, err) + require.Contains(t, err.Error(), "empty release note") + require.Equal(t, "", res) + }, + }, } { tc.expect(noteTextFromString(tc.input)) } @@ -384,7 +394,7 @@ Please use the following format for linking documentation: ` + mdSep + ` `, - shouldExclude: true, + shouldExclude: false, // noteTextFromString will catch empty release notes }, } { res := MatchesExcludeFilter(tc.input)