From 07a58003c36efd8d08ed48815d4a740e150304d4 Mon Sep 17 00:00:00 2001 From: Anton Miniailo Date: Wed, 6 Dec 2023 13:23:02 -0500 Subject: [PATCH] Automatically copy changelog message to the backport or put no-changelog label --- bot/internal/bot/backport.go | 29 ++++++-- bot/internal/bot/backport_test.go | 113 +++++++++++++++++++++++++++++ bot/internal/bot/bot.go | 9 ++- bot/internal/bot/bot_test.go | 11 ++- bot/internal/bot/changelog.go | 20 ++--- bot/internal/bot/changelog_test.go | 60 +++++++-------- 6 files changed, 187 insertions(+), 55 deletions(-) diff --git a/bot/internal/bot/backport.go b/bot/internal/bot/backport.go index 3cdfcba9..0bfe68ba 100644 --- a/bot/internal/bot/backport.go +++ b/bot/internal/bot/backport.go @@ -30,9 +30,10 @@ import ( "strings" "text/template" - "github.com/gravitational/shared-workflows/bot/internal/github" - "github.com/gravitational/trace" + "golang.org/x/exp/slices" + + "github.com/gravitational/shared-workflows/bot/internal/github" ) // Backport will create backport Pull Requests (if requested) when a Pull @@ -79,6 +80,11 @@ func (b *Bot) Backport(ctx context.Context) error { var rows []row + g := git + if b.c.Git != nil { + g = b.c.Git + } + // Loop over all requested backport branches and create backport branch and // GitHub Pull Request. for _, base := range branches { @@ -92,7 +98,7 @@ func (b *Bot) Backport(ctx context.Context) error { base, pull, head, - git, + g, ) if err != nil { log.Printf("Failed to create backport branch:\n%v\n", trace.DebugReport(err)) @@ -104,6 +110,16 @@ func (b *Bot) Backport(ctx context.Context) error { continue } + labels := []string{NoChangelogLabel} + bodyText := fmt.Sprintf("Backport #%v to %v", b.c.Environment.Number, base) + if entries := b.getChangelogEntries(pull.UnsafeBody); len(entries) > 0 && !slices.Contains(pull.UnsafeLabels, NoChangelogLabel) { + bodyText += "\n\n" + labels = labels[:0] + for _, entry := range entries { + bodyText += fmt.Sprintf("%s%s\n", ChangelogPrefix, entry) + } + } + rows = append(rows, row{ Branch: base, Failed: false, @@ -117,7 +133,8 @@ func (b *Bot) Backport(ctx context.Context) error { RawQuery: url.Values{ "expand": []string{"1"}, "title": []string{fmt.Sprintf("[%v] %v", strings.Trim(base, "branch/"), pull.UnsafeTitle)}, - "body": []string{fmt.Sprintf("Backport #%v to %v", b.c.Environment.Number, base)}, + "body": []string{bodyText}, + "labels": labels, }.Encode(), }, }) @@ -162,8 +179,10 @@ func (b *Bot) BackportLocal(ctx context.Context, branch string) error { return nil } +const botBackportBranchPrefix = "bot/backport" + func (b *Bot) backportBranchName(base string) string { - return fmt.Sprintf("bot/backport-%v-%v", b.c.Environment.Number, base) + return fmt.Sprintf("%s-%v-%v", botBackportBranchPrefix, b.c.Environment.Number, base) } // findBranches looks through the labels attached to a Pull Request for all the diff --git a/bot/internal/bot/backport_test.go b/bot/internal/bot/backport_test.go index 74ef541d..0cdf0e08 100644 --- a/bot/internal/bot/backport_test.go +++ b/bot/internal/bot/backport_test.go @@ -17,9 +17,14 @@ limitations under the License. package bot import ( + "context" "testing" "github.com/stretchr/testify/require" + + "github.com/gravitational/shared-workflows/bot/internal/env" + "github.com/gravitational/shared-workflows/bot/internal/github" + "github.com/gravitational/shared-workflows/bot/internal/review" ) func TestFindBranches(t *testing.T) { @@ -35,3 +40,111 @@ func TestFindBranches(t *testing.T) { "master", }) } + +func TestBackport(t *testing.T) { + buildTestBot := func(github Client) (*Bot, context.Context) { + r, _ := review.New(&review.Config{ + CodeReviewers: map[string]review.Reviewer{"dev": review.Reviewer{ + Team: "core", + }}, + CodeReviewersOmit: map[string]bool{}, + DocsReviewers: map[string]review.Reviewer{}, + DocsReviewersOmit: map[string]bool{}, + Admins: []string{}, + }) + + return &Bot{ + c: &Config{ + Environment: &env.Environment{ + Organization: "foo", + Author: "dev", + Repository: "bar", + Number: 42, + UnsafeBase: "branch/v8", + UnsafeHead: "fix", + }, + GitHub: github, + Review: r, + Git: gitDryRun, + }, + }, context.Background() + } + + tests := []struct { + desc string + github Client + assertFunc require.ValueAssertionFunc + }{ + { + desc: "pr without backport labels", + github: &fakeGithub{}, + assertFunc: require.Empty, + }, + { + desc: "pr with backport label, no changelog", + github: &fakeGithub{ + pull: github.PullRequest{ + Author: "dev", + Repository: "Teleport", + Number: 42, + UnsafeTitle: "Best PR", + UnsafeBody: "This is PR body", + UnsafeLabels: []string{"backport/branch/v7"}, + }, + jobs: []github.Job{{Name: "Job1", ID: 1}}, + }, + assertFunc: func(t require.TestingT, i interface{}, i2 ...interface{}) { + comments, ok := i.([]github.Comment) + require.True(t, ok) + require.Len(t, comments, 1) + require.Equal(t, + ` +@dev See the table below for backport results. + +| Branch | Result | +|--------|--------| +| branch/v7 | [Create PR](https://github.com/foo/bar/compare/branch/v7...bot/backport-42-branch/v7?body=Backport+%2342+to+branch%2Fv7&expand=1&labels=no-changelog&title=%5Bv7%5D+Best+PR) | +`, comments[0].Body) + }, + }, + { + desc: "pr with backport label and with changelog", + github: &fakeGithub{ + pull: github.PullRequest{ + Author: "dev", + Repository: "Teleport", + Number: 42, + UnsafeTitle: "Best PR", + UnsafeBody: "This is PR body\n\nchangelog: important change", + UnsafeLabels: []string{"backport/branch/v7"}, + }, + jobs: []github.Job{{Name: "Job1", ID: 1}}, + }, + assertFunc: func(t require.TestingT, i interface{}, i2 ...interface{}) { + comments, ok := i.([]github.Comment) + require.True(t, ok) + require.Len(t, comments, 1) + require.Equal(t, + ` +@dev See the table below for backport results. + +| Branch | Result | +|--------|--------| +| branch/v7 | [Create PR](https://github.com/foo/bar/compare/branch/v7...bot/backport-42-branch/v7?body=Backport+%2342+to+branch%2Fv7%0A%0Achangelog%3A+important+change%0A&expand=1&title=%5Bv7%5D+Best+PR) | +`, comments[0].Body) + }, + }, + } + + for _, test := range tests { + t.Run(test.desc, func(t *testing.T) { + b, ctx := buildTestBot(test.github) + + err := b.Backport(ctx) + require.NoError(t, err) + + comments, _ := b.c.GitHub.ListComments(nil, "", "", 0) + test.assertFunc(t, comments) + }) + } +} diff --git a/bot/internal/bot/bot.go b/bot/internal/bot/bot.go index b0d02559..285879fc 100644 --- a/bot/internal/bot/bot.go +++ b/bot/internal/bot/bot.go @@ -20,11 +20,11 @@ import ( "context" "strings" + "github.com/gravitational/trace" + "github.com/gravitational/shared-workflows/bot/internal/env" "github.com/gravitational/shared-workflows/bot/internal/github" "github.com/gravitational/shared-workflows/bot/internal/review" - - "github.com/gravitational/trace" ) // Client implements the GitHub API. @@ -44,7 +44,7 @@ type Client interface { // GetPullRequest returns a specific Pull Request. GetPullRequest(ctx context.Context, organization string, repository string, number int) (github.PullRequest, error) - // GetPullRequestWithCommitsd returns a specific Pull Request with commits. + // GetPullRequestWithCommits returns a specific Pull Request with commits. GetPullRequestWithCommits(ctx context.Context, organization string, repository string, number int) (github.PullRequest, error) // CreatePullRequest will create a Pull Request. @@ -97,6 +97,9 @@ type Config struct { // Review is used to get code and docs reviewers. Review *review.Assignments + + // Git is used to run git commands, uses dry run in tests. + Git func(...string) error } // CheckAndSetDefaults checks and sets defaults. diff --git a/bot/internal/bot/bot_test.go b/bot/internal/bot/bot_test.go index 33445fe4..e5de331d 100644 --- a/bot/internal/bot/bot_test.go +++ b/bot/internal/bot/bot_test.go @@ -20,11 +20,11 @@ import ( "context" "testing" + "github.com/stretchr/testify/require" + "github.com/gravitational/shared-workflows/bot/internal/env" "github.com/gravitational/shared-workflows/bot/internal/github" "github.com/gravitational/shared-workflows/bot/internal/review" - - "github.com/stretchr/testify/require" ) // TestClassifyChanges checks that PR contents are correctly parsed for docs and @@ -311,6 +311,7 @@ func TestDoNotMerge(t *testing.T) { type fakeGithub struct { files []github.PullRequestFile pull github.PullRequest + jobs []github.Job reviewers []string reviews []github.Review orgMembers map[string]struct{} @@ -364,7 +365,7 @@ func (f *fakeGithub) ListWorkflowRuns(ctx context.Context, organization string, } func (f *fakeGithub) ListWorkflowJobs(ctx context.Context, organization string, repository string, runID int64) ([]github.Job, error) { - return nil, nil + return f.jobs, nil } func (f *fakeGithub) DeleteWorkflowRun(ctx context.Context, organization string, repository string, runID int64) error { @@ -377,6 +378,10 @@ func (f *fakeGithub) IsOrgMember(ctx context.Context, user string, org string) ( } func (f *fakeGithub) CreateComment(ctx context.Context, organization string, repository string, number int, comment string) error { + f.comments = append(f.comments, github.Comment{ + Body: comment, + }) + return nil } diff --git a/bot/internal/bot/changelog.go b/bot/internal/bot/changelog.go index 06236a28..2dee7441 100644 --- a/bot/internal/bot/changelog.go +++ b/bot/internal/bot/changelog.go @@ -51,9 +51,11 @@ func (b *Bot) CheckChangelog(ctx context.Context) error { return nil } - changelogEntries, err := b.getChangelogEntries(ctx, pull.UnsafeBody) - if err != nil { - return trace.Wrap(err, "failed to get changelog entry") + changelogEntries := b.getChangelogEntries(pull.UnsafeBody) + if len(changelogEntries) == 0 { + return trace.Wrap( + 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), + "failed to get changelog entry") } for _, changelogEntry := range changelogEntries { @@ -66,20 +68,18 @@ func (b *Bot) CheckChangelog(ctx context.Context) error { return nil } -func (b *Bot) getChangelogEntries(ctx context.Context, prBody string) ([]string, error) { +func (b *Bot) getChangelogEntries(prBody string) []string { 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 + if len(changelogMatches) > 0 { + log.Printf("Found changelog entries %v", changelogMatches) + } + return changelogMatches } // 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 aaa50f67..1a0a001a 100644 --- a/bot/internal/bot/changelog_test.go +++ b/bot/internal/bot/changelog_test.go @@ -23,9 +23,10 @@ import ( "testing" "unicode" + "github.com/stretchr/testify/require" + "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) { @@ -40,34 +41,29 @@ func TestChangelog(t *testing.T) { func TestGetChangelogEntry(t *testing.T) { tests := []struct { - desc string - body string - shouldError bool - expected []string + desc string + body string + 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-simple", + body: strings.Join([]string{"some typical PR entry", fmt.Sprintf("%schangelog entry", ChangelogPrefix), "some extra text"}, "\n"), + 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-case-invariant", + body: strings.Join([]string{"some typical PR entry", fmt.Sprintf("%schangelog entry", strings.ToUpper(ChangelogPrefix))}, "\n"), + 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-prefix-in-changelog-entry", + body: strings.Join([]string{"some typical PR entry", strings.Repeat(ChangelogPrefix, 5)}, "\n"), + 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-only-changelog-in-body", + body: fmt.Sprintf("%schangelog entry", ChangelogPrefix), + expected: []string{"changelog entry"}, }, { desc: "pass-multiple-entries", @@ -81,28 +77,24 @@ func TestGetChangelogEntry(t *testing.T) { "entry 2", "entry 3", }, - shouldError: false, }, { - desc: "fail-if-no-body", - body: "", - shouldError: true, + desc: "empty-if-no-body", + body: "", + expected: nil, }, { - desc: "fail-if-no-entry", - body: "some typical PR entry", - shouldError: true, + desc: "empty-if-no-entry", + body: "some typical PR entry", + expected: nil, }, } for _, test := range tests { t.Run(test.desc, func(t *testing.T) { - b, ctx := buildTestingFixtures() + b, _ := buildTestingFixtures() - changelogEntries, err := b.getChangelogEntries(ctx, test.body) - require.Equal(t, test.shouldError, err != nil) - if !test.shouldError { - require.Exactly(t, test.expected, changelogEntries) - } + changelogEntries := b.getChangelogEntries(test.body) + require.Exactly(t, test.expected, changelogEntries) }) } }