-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added bot command to enforce changelog requirements #159
Merged
Merged
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
7882335
Added bot command to enforce changelog requirements
fheinecke f7ca941
Update bot/internal/bot/changelog.go
fheinecke 45d081b
Removed changelog entry restriction
fheinecke ef4c493
Reject "none" changelog entries to facilitate with migration to tool
fheinecke 4e1a537
Added explicit message that the changelog entry failed validation
fheinecke 481172d
Verified that logged errors are reasonably close to a properly format…
fheinecke File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,124 @@ | ||
| /* | ||
| 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. | ||
| 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 | ||
| } | ||
|
|
||
| changelogEntries, err := b.getChangelogEntries(ctx, pull.UnsafeBody) | ||
| if err != nil { | ||
| return trace.Wrap(err, "failed to get 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) getChangelogEntries(ctx context.Context, prBody string) ([]string, error) { | ||
| changelogRegex := regexp.MustCompile(ChangelogRegex) | ||
|
|
||
| changelogMatches := changelogRegex.FindAllString(prBody, -1) | ||
| if len(changelogMatches) == 0 { | ||
| 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) | ||
| } | ||
|
|
||
| for i, changelogMatch := range changelogMatches { | ||
| changelogMatches[i] = changelogMatch[len(ChangelogPrefix):] // Case insensitive prefix removal | ||
| } | ||
|
|
||
| log.Printf("Found changelog entries %v", changelogMatches) | ||
| return changelogMatches, 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...) | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,216 @@ | ||
| /* | ||
| 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), "some extra text"}, "\n"), | ||
| shouldError: false, | ||
| 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: []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: []string{strings.Repeat(ChangelogPrefix, 4)}, | ||
| }, | ||
| { | ||
| desc: "pass-only-changelog-in-body", | ||
| body: fmt.Sprintf("%schangelog entry", ChangelogPrefix), | ||
| shouldError: false, | ||
| 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", | ||
| body: "", | ||
| shouldError: true, | ||
| }, | ||
| { | ||
| desc: "fail-if-no-entry", | ||
| body: "some typical PR entry", | ||
| shouldError: true, | ||
| }, | ||
| } | ||
| for _, test := range tests { | ||
| t.Run(test.desc, func(t *testing.T) { | ||
| b, ctx := buildTestingFixtures() | ||
|
|
||
| changelogEntries, err := b.getChangelogEntries(ctx, test.body) | ||
| require.Equal(t, test.shouldError, err != nil) | ||
| if !test.shouldError { | ||
| require.Exactly(t, test.expected, changelogEntries) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| 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 .", | ||
| 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: "[email protected]", | ||
| Body: "PR comment body", | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, context.Background() | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make logic more clear, I would make the following changes:
b.logFailedCheck()from each branch here and in "getChangelogEntry", have them return a normaltrace.BadParameter("xxx")error.CheckChangelogcallb.logFailedCheck(ctx, err)when the respective function returns an error.Basically, move
logFailedCheckcall up the stack. Otherwise it's very implicit that get/validate functions also leave comments on the PR.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree here, on the grounds that
validateChangelogEntrycould potentially return an error that is not due to an invalid changelog. Currently this is not an issue, however, if a developer were to add another check that does something like checking to see if the changelog entry is already in the CHANGELOG.md file, then this could result in an error message being written to the PR body that is not due to an invalid changelog entry. More specifically, if a check was added that did something likeerr := os.ReadFile("CHANGELOG.md")then this error could potentially get logged as a changelog entry failure.Maybe one of these would be a better solution to the issue:
validateChangelogEntryfunction that explicitly says that it logs errors to the PRWhat do you think?
If I were to go with the second route, would it be better to return and instance of this new error type, or return a
trace.Wrapof this new error type? In other words,ValidationError("some message")ortrace.Wrap(ValidationError("some message"), "some error occured")?