Skip to content
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.

Sign up for GitHub

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

Automatically copy changelog message to the backport or put no-changelog label #195

Merged
merged 1 commit into from
Dec 6, 2023
Merged

Automatically copy changelog message to the backport or put no-changelog label #195

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 24 additions & 5 deletions bot/internal/bot/backport.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand All @@ -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))
Expand All @@ -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,
Expand All @@ -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},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just add "labels" here than modifying labels.go, would be a little bit cleaner. I actually had this logic implemented and sitting in #148 for a while but never got a chance to wrap it up.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/gravitational/shared-workflows/pull/148/files#diff-6a92e12adffd18e18a79d45839beff2d7b47b7d29ae8335552c7faa19b83966bR127

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, damn, I tested it and it didn't work for me, now I tried again and it works, because first time I tried it with test label being "label1" and it probably didn't put it because such label doesn't exist 🤦
Will change to setting label here 👍

Copy link
Contributor Author

@AntonAM AntonAM Dec 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed. I had to force push to overwrite the commit, because during writing tests for backport flow it accidentally ran real git commands on my machine and overwrote my git user, which I didn't notice until now (it's safe now, dry run in tests).

"labels": labels,
}.Encode(),
},
})
Expand Down Expand Up @@ -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
Expand Down
113 changes: 113 additions & 0 deletions bot/internal/bot/backport_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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)
})
}
}
9 changes: 6 additions & 3 deletions bot/internal/bot/bot.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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.
Expand Down Expand Up @@ -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.
Expand Down
11 changes: 8 additions & 3 deletions bot/internal/bot/bot_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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{}
Expand Down Expand Up @@ -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 {
Expand All @@ -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
}

Expand Down
20 changes: 10 additions & 10 deletions bot/internal/bot/changelog.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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.
Expand Down
Loading
Loading