From 7882335ec2efcf181d155e46bb0fdc7cef377ada Mon Sep 17 00:00:00 2001 From: Fred Heinecke Date: Fri, 1 Sep 2023 17:22:33 -0500 Subject: [PATCH 1/6] Added bot command to enforce changelog requirements --- bot/go.mod | 3 +- bot/go.sum | 6 +- bot/internal/bot/changelog.go | 123 +++++++++++++++++ bot/internal/bot/changelog_test.go | 207 +++++++++++++++++++++++++++++ bot/main.go | 2 + 5 files changed, 338 insertions(+), 3 deletions(-) create mode 100644 bot/internal/bot/changelog.go create mode 100644 bot/internal/bot/changelog_test.go diff --git a/bot/go.mod b/bot/go.mod index f67ccc93..f33ff894 100644 --- a/bot/go.mod +++ b/bot/go.mod @@ -17,8 +17,9 @@ require ( github.com/pmezard/go-difflib v1.0.0 // indirect github.com/sirupsen/logrus v1.9.0 // indirect golang.org/x/crypto v0.6.0 // indirect + golang.org/x/exp v0.0.0-20230905200255-921286631fa9 golang.org/x/net v0.7.0 // indirect - golang.org/x/sys v0.5.0 // indirect + golang.org/x/sys v0.12.0 // indirect golang.org/x/term v0.5.0 // indirect google.golang.org/appengine v1.6.7 // indirect google.golang.org/protobuf v1.28.1 // indirect diff --git a/bot/go.sum b/bot/go.sum index 4b5ff398..226f7340 100644 --- a/bot/go.sum +++ b/bot/go.sum @@ -79,6 +79,8 @@ golang.org/x/crypto v0.0.0-20220126234351-aa10faf2a1f8/go.mod h1:IxCIyHEi3zRg3s0 golang.org/x/crypto v0.6.0 h1:qfktjS5LUO+fFKeJXZ+ikTRijMmljikvG68fpMMruSc= golang.org/x/crypto v0.6.0/go.mod h1:OFC/31mSvZgRz0V1QTNCzfAI1aIRzbiufJtkMIlEp58= golang.org/x/exp v0.0.0-20190121172915-509febef88a4/go.mod h1:CJ0aWSM057203Lf6IL+f9T1iT9GByDxfZKAQTCR3kQA= +golang.org/x/exp v0.0.0-20230905200255-921286631fa9 h1:GoHiUyI/Tp2nVkLI2mCxVkOjsbSXD66ic0XW0js0R9g= +golang.org/x/exp v0.0.0-20230905200255-921286631fa9/go.mod h1:S2oDrQGGwySpoQPVqRShND87VCbxmc6bL1Yd2oYrm6k= golang.org/x/lint v0.0.0-20181026193005-c67002cb31c3/go.mod h1:UVdnD1Gm6xHRNCYTkRU2/jEulfH38KcIWyp/GAMgvoE= golang.org/x/lint v0.0.0-20190227174305-5b3e6a55c961/go.mod h1:wehouNa3lNwaWXcvxsM5YxQ5yQlVC4a0KAMCusXpPoU= golang.org/x/lint v0.0.0-20190313153728-d0100b6bd8b3/go.mod h1:6SW0HCj/g11FgYtHlgUYUwCkIfeOF89ocIRzGO/8vkc= @@ -113,8 +115,8 @@ golang.org/x/sys v0.0.0-20210615035016-665e8c7367d1/go.mod h1:oPkhp1MJrh7nUepCBc golang.org/x/sys v0.0.0-20211216021012-1d35b9e2eb4e/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.0.0-20220114195835-da31bd327af9/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.0.0-20220715151400-c0bba94af5f8/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= -golang.org/x/sys v0.5.0 h1:MUK/U/4lj1t1oPg0HfuXDN/Z1wv31ZJ/YcPiGccS4DU= -golang.org/x/sys v0.5.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= +golang.org/x/sys v0.12.0 h1:CM0HF96J0hcLAwsHPJZjfdNzs0gftsLfgKt57wWHJ0o= +golang.org/x/sys v0.12.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo= golang.org/x/term v0.0.0-20210927222741-03fcf44c2211/go.mod h1:jbD1KX2456YbFQfuXm/mYQcufACuNUgVhRMnK/tPxf8= golang.org/x/term v0.5.0 h1:n2a8QNdAb0sZNpU9R1ALUXBbY+w51fCQDN+7EdxNBsY= diff --git a/bot/internal/bot/changelog.go b/bot/internal/bot/changelog.go new file mode 100644 index 00000000..141a146a --- /dev/null +++ b/bot/internal/bot/changelog.go @@ -0,0 +1,123 @@ +/* +Copyright 2022 Gravitational, Inc. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package bot + +import ( + "context" + "fmt" + "log" + "regexp" + "strings" + "unicode" + + "github.com/gravitational/trace" + "golang.org/x/exp/slices" +) + +const NoChangelogLabel string = "no-changelog" +const ChangelogPrefix string = "changelog: " +const ChangelogRegex string = "(?mi)^changelog: .*" + +// Checks if the PR contains a changelog entry in the PR body, or a "no-changelog" label. +// +// A few tests are performed on the extracted changelog entry to ensure it conforms to a +// common standard. +func (b *Bot) CheckChangelog(ctx context.Context) error { + pull, err := b.c.GitHub.GetPullRequest(ctx, + b.c.Environment.Organization, + b.c.Environment.Repository, + b.c.Environment.Number, + ) + if err != nil { + return trace.Wrap(err, "failed to retrieve pull request for https://github.com/%s/%s/pull/%d", b.c.Environment.Organization, b.c.Environment.Repository, b.c.Environment.Number) + } + + if slices.Contains(pull.UnsafeLabels, NoChangelogLabel) { + log.Printf("PR contains %q label, skipping changelog check", NoChangelogLabel) + return nil + } + + changelogEntry, err := b.getChangelogEntry(ctx, pull.UnsafeBody) + if err != nil { + return trace.Wrap(err, "failed to get changelog entry") + } + + err = b.validateChangelogEntry(ctx, changelogEntry) + if err != nil { + return trace.Wrap(err, "failed to validate changelog entry") + } + + return nil +} + +func (b *Bot) getChangelogEntry(ctx context.Context, prBody string) (string, error) { + changelogRegex := regexp.MustCompile(ChangelogRegex) + + changelogMatches := changelogRegex.FindAllString(prBody, 2) // Two or more changelog entries is invalid, so no need to search for more than two. + if len(changelogMatches) == 0 { + return "", b.logFailedCheck(ctx, "Changelog entry not found in the PR body. Please add a %q label to the PR, or a changelog line starting with `%s` followed by the changelog entry for the PR.", NoChangelogLabel, ChangelogPrefix) + } + + if len(changelogMatches) > 1 { + return "", b.logFailedCheck(ctx, "Found two or more changelog entries in the PR body: %v", changelogMatches) + } + + changelogEntry := changelogMatches[0][len(ChangelogPrefix):] // Case insensitive prefix removal + log.Printf("Found changelog entry %q", changelogEntry) + return changelogEntry, nil +} + +// Checks for common issues with the changelog entry. +// This is not intended to be comprehensive, rather, it is intended to cover the majority of problems. +func (b *Bot) validateChangelogEntry(ctx context.Context, changelogEntry string) error { + if strings.TrimSpace(changelogEntry) == "" { + return b.logFailedCheck(ctx, "The changelog entry must contain one or more non-whitespace characters") + } + + if !unicode.IsLetter([]rune(changelogEntry)[0]) { + return b.logFailedCheck(ctx, "The changelog entry must start with a letter") + } + + if strings.HasPrefix(strings.ToLower(changelogEntry), "backport of") || + strings.HasPrefix(strings.ToLower(changelogEntry), "backports") { + return b.logFailedCheck(ctx, "The changelog entry must contain the actual change, not a reference to the source PR of the backport.") + } + + if strings.Contains(changelogEntry, "](") { + return b.logFailedCheck(ctx, "The changelog entry must not contain a Markdown link or image") + } + + if strings.Contains(changelogEntry, "```") { + return b.logFailedCheck(ctx, "The changelog entry must not contain a multiline code block") + } + + return nil +} + +func (b *Bot) logFailedCheck(ctx context.Context, format string, args ...interface{}) error { + err := b.c.GitHub.CreateComment(ctx, + b.c.Environment.Organization, + b.c.Environment.Repository, + b.c.Environment.Number, + fmt.Sprintf(format, args...), + ) + if err != nil { + return trace.Wrap(err, "failed to create or update the changelog comment") + } + + return trace.Errorf(format, args...) +} diff --git a/bot/internal/bot/changelog_test.go b/bot/internal/bot/changelog_test.go new file mode 100644 index 00000000..dd3e097e --- /dev/null +++ b/bot/internal/bot/changelog_test.go @@ -0,0 +1,207 @@ +/* +Copyright 2021 Gravitational, Inc. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package bot + +import ( + "context" + "fmt" + "strings" + "testing" + + "github.com/gravitational/shared-workflows/bot/internal/env" + "github.com/gravitational/shared-workflows/bot/internal/github" + "github.com/stretchr/testify/require" +) + +func TestChangelog(t *testing.T) { + t.Run("pass-no-changelog-label", func(t *testing.T) { + b, ctx := buildTestingFixtures() + b.c.GitHub.(*fakeGithub).pull.UnsafeLabels = []string{NoChangelogLabel} + + err := b.CheckChangelog(ctx) + require.True(t, err == nil) + }) +} + +func TestGetChangelogEntry(t *testing.T) { + tests := []struct { + desc string + body string + shouldError bool + expected string + }{ + { + desc: "pass-simple", + body: strings.Join([]string{"some typical PR entry", fmt.Sprintf("%schangelog entry", ChangelogPrefix)}, "\n"), + shouldError: false, + expected: "changelog entry", + }, + { + desc: "pass-case-invariant", + body: strings.Join([]string{"some typical PR entry", fmt.Sprintf("%schangelog entry", strings.ToUpper(ChangelogPrefix))}, "\n"), + shouldError: false, + expected: "changelog entry", + }, + { + desc: "pass-prefix-in-changelog-entry", + body: strings.Join([]string{"some typical PR entry", strings.Repeat(ChangelogPrefix, 5)}, "\n"), + shouldError: false, + expected: strings.Repeat(ChangelogPrefix, 4), + }, + { + desc: "pass-only-changelog-in-body", + body: fmt.Sprintf("%schangelog entry", ChangelogPrefix), + shouldError: false, + expected: "changelog entry", + }, + { + desc: "fail-if-no-body", + body: "", + shouldError: true, + }, + { + desc: "fail-if-no-entry", + body: "some typical PR entry", + shouldError: true, + }, + { + desc: "fail-if-multiple-entries", + body: strings.Join([]string{ChangelogPrefix, ChangelogPrefix, ChangelogPrefix}, "\n"), + shouldError: true, + }, + } + for _, test := range tests { + t.Run(test.desc, func(t *testing.T) { + b, ctx := buildTestingFixtures() + + changelogEntry, err := b.getChangelogEntry(ctx, test.body) + require.Equal(t, test.shouldError, err != nil) + if !test.shouldError { + require.Equal(t, test.expected, changelogEntry) + } + }) + } +} + +func TestValidateGetChangelogEntry(t *testing.T) { + tests := []struct { + desc string + entry string + shouldError bool + }{ + { + desc: "pass-simple", + entry: "Changelog entry", + shouldError: false, + }, + { + desc: "pass-markdown-single-line-code-block", + entry: "Changelog `entry`", + shouldError: false, + }, + { + desc: "fail-empty", + entry: "", + shouldError: true, + }, + { + desc: "fail-whitespace", + entry: " \t ", + shouldError: true, + }, + { + desc: "fail-non-alphabetical-starting-character", + entry: "1234Changelog entry", + shouldError: true, + }, + { + desc: "fail-refers-to-backport", + entry: "Backport of #1234", + shouldError: true, + }, + { + desc: "fail-ends-with-markdown-link", + entry: "Changelog [entry](https://some-link.com).", + shouldError: true, + }, + { + desc: "fail-ends-with-markdown-image", + entry: "Changelog ![entry](https://some-link.com).", + shouldError: true, + }, + { + desc: "fail-ends-with-markdown-header", + entry: "## Changelog entry", + shouldError: true, + }, + { + desc: "fail-ends-with-markdown-ordered-list", + entry: "1. Changelog entry", + shouldError: true, + }, + { + desc: "fail-ends-with-markdown-unordered-list", + entry: "- Changelog entry", + shouldError: true, + }, + { + desc: "fail-ends-with-markdown-multiline-code-block", + entry: "Changelog entry ```", + shouldError: true, + }, + } + for _, test := range tests { + t.Run(test.desc, func(t *testing.T) { + b, ctx := buildTestingFixtures() + + err := b.validateChangelogEntry(ctx, test.entry) + require.Equal(t, test.shouldError, err != nil) + }) + } +} + +func TestLogFailedCheck(t *testing.T) { + t.Run("fail-contains-passed-message", func(t *testing.T) { + b, ctx := buildTestingFixtures() + + err := b.logFailedCheck(ctx, "error %s", "message") + require.ErrorContains(t, err, "error message") + }) +} + +func buildTestingFixtures() (*Bot, context.Context) { + return &Bot{ + c: &Config{ + Environment: &env.Environment{ + Organization: "foo", + Author: "9", + Repository: "bar", + Number: 0, + UnsafeBase: "branch/v8", + UnsafeHead: "fix", + }, + GitHub: &fakeGithub{ + comments: []github.Comment{ + { + Author: "foo@bar.com", + Body: "PR comment body", + }, + }, + }, + }, + }, context.Background() +} diff --git a/bot/main.go b/bot/main.go index 6d063282..82776603 100644 --- a/bot/main.go +++ b/bot/main.go @@ -78,6 +78,8 @@ func main() { err = b.CalculateBinarySizes(ctx, flags.buildDir, flags.artifacts, out) case "bloat": err = b.BloatCheck(ctx, flags.baseStats, flags.buildDir, flags.artifacts, os.Stdout) + case "changelog": + err = b.CheckChangelog(ctx) default: err = trace.BadParameter("unknown workflow: %v", flags.workflow) } From f7ca941c2e0f423f3b5ad67d2fac1c7d67f8754c Mon Sep 17 00:00:00 2001 From: fheinecke <23390735+fheinecke@users.noreply.github.com> Date: Mon, 25 Sep 2023 18:30:00 -0500 Subject: [PATCH 2/6] Update bot/internal/bot/changelog.go Co-authored-by: Roman Tkachenko --- bot/internal/bot/changelog.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bot/internal/bot/changelog.go b/bot/internal/bot/changelog.go index 141a146a..602783fa 100644 --- a/bot/internal/bot/changelog.go +++ b/bot/internal/bot/changelog.go @@ -1,5 +1,5 @@ /* -Copyright 2022 Gravitational, Inc. +Copyright 2023 Gravitational, Inc. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. From 45d081b384a1334501a61cb7092aafb8fb68a7a5 Mon Sep 17 00:00:00 2001 From: Fred Heinecke Date: Mon, 25 Sep 2023 18:44:22 -0500 Subject: [PATCH 3/6] Removed changelog entry restriction --- bot/internal/bot/changelog.go | 25 +++++++++++---------- bot/internal/bot/changelog_test.go | 35 +++++++++++++++++++----------- 2 files changed, 35 insertions(+), 25 deletions(-) diff --git a/bot/internal/bot/changelog.go b/bot/internal/bot/changelog.go index 602783fa..445da678 100644 --- a/bot/internal/bot/changelog.go +++ b/bot/internal/bot/changelog.go @@ -51,34 +51,35 @@ func (b *Bot) CheckChangelog(ctx context.Context) error { return nil } - changelogEntry, err := b.getChangelogEntry(ctx, pull.UnsafeBody) + changelogEntries, err := b.getChangelogEntries(ctx, pull.UnsafeBody) if err != nil { return trace.Wrap(err, "failed to get changelog entry") } - err = b.validateChangelogEntry(ctx, changelogEntry) - if err != nil { - return trace.Wrap(err, "failed to validate changelog entry") + for _, changelogEntry := range changelogEntries { + err = b.validateChangelogEntry(ctx, changelogEntry) + if err != nil { + return trace.Wrap(err, "failed to validate changelog entry %q", changelogEntry) + } } return nil } -func (b *Bot) getChangelogEntry(ctx context.Context, prBody string) (string, error) { +func (b *Bot) getChangelogEntries(ctx context.Context, prBody string) ([]string, error) { changelogRegex := regexp.MustCompile(ChangelogRegex) - changelogMatches := changelogRegex.FindAllString(prBody, 2) // Two or more changelog entries is invalid, so no need to search for more than two. + changelogMatches := changelogRegex.FindAllString(prBody, -1) if len(changelogMatches) == 0 { - return "", b.logFailedCheck(ctx, "Changelog entry not found in the PR body. Please add a %q label to the PR, or a changelog line starting with `%s` followed by the changelog entry for the PR.", NoChangelogLabel, ChangelogPrefix) + return nil, b.logFailedCheck(ctx, "Changelog entry not found in the PR body. Please add a %q label to the PR, or changelog lines starting with `%s` followed by the changelog entries for the PR.", NoChangelogLabel, ChangelogPrefix) } - if len(changelogMatches) > 1 { - return "", b.logFailedCheck(ctx, "Found two or more changelog entries in the PR body: %v", changelogMatches) + for i, changelogMatch := range changelogMatches { + changelogMatches[i] = changelogMatch[len(ChangelogPrefix):] // Case insensitive prefix removal } - changelogEntry := changelogMatches[0][len(ChangelogPrefix):] // Case insensitive prefix removal - log.Printf("Found changelog entry %q", changelogEntry) - return changelogEntry, nil + log.Printf("Found changelog entries %v", changelogMatches) + return changelogMatches, nil } // Checks for common issues with the changelog entry. diff --git a/bot/internal/bot/changelog_test.go b/bot/internal/bot/changelog_test.go index dd3e097e..978b0e32 100644 --- a/bot/internal/bot/changelog_test.go +++ b/bot/internal/bot/changelog_test.go @@ -42,31 +42,45 @@ func TestGetChangelogEntry(t *testing.T) { desc string body string shouldError bool - expected string + expected []string }{ { desc: "pass-simple", - body: strings.Join([]string{"some typical PR entry", fmt.Sprintf("%schangelog entry", ChangelogPrefix)}, "\n"), + body: strings.Join([]string{"some typical PR entry", fmt.Sprintf("%schangelog entry", ChangelogPrefix), "some extra text"}, "\n"), shouldError: false, - expected: "changelog entry", + expected: []string{"changelog entry"}, }, { desc: "pass-case-invariant", body: strings.Join([]string{"some typical PR entry", fmt.Sprintf("%schangelog entry", strings.ToUpper(ChangelogPrefix))}, "\n"), shouldError: false, - expected: "changelog entry", + expected: []string{"changelog entry"}, }, { desc: "pass-prefix-in-changelog-entry", body: strings.Join([]string{"some typical PR entry", strings.Repeat(ChangelogPrefix, 5)}, "\n"), shouldError: false, - expected: strings.Repeat(ChangelogPrefix, 4), + expected: []string{strings.Repeat(ChangelogPrefix, 4)}, }, { desc: "pass-only-changelog-in-body", body: fmt.Sprintf("%schangelog entry", ChangelogPrefix), shouldError: false, - expected: "changelog entry", + expected: []string{"changelog entry"}, + }, + { + desc: "pass-multiple-entries", + body: strings.Join([]string{ + ChangelogPrefix + "entry 1", + ChangelogPrefix + "entry 2", + ChangelogPrefix + "entry 3", + }, "\n"), + expected: []string{ + "entry 1", + "entry 2", + "entry 3", + }, + shouldError: false, }, { desc: "fail-if-no-body", @@ -78,20 +92,15 @@ func TestGetChangelogEntry(t *testing.T) { body: "some typical PR entry", shouldError: true, }, - { - desc: "fail-if-multiple-entries", - body: strings.Join([]string{ChangelogPrefix, ChangelogPrefix, ChangelogPrefix}, "\n"), - shouldError: true, - }, } for _, test := range tests { t.Run(test.desc, func(t *testing.T) { b, ctx := buildTestingFixtures() - changelogEntry, err := b.getChangelogEntry(ctx, test.body) + changelogEntries, err := b.getChangelogEntries(ctx, test.body) require.Equal(t, test.shouldError, err != nil) if !test.shouldError { - require.Equal(t, test.expected, changelogEntry) + require.Exactly(t, test.expected, changelogEntries) } }) } From ef4c493d4ef3040f2f95f7b16e1b4f31d562e2fd Mon Sep 17 00:00:00 2001 From: Fred Heinecke Date: Tue, 26 Sep 2023 16:10:11 -0500 Subject: [PATCH 4/6] Reject "none" changelog entries to facilitate with migration to tool --- bot/internal/bot/changelog.go | 11 ++++++++--- bot/internal/bot/changelog_test.go | 5 +++++ 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/bot/internal/bot/changelog.go b/bot/internal/bot/changelog.go index 445da678..f0154bdf 100644 --- a/bot/internal/bot/changelog.go +++ b/bot/internal/bot/changelog.go @@ -85,7 +85,8 @@ func (b *Bot) getChangelogEntries(ctx context.Context, prBody string) ([]string, // Checks for common issues with the changelog entry. // This is not intended to be comprehensive, rather, it is intended to cover the majority of problems. func (b *Bot) validateChangelogEntry(ctx context.Context, changelogEntry string) error { - if strings.TrimSpace(changelogEntry) == "" { + changelogEntry = strings.ToLower(strings.TrimSpace(changelogEntry)) // Format the entry for easy validation + if changelogEntry == "" { return b.logFailedCheck(ctx, "The changelog entry must contain one or more non-whitespace characters") } @@ -93,8 +94,8 @@ func (b *Bot) validateChangelogEntry(ctx context.Context, changelogEntry string) return b.logFailedCheck(ctx, "The changelog entry must start with a letter") } - if strings.HasPrefix(strings.ToLower(changelogEntry), "backport of") || - strings.HasPrefix(strings.ToLower(changelogEntry), "backports") { + if strings.HasPrefix(changelogEntry, "backport of") || + strings.HasPrefix(changelogEntry, "backports") { return b.logFailedCheck(ctx, "The changelog entry must contain the actual change, not a reference to the source PR of the backport.") } @@ -106,6 +107,10 @@ func (b *Bot) validateChangelogEntry(ctx context.Context, changelogEntry string) return b.logFailedCheck(ctx, "The changelog entry must not contain a multiline code block") } + if changelogEntry == "none" { + return b.logFailedCheck(ctx, "The %q label must be set instead of listing 'none' as the changelog entry", NoChangelogLabel) + } + return nil } diff --git a/bot/internal/bot/changelog_test.go b/bot/internal/bot/changelog_test.go index 978b0e32..68c97b92 100644 --- a/bot/internal/bot/changelog_test.go +++ b/bot/internal/bot/changelog_test.go @@ -172,6 +172,11 @@ func TestValidateGetChangelogEntry(t *testing.T) { entry: "Changelog entry ```", shouldError: true, }, + { + desc: "fail-is-set-to-none", + entry: " None ", + shouldError: true, + }, } for _, test := range tests { t.Run(test.desc, func(t *testing.T) { From 4e1a53751b1131e54b1f8e89a9489610e63a3a88 Mon Sep 17 00:00:00 2001 From: Fred Heinecke Date: Tue, 26 Sep 2023 16:13:06 -0500 Subject: [PATCH 5/6] Added explicit message that the changelog entry failed validation --- bot/internal/bot/changelog.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bot/internal/bot/changelog.go b/bot/internal/bot/changelog.go index f0154bdf..3c7e4ae8 100644 --- a/bot/internal/bot/changelog.go +++ b/bot/internal/bot/changelog.go @@ -119,7 +119,7 @@ func (b *Bot) logFailedCheck(ctx context.Context, format string, args ...interfa b.c.Environment.Organization, b.c.Environment.Repository, b.c.Environment.Number, - fmt.Sprintf(format, args...), + fmt.Sprintf("The PR changelog entry failed validation: %s", fmt.Sprintf(format, args...)), ) if err != nil { return trace.Wrap(err, "failed to create or update the changelog comment") From 481172d84cad7eb17d0c72d5f8743e0f2e8c2952 Mon Sep 17 00:00:00 2001 From: Fred Heinecke Date: Tue, 26 Sep 2023 16:30:31 -0500 Subject: [PATCH 6/6] Verified that logged errors are reasonably close to a properly formatted sentence --- bot/internal/bot/changelog.go | 10 +++++----- bot/internal/bot/changelog_test.go | 12 +++++++++++- 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/bot/internal/bot/changelog.go b/bot/internal/bot/changelog.go index 3c7e4ae8..06236a28 100644 --- a/bot/internal/bot/changelog.go +++ b/bot/internal/bot/changelog.go @@ -87,11 +87,11 @@ func (b *Bot) getChangelogEntries(ctx context.Context, prBody string) ([]string, func (b *Bot) validateChangelogEntry(ctx context.Context, changelogEntry string) error { changelogEntry = strings.ToLower(strings.TrimSpace(changelogEntry)) // Format the entry for easy validation if changelogEntry == "" { - return b.logFailedCheck(ctx, "The changelog entry must contain one or more non-whitespace characters") + return b.logFailedCheck(ctx, "The changelog entry must contain one or more non-whitespace characters.") } if !unicode.IsLetter([]rune(changelogEntry)[0]) { - return b.logFailedCheck(ctx, "The changelog entry must start with a letter") + return b.logFailedCheck(ctx, "The changelog entry must start with a letter.") } if strings.HasPrefix(changelogEntry, "backport of") || @@ -100,15 +100,15 @@ func (b *Bot) validateChangelogEntry(ctx context.Context, changelogEntry string) } if strings.Contains(changelogEntry, "](") { - return b.logFailedCheck(ctx, "The changelog entry must not contain a Markdown link or image") + return b.logFailedCheck(ctx, "The changelog entry must not contain a Markdown link or image.") } if strings.Contains(changelogEntry, "```") { - return b.logFailedCheck(ctx, "The changelog entry must not contain a multiline code block") + return b.logFailedCheck(ctx, "The changelog entry must not contain a multiline code block.") } if changelogEntry == "none" { - return b.logFailedCheck(ctx, "The %q label must be set instead of listing 'none' as the changelog entry", NoChangelogLabel) + return b.logFailedCheck(ctx, "The %q label must be set instead of listing 'none' as the changelog entry.", NoChangelogLabel) } return nil diff --git a/bot/internal/bot/changelog_test.go b/bot/internal/bot/changelog_test.go index 68c97b92..aaa50f67 100644 --- a/bot/internal/bot/changelog_test.go +++ b/bot/internal/bot/changelog_test.go @@ -21,6 +21,7 @@ import ( "fmt" "strings" "testing" + "unicode" "github.com/gravitational/shared-workflows/bot/internal/env" "github.com/gravitational/shared-workflows/bot/internal/github" @@ -183,7 +184,16 @@ func TestValidateGetChangelogEntry(t *testing.T) { b, ctx := buildTestingFixtures() err := b.validateChangelogEntry(ctx, test.entry) - require.Equal(t, test.shouldError, err != nil) + if !test.shouldError { + require.NoError(t, err, "the test should not have errored but did") + return + } + + require.Error(t, err, "the test should have errored but did not") + errorMessage := err.Error() + require.NotEmpty(t, strings.TrimSpace(errorMessage), "the error message was empty or whitespace") + require.True(t, strings.HasSuffix(errorMessage, "."), "the error message did not end with a \".\"") + require.True(t, unicode.IsUpper(rune(errorMessage[0])), "the error message did not start with an upper case letter") }) } }