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.

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

Revert "Backport merged commit SHA instead of individual commits (#313)" #320

Merged
merged 1 commit into from
Feb 4, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
21 changes: 11 additions & 10 deletions bot/internal/bot/backport.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ func (b *Bot) Backport(ctx context.Context) error {
return trace.BadParameter("automatic backports are only supported for internal contributors")
}

pull, err := b.c.GitHub.GetPullRequest(ctx,
pull, err := b.c.GitHub.GetPullRequestWithCommits(ctx,
b.c.Environment.Organization,
b.c.Environment.Repository,
b.c.Environment.Number)
Expand Down Expand Up @@ -155,7 +155,7 @@ func (b *Bot) Backport(ctx context.Context) error {
// BackportLocal executes dry run backport workflow locally. No git commands
// are actually executed, just printed in the console.
func (b *Bot) BackportLocal(ctx context.Context, branch string) error {
pull, err := b.c.GitHub.GetPullRequest(ctx,
pull, err := b.c.GitHub.GetPullRequestWithCommits(ctx,
b.c.Environment.Organization,
b.c.Environment.Repository,
b.c.Environment.Number)
Expand Down Expand Up @@ -236,15 +236,16 @@ func (b *Bot) createBackportBranch(ctx context.Context, organization string, rep
return trace.Wrap(err)
}

// Cherry-pick the _merged_ commit on the base branch instead of
// the individual commits from the PR to the backport branch.
if err := git("cherry-pick", pull.MergeCommitSHA); err != nil {
// If cherry-pick fails with conflict, abort it, otherwise we
// won't be able to switch branch for the next backport.
if errAbrt := git("cherry-pick", "--abort"); errAbrt != nil {
return trace.NewAggregate(err, errAbrt)
// Cherry-pick all commits from the PR to the backport branch.
for _, commit := range pull.Commits {
if err := git("cherry-pick", commit); err != nil {
// If cherry-pick fails with conflict, abort it, otherwise we
// won't be able to switch branch for the next backport.
if errAbrt := git("cherry-pick", "--abort"); errAbrt != nil {
return trace.NewAggregate(err, errAbrt)
}
return trace.Wrap(err)
}
return trace.Wrap(err)
}

// Push the backport branch to Github.
Expand Down
3 changes: 3 additions & 0 deletions bot/internal/bot/bot.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,9 @@ type Client interface {
// GetPullRequest returns a specific Pull Request.
GetPullRequest(ctx context.Context, organization string, repository string, number int) (github.PullRequest, error)

// 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.
CreatePullRequest(ctx context.Context, organization string, repository string, title string, head string, base string, body string, draft bool) (int, error)

Expand Down
50 changes: 33 additions & 17 deletions bot/internal/github/github.go
Original file line number Diff line number Diff line change
Expand Up @@ -196,14 +196,11 @@ type PullRequest struct {
UnsafeLabels []string
// Fork determines if the pull request is from a fork.
Fork bool
// MergeCommitSHA changes depending on the state of the pull request. Before merging a pull request,
// the MergeCommitSHA holds the SHA of the test merge commit. After merging a pull request,
// the MergeCommitSHA changes depending on how you merged the pull request:
// Commits is a list of commit SHAs for the pull request.
//
// If merged as a merge commit, MergeCommitSHA represents the SHA of the merge commit.
// If merged via a squash, MergeCommitSHA represents the SHA of the squashed commit on the base branch.
// If rebased, MergeCommitSHA represents the commit that the base branch was updated to.
MergeCommitSHA string
// It is only populated if the pull request was fetched using
// GetPullRequestWithCommits method.
Commits []string
}

// Branch is a git Branch.
Expand Down Expand Up @@ -283,6 +280,27 @@ func (f PullRequestFiles) SourceFiles() (files PullRequestFiles) {
return files
}

// GetPullRequestWithCommits returns the specified pull request with commits.
func (c *Client) GetPullRequestWithCommits(ctx context.Context, organization string, repository string, number int) (PullRequest, error) {
pull, err := c.GetPullRequest(ctx, organization, repository, number)
if err != nil {
return PullRequest{}, trace.Wrap(err)
}

commits, _, err := c.client.PullRequests.ListCommits(ctx, organization, repository, number, &go_github.ListOptions{})
if err != nil {
return PullRequest{}, trace.Wrap(err)
}

for _, commit := range commits {
if len(commit.Parents) <= 1 { // Skip merge commits.
pull.Commits = append(pull.Commits, *commit.SHA)
}
}

return pull, nil
}

// GetPullRequest returns a specific Pull Request.
func (c *Client) GetPullRequest(ctx context.Context, organization string, repository string, number int) (PullRequest, error) {
pull, _, err := c.client.PullRequests.Get(ctx,
Expand Down Expand Up @@ -311,11 +329,10 @@ func (c *Client) GetPullRequest(ctx context.Context, organization string, reposi
Ref: pull.GetHead().GetRef(),
SHA: pull.GetHead().GetSHA(),
},
UnsafeTitle: pull.GetTitle(),
UnsafeBody: pull.GetBody(),
UnsafeLabels: labels,
Fork: pull.GetHead().GetRepo().GetFork(),
MergeCommitSHA: pull.GetMergeCommitSHA(),
UnsafeTitle: pull.GetTitle(),
UnsafeBody: pull.GetBody(),
UnsafeLabels: labels,
Fork: pull.GetHead().GetRepo().GetFork(),
}, nil
}

Expand Down Expand Up @@ -358,11 +375,10 @@ func (c *Client) ListPullRequests(ctx context.Context, organization string, repo
Ref: pull.GetHead().GetRef(),
SHA: pull.GetHead().GetSHA(),
},
UnsafeTitle: pull.GetTitle(),
UnsafeBody: pull.GetBody(),
UnsafeLabels: labels,
Fork: pull.GetHead().GetRepo().GetFork(),
MergeCommitSHA: pull.GetMergeCommitSHA(),
UnsafeTitle: pull.GetTitle(),
UnsafeBody: pull.GetBody(),
UnsafeLabels: labels,
Fork: pull.GetHead().GetRepo().GetFork(),
})
}
if resp.NextPage == 0 {
Expand Down
Loading