From 226c674370f03d5bf8909641cf71f8dd7e3de8e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20Mudrini=C4=87?= Date: Wed, 28 Sep 2022 17:21:47 +0200 Subject: [PATCH] Ensure release-notes tool doesn't skip automated cherry picks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Marko Mudrinić --- pkg/notes/notes.go | 38 ++++++++++++++++++++------------ pkg/notes/notes_gatherer_test.go | 29 +++++++++++------------- 2 files changed, 37 insertions(+), 30 deletions(-) diff --git a/pkg/notes/notes.go b/pkg/notes/notes.go index d87edd4c7b3..f16466af6fb 100644 --- a/pkg/notes/notes.go +++ b/pkg/notes/notes.go @@ -619,29 +619,17 @@ var noteExclusionFilters = []*regexp.Regexp{ // '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*```"), + regexp.MustCompile("(?i)```release-notes?\\s*('\")?(none|n/a|na)('\")?\\s*```"), // simple '/release-note-none' tag regexp.MustCompile("/release-note-none"), } -// Similarly, now that the known not-release-notes are filtered out, we can -// use some patterns to find actual release notes. -var noteInclusionFilters = []*regexp.Regexp{ - regexp.MustCompile("release-note"), - regexp.MustCompile("Does this PR introduce a user-facing change?"), -} - // MatchesExcludeFilter returns true if the string matches an excluded release note func MatchesExcludeFilter(msg string) bool { return matchesFilter(msg, noteExclusionFilters) } -// MatchesIncludeFilter returns true if the string matches an included release note -func MatchesIncludeFilter(msg string) bool { - return matchesFilter(msg, noteInclusionFilters) -} - func matchesFilter(msg string, filters []*regexp.Regexp) bool { for _, filter := range filters { if filter.MatchString(msg) { @@ -732,12 +720,34 @@ func (g *Gatherer) notesForCommit(commit *gogithub.RepositoryCommit) (*Result, e "Got PR #%d for commit: %s", pr.GetNumber(), commit.GetSHA(), ) - if MatchesIncludeFilter(prBody) { + // If we match exclusion filter (release-note-none), we don't look further, + // instead we return that PR. We return that PR because it might have a map, + // which is checked after this function returns + if MatchesExcludeFilter(prBody) { + res := &Result{commit: commit, pullRequest: pr} + logrus.Infof("PR #%d contains exclusion (release-note-none)", pr.GetNumber()) + + return res, nil + } + + // If we didn't match the exclusion filter, try to extract the release note from the PR. + // If we can't extract the release note, consider that the PR is invalid and take the next one + s, err := noteTextFromString(prBody) + if err != nil { + logrus.Infof("PR #%d does not seem to contain a valid release note, skipping", pr.GetNumber()) + + continue + } + + // If we found a valid release note, return the PR, otherwise, take the next one + if len(s) > 0 { res := &Result{commit: commit, pullRequest: pr} logrus.Infof("PR #%d seems to contain a release note", pr.GetNumber()) // Do not test further PRs for this commit as soon as one PR matched return res, nil } + + logrus.Infof("PR #%d does not seem to contain a valid release note, skipping", pr.GetNumber()) } return nil, nil diff --git a/pkg/notes/notes_gatherer_test.go b/pkg/notes/notes_gatherer_test.go index 5596524e7c5..9fc7fb9bcb1 100644 --- a/pkg/notes/notes_gatherer_test.go +++ b/pkg/notes/notes_gatherer_test.go @@ -335,26 +335,23 @@ func TestGatherNotes(t *testing.T) { listPullRequestsWithCommitStubber: func(t *testing.T) listPullRequestsWithCommitStub { prsPerCall := [][]*github.PullRequest{ // explicitly excluded - {pullRequest(1, "something ```release-note N/a``` something")}, - {pullRequest(2, "something ```release-note Na``` something")}, - {pullRequest(3, "something ```release-note None ``` something")}, - {pullRequest(4, "something ```release-note 'None' ``` something")}, + {pullRequest(1, "something ```release-note\nN/a\n``` something")}, + {pullRequest(2, "something ```release-note\nNa\n``` something")}, + {pullRequest(3, "something ```release-note\nNone \n``` something")}, + {pullRequest(4, "something ```release-note\n'None' \n``` something")}, {pullRequest(5, "something /release-note-none something")}, - // explicitly included - {pullRequest(6, "something release-note something")}, - {pullRequest(7, "something Does this PR introduce a user-facing change something")}, // multiple PRs { // first does no match, second one matches, rest is ignored - pullRequest(8, ""), - pullRequest(9, " Does this PR introduce a user-facing change"), - pullRequest(10, "does-not-matter--is-not-considered"), + pullRequest(6, ""), + pullRequest(7, " something ```release-note\nTest\n``` something"), + pullRequest(8, "does-not-matter--is-not-considered"), }, // some other strange things - {pullRequest(11, "Does this PR introduce a user-facing chang")}, // matches despite the missing 'e' - {pullRequest(12, "release-note /release-note-none")}, // excluded, the exclusion filters take precedence - {pullRequest(13, "```release-note NAAAAAAAAAA```")}, // included, does not match the N/A filter, but the 'release-note' check - {pullRequest(14, "```release-note none something ```")}, // included, does not match the N/A filter, but the 'release-note' check - {pullRequest(15, "release-noteNOOOO")}, // included + {pullRequest(9, "release-note /release-note-none")}, // excluded, the exclusion filters take precedence + {pullRequest(10, "```release-note\nNAAAAAAAAAA\n```")}, // included, does not match the N/A filter, but the 'release-note' check + {pullRequest(11, "```release-note\nnone something\n```")}, // included, does not match the N/A filter, but the 'release-note' check + // empty release note block shouldn't be matched + {pullRequest(12, "```release-note\n\n```")}, } var callCount int64 = -1 @@ -370,7 +367,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 := 13 + expectedResultSize := 9 if e, a := expectedResultSize, len(results); e != a { t.Errorf("Expected the result to be of size %d, got %d", e, a) }