Nathanmalnoury / gh-backport-action

Github action to backport PR.
MIT License
2 stars 4 forks source link

Make PR title and body customizable templates #3

Closed chambm closed 2 years ago

chambm commented 2 years ago

Supported template variables are: 'pr_branch', 'pr_number', and 'base_branch'. The default title and body will stay the same. I'm making this PR because I'm not a fan of the default title and this was low hanging fruit. :) Thanks for the cool action!

An unrelated issue: how hard would it be to use the squashed commit instead of the individual commits?

Nathanmalnoury commented 2 years ago

Hey, It's nice to see someone using this action ! Thanks for the PR, It's definitively a nice to have feature, and the implementation is fine to me, I'll let you merge it yourself, or I'll do it during the weekend probably!


As per the squash question, I guess the answer would be "quite easy": In backport_commits (main.py:20) where holding every commit hash there is in that branch in commits. According to this SO https://stackoverflow.com/a/5201642, we could use the first hash in commits to do something like:

git('reset', '--soft', first_hash')
git('commit')

And then git(push) as it's currently done in line 35.

But even if it is easy, there is a squash and merge option in GitHub that would result in a single squash commit in the pr_branch. How does that solution compares to the squash in the action option ?

chambm commented 2 years ago

I actually did the squash and merge option in GitHub but backport_commits still created the automatic PR with the individual commits. See: https://github.com/ProteoWizard/pwiz/pull/1861 https://github.com/ProteoWizard/pwiz/pull/1868

The original PR 1861 was squashed and merged (you can see 3 individual commits but only 1 was merged into master).

I asked that question before I tried the action on a PR that had been updated from master (e.g. Merge branch 'master' into /feature/branch). When I tried that, it failed, even though I did a squash and merge (which should effectively hide the merge from master): https://github.com/ProteoWizard/pwiz/pull/1864 https://github.com/ProteoWizard/pwiz/issues/1869 (btw really cool idea to open an issue on action failure although that should probably be optional)

Nathanmalnoury commented 2 years ago

I think that behaviour is probably because the PR target branch is not squashed in git, so when the action cherry-picks it there's still the complete history in it.

The idea would then be to have 2 cherry-picking strategies:

I guess it makes it a bit more difficult than what I thought initially because we need to get that information from somewhere and then chose the strategy accordingly. And implement that squash strategy,

Thanks for the feedback on this branch ProteoWizard/pwiz#1864 : I'm not used to updating with merge, and I did not test that before. I see that there is a git cherry-pick --mainline option that would solve that.

-m parent-number, --mainline parent-number
     Usually you cannot cherry-pick a merge because you do not know which side of 
     the merge should be considered the mainline. This option specifies the parent 
     number (starting from 1) of the mainline and allows cherry-pick to replay the
     change relative to the specified parent.

But I am unsure if I am going to be able to figure out dynamically which parent to choose as the mainline.

btw really cool idea to open an issue on action failure although that should probably be optional Yes it really should!

chambm commented 2 years ago

Would rebasing before squashing solve the picking parent problem? You don't need to pick a parent to rebase, right?

Nathanmalnoury commented 2 years ago

Maybe it would, but wouldn't that mean that we'd be backporting unrelated changes from master (because of that merge) to the pr_branch ?

chambm commented 2 years ago

I don't think so. As I understand it, the rebase just changes the targeted commit for the base_branch of that PR, then regenerates new commits targeting that base code for each of the branch's commits. I'm sure the reality is much more complicated with merges from other feature branches possible, but I'm thinking about the simple case of one feature branch with no merges except from its base_branch.

As a workaround, I added an Action that lets me tell GitHub to do the rebase for me (comment /rebase). When I did that before Squash and merge, your backport action worked fine (it picked only the individual commits): https://github.com/ProteoWizard/pwiz/pull/1871 https://github.com/ProteoWizard/pwiz/pull/1873

If there was a real conflict such that the commits cannot be cleanly rebased onto the latest base_branch changes, or the commits rebased on the base_branch can not be cleanly applied to the pr_branch, then the automatic backport should fail because the conflict will have to be resolved manually.

Nathanmalnoury commented 2 years ago

Well if it works fine with another Action, I don't see why it wouldn't with this one..!

I guess the backport strategy (cherry-pick | rebase) could also be an option, and that would not be too difficult to implement I think. Thank tyou for giving it a try :slightly_smiling_face: