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

feat: make backport.py more flexible for complex pull requests #7260

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

cfm
Copy link
Member

@cfm cfm commented Oct 17, 2024

Status

Ready for review

Description of Changes

The much-loved backport.py script makes 2.5 assumptions that do not hold for complex pull requests like #7143, which we fix here:

  1. There are fewer than 30 commits (the default number of results returned by gh api) in the pull request.
  2. All commits can be merged naïvely via a mass git cherry-pick -x (i.e., there are no merge commits requiring git [cherry-pick → merge] -m).
    1. All of the commits in the pull request should be backported.

Testing

Depending on the state of your local release/2.10.1 branch, you can replicate #7257 with:

$ git branch -D release/2.10.1
$ git checkout -b release/2.10.1 release/2.10.0
$ utils/backport.py 7143 2.10.1 origin --skip 6e3d7bed17537fa8f69f941f21a8eec34e8da113

Deployment

Development-only; no deployment considerations.

Copy link
Member

@legoktm legoktm left a comment

Choose a reason for hiding this comment

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

Thanks, it LGTM; the one change I'm asking for is if can we mention the skipped commits in the PR body.

utils/backport.py Outdated Show resolved Hide resolved
branch = f"backport-{args.pr}"
base = f"release/{args.version}"
remote = args.remote
subprocess.check_call(["git", "fetch", remote])
subprocess.check_call(["git", "checkout", "-b", branch, f"{remote}/{base}"])
subprocess.check_call(["git", "cherry-pick", "-x"] + [commit["sha"] for commit in commits])
subprocess.check_call(["git", "cherry-pick", "-x"] + commit_hashes)
if input("OK to push and create PR? [y/N]").lower() != "y":
sys.exit()
subprocess.check_call(["git", "push", "-u", remote, branch])
Copy link
Member

Choose a reason for hiding this comment

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

In the message body below, let's mention the skipped commits? E.g. Only contains changes from #{args.pr}, except {commits}.?

Copy link
Member Author

@cfm cfm Oct 18, 2024

Choose a reason for hiding this comment

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

Good idea. Added in 234d5bf, but do let me know if you see a nicer way to build up the conditional skip_message without fancier templating.

cfm added a commit that referenced this pull request Oct 18, 2024
@cfm cfm force-pushed the backport-many-but-not-all-the-things branch from 9b3123e to 234d5bf Compare October 18, 2024 00:56
@cfm cfm requested a review from legoktm October 18, 2024 01:01
cfm and others added 4 commits November 15, 2024 09:39
For example, Weblate-side merge commits from "develop" require "git
cherry-pick -m" and therefore can't be merged en masse even if we wanted
their contents.
Otherwise only the first 30 commits are returned.
Co-authored-by: Kunal Mehta <legoktm@debian.org>
@cfm cfm force-pushed the backport-many-but-not-all-the-things branch from 234d5bf to ae00ff7 Compare November 15, 2024 17:39
@cfm cfm added the blocked label Nov 15, 2024
@cfm
Copy link
Member Author

cfm commented Nov 15, 2024

Currently blocked on #7339.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Under Review
Development

Successfully merging this pull request may close these issues.

2 participants