From dbfe3ae66c71301a4cc5449631676acc08f2044b Mon Sep 17 00:00:00 2001 From: rosstimothy <39066650+rosstimothy@users.noreply.github.com> Date: Tue, 4 Feb 2025 09:31:44 -0500 Subject: [PATCH] Revert "Backport merged commit SHA instead of individual commits (#313)" (#320) This reverts commit ed99ff62ba6f4adb0095c64a914a588f19e5d5db. Co-authored-by: github-actions --- bot/internal/bot/backport.go | 21 ++++++++------- bot/internal/bot/bot.go | 3 +++ bot/internal/github/github.go | 50 +++++++++++++++++++++++------------ 3 files changed, 47 insertions(+), 27 deletions(-) diff --git a/bot/internal/bot/backport.go b/bot/internal/bot/backport.go index caad629a..d015542c 100644 --- a/bot/internal/bot/backport.go +++ b/bot/internal/bot/backport.go @@ -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) @@ -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) @@ -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. diff --git a/bot/internal/bot/bot.go b/bot/internal/bot/bot.go index c22e2056..8fe2c398 100644 --- a/bot/internal/bot/bot.go +++ b/bot/internal/bot/bot.go @@ -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) diff --git a/bot/internal/github/github.go b/bot/internal/github/github.go index 501a65d4..7cd1e737 100644 --- a/bot/internal/github/github.go +++ b/bot/internal/github/github.go @@ -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. @@ -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, @@ -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 } @@ -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 {