Added bot command to enforce changelog requirements #159
Conversation
There was a problem hiding this comment.
@fheinecke Overall I like this, but left a few simplification suggestions.
fheinecke
force-pushed
the
fred/enforce-changelog-check-1
branch
from
1d7ef8d to
a91854f
Compare
fheinecke
force-pushed
the
fred/enforce-changelog-check-1
branch
from
a91854f to
7882335
Compare
bot/internal/bot/changelog.go
Outdated
| // 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") |
There was a problem hiding this comment.
To make logic more clear, I would make the following changes:
- Instead of returning
b.logFailedCheck()from each branch here and in "getChangelogEntry", have them return a normaltrace.BadParameter("xxx")error. - In
CheckChangelogcallb.logFailedCheck(ctx, err)when the respective function returns an error.
Basically, move logFailedCheck call 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.
I disagree here, on the grounds that validateChangelogEntry could 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 like err := 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:
- I could add a comment to the
validateChangelogEntryfunction that explicitly says that it logs errors to the PR - I could return an error type that is specific to a changelog entry problem, and only log to the PR if the error is of this type
What 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.Wrap of this new error type? In other words, ValidationError("some message") or trace.Wrap(ValidationError("some message"), "some error occured")?
Co-authored-by: Roman Tkachenko <[email protected]>
This allows for repos using the shared-workflows bot to enforce changelog requirements.