Skip to content

Commit

Permalink
Addressed PR comments round 1
Browse files Browse the repository at this point in the history
  • Loading branch information
@fheinecke
fheinecke committed Sep 11, 2023
1 parent 63de94e commit 1d7ef8d
Show file tree
Hide file tree
Showing 6 changed files with 28 additions and 59 deletions.
12 changes: 6 additions & 6 deletions bot/internal/bot/bot.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,16 +62,16 @@ type Client interface {
// CreateComment will leave a comment on an Issue or Pull Request.
CreateComment(ctx context.Context, organization string, repository string, number int, comment string) error

// ListStatefulComments finds comments on a given PR matching the provided tag
ListStatefulComments(ctx context.Context, organization string, repository string, number int, tag string) ([]github.Comment, error)
// ListCommentsByTag finds comments on a given PR matching the provided tag
ListCommentsByTag(ctx context.Context, organization string, repository string, number int, tag string) ([]github.Comment, error)

// CreateOrUpdateStatefulComment will create a comment on a Pull Request, or update a pre-existing comment if one is found with a matching tag.
// CreateOrUpdateCommentByTag will create a comment on a Pull Request, or update a pre-existing comment if one is found with a matching tag.
// Returns the number of created or updated comments.
CreateOrUpdateStatefulComment(ctx context.Context, organization string, repository string, number int, comment string, tag string) (int, error)
CreateOrUpdateCommentByTag(ctx context.Context, organization string, repository string, number int, comment string, tag string) (int, error)

// DeleteStatefulComment will delete all comments on a Pull Request found with a matching tag.
// DeleteCommentByTag will delete all comments on a Pull Request found with a matching tag.
// Returns the number of deleted comments.
DeleteStatefulComment(ctx context.Context, organization string, repository string, number int, tag string) (int, error)
DeleteCommentByTag(ctx context.Context, organization string, repository string, number int, tag string) (int, error)

// ListComments will list all comments on an Issue or Pull Request.
ListComments(ctx context.Context, organization string, repository string, number int) ([]github.Comment, error)
Expand Down
6 changes: 3 additions & 3 deletions bot/internal/bot/bot_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -376,15 +376,15 @@ func (f *fakeGithub) IsOrgMember(ctx context.Context, user string, org string) (
return member, nil
}

func (f *fakeGithub) ListStatefulComments(ctx context.Context, organization string, repository string, number int, tag string) ([]github.Comment, error) {
func (f *fakeGithub) ListCommentsByTag(ctx context.Context, organization string, repository string, number int, tag string) ([]github.Comment, error) {
return f.comments, nil
}

func (f *fakeGithub) CreateOrUpdateStatefulComment(ctx context.Context, organization string, repository string, number int, comment string, tag string) (int, error) {
func (f *fakeGithub) CreateOrUpdateCommentByTag(ctx context.Context, organization string, repository string, number int, comment string, tag string) (int, error) {
return len(f.comments), nil
}

func (f *fakeGithub) DeleteStatefulComment(ctx context.Context, organization string, repository string, number int, tag string) (int, error) {
func (f *fakeGithub) DeleteCommentByTag(ctx context.Context, organization string, repository string, number int, tag string) (int, error) {
return len(f.comments), nil
}

Expand Down
29 changes: 9 additions & 20 deletions bot/internal/bot/changelog.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,11 @@ const NoChangelogLabel string = "no-changelog"
const ChangelogPrefix string = "changelog: "
const ChangelogTag string = "changelog"

func (b *Bot) Changelog(ctx context.Context) error {
// 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,
Expand All @@ -43,7 +47,7 @@ func (b *Bot) Changelog(ctx context.Context) error {
}

if slices.Contains(pull.UnsafeLabels, NoChangelogLabel) {
log.Printf("PR contains no changelog label %q, skipping changelog check", NoChangelogLabel)
log.Printf("PR contains %q label, skipping changelog check", NoChangelogLabel)
return nil
}

Expand All @@ -67,10 +71,7 @@ func (b *Bot) Changelog(ctx context.Context) error {

func (b *Bot) getChangelogEntry(ctx context.Context, prBody string) (string, error) {
changelogRegexString := fmt.Sprintf("(?mi)^%s.*", regexp.QuoteMeta(ChangelogPrefix))
changelogRegex, err := regexp.Compile(changelogRegexString)
if err != nil {
return "", trace.Wrap(err, "failed to compile changelog prefix regex %q", changelogRegexString)
}
changelogRegex := regexp.MustCompile(changelogRegexString)

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 {
Expand All @@ -97,23 +98,11 @@ func (b *Bot) validateChangelogEntry(ctx context.Context, changelogEntry string)
return b.logFailedCheck(ctx, "The changelog entry must start with a letter")
}

if unicode.IsLower([]rune(changelogEntry)[0]) {
return b.logFailedCheck(ctx, "The changelog entry must start with an uppercase character")
}

if strings.TrimSpace(changelogEntry) != changelogEntry {
return b.logFailedCheck(ctx, "The changelog entry must not contain leading or trailing whitespace")
}

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 unicode.IsPunct([]rune(changelogEntry)[len(changelogEntry)-1:][0]) {
return b.logFailedCheck(ctx, "The changelog entry must not end with punctuation.")
}

if strings.Contains(changelogEntry, "](") {
return b.logFailedCheck(ctx, "The changelog entry must not contain a Markdown link or image")
}
Expand All @@ -126,7 +115,7 @@ func (b *Bot) validateChangelogEntry(ctx context.Context, changelogEntry string)
}

func (b *Bot) logFailedCheck(ctx context.Context, format string, args ...interface{}) error {
createOrUpdateCount, err := b.c.GitHub.CreateOrUpdateStatefulComment(ctx,
createOrUpdateCount, err := b.c.GitHub.CreateOrUpdateCommentByTag(ctx,
b.c.Environment.Organization,
b.c.Environment.Repository,
b.c.Environment.Number,
Expand All @@ -144,7 +133,7 @@ func (b *Bot) logFailedCheck(ctx context.Context, format string, args ...interfa
}

func (b *Bot) deleteComments(ctx context.Context) error {
_, err := b.c.GitHub.DeleteStatefulComment(ctx,
_, err := b.c.GitHub.DeleteCommentByTag(ctx,
b.c.Environment.Organization,
b.c.Environment.Repository,
b.c.Environment.Number,
Expand Down
22 changes: 1 addition & 21 deletions bot/internal/bot/changelog_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ func TestChangelog(t *testing.T) {
b, ctx := buildTestingFixtures()
b.c.GitHub.(*fakeGithub).pull.UnsafeLabels = []string{NoChangelogLabel}

err := b.Changelog(ctx)
err := b.CheckChangelog(ctx)
require.True(t, err == nil)
})
}
Expand Down Expand Up @@ -128,31 +128,11 @@ func TestValidateGetChangelogEntry(t *testing.T) {
entry: "1234Changelog entry",
shouldError: true,
},
{
desc: "fail-lowercase-starting-character",
entry: "changelog entry",
shouldError: true,
},
{
desc: "fail-trailing-whitespace",
entry: "Changelog entry ",
shouldError: true,
},
{
desc: "fail-leading-whitespace",
entry: " Changelog entry",
shouldError: true,
},
{
desc: "fail-refers-to-backport",
entry: "Backport of #1234",
shouldError: true,
},
{
desc: "fail-ends-with-punctuation",
entry: "Changelog entry.",
shouldError: true,
},
{
desc: "fail-ends-with-markdown-link",
entry: "Changelog [entry](https://some-link.com).",
Expand Down
16 changes: 8 additions & 8 deletions bot/internal/github/github.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -604,8 +604,8 @@ func (c *Client) createStateTagLine(tag string) string {
return fmt.Sprintf("state-tag: %s", tag)
}

// ListStatefulComments finds comments on a given PR matching the provided tag
func (c *Client) ListStatefulComments(ctx context.Context, organization string, repository string, number int, tag string) ([]Comment, error) {
// ListCommentsByTag finds comments on a given PR matching the provided tag
func (c *Client) ListCommentsByTag(ctx context.Context, organization string, repository string, number int, tag string) ([]Comment, error) {
comments, err := c.ListComments(ctx, organization, repository, number)
if err != nil {
return nil, trace.Wrap(err, "failed to get pull request for https://github.com/%s/%s/pull/%d", organization, repository, number)
Expand Down Expand Up @@ -633,10 +633,10 @@ func (c *Client) ListStatefulComments(ctx context.Context, organization string,
return matchedComments, nil
}

// CreateOrUpdateStatefulComment will create a comment on a Pull Request, or update a pre-existing comment if one is found with a matching tag.
// CreateOrUpdateCommentByTag will create a comment on a Pull Request, or update a pre-existing comment if one is found with a matching tag.
// Returns the number of created or updated comments.
func (c *Client) CreateOrUpdateStatefulComment(ctx context.Context, organization string, repository string, number int, comment string, tag string) (int, error) {
matchedComments, err := c.ListStatefulComments(ctx, organization, repository, number, tag)
func (c *Client) CreateOrUpdateCommentByTag(ctx context.Context, organization string, repository string, number int, comment string, tag string) (int, error) {
matchedComments, err := c.ListCommentsByTag(ctx, organization, repository, number, tag)
if err != nil {
return 0, trace.Wrap(err, "failed to get matching comments for tag %q", tag)
}
Expand Down Expand Up @@ -666,10 +666,10 @@ func (c *Client) CreateOrUpdateStatefulComment(ctx context.Context, organization
return 1, nil
}

// DeleteStatefulComment will delete all comments on a Pull Request found with a matching tag.
// DeleteCommentByTag will delete all comments on a Pull Request found with a matching tag.
// Returns the number of deleted comments.
func (c *Client) DeleteStatefulComment(ctx context.Context, organization string, repository string, number int, tag string) (int, error) {
matchedComments, err := c.ListStatefulComments(ctx, organization, repository, number, tag)
func (c *Client) DeleteCommentByTag(ctx context.Context, organization string, repository string, number int, tag string) (int, error) {
matchedComments, err := c.ListCommentsByTag(ctx, organization, repository, number, tag)
if err != nil {
return 0, trace.Wrap(err, "failed to get matching comments for tag %q", tag)
}
Expand Down
2 changes: 1 addition & 1 deletion bot/main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ func main() {
case "bloat":
err = b.BloatCheck(ctx, flags.baseStats, flags.buildDir, flags.artifacts, os.Stdout)
case "changelog":
err = b.Changelog(ctx)
err = b.CheckChangelog(ctx)
default:
err = trace.BadParameter("unknown workflow: %v", flags.workflow)
}
Expand Down

0 comments on commit 1d7ef8d

Please sign in to comment.