Add `conflict_resolution` input

vermz99 commented 6 months ago

Summary

The goal of this PR is to allow creating draft PR for backport that resulted in merge conflict.

Similar to #386

Consideration

New input was added in experimental since side effect are not fully well-know. I did some testing and it seems to work great, but I might have overlooked something.
cherry-pick is stopped at first commit conflict. ~~The commit in conflict is not committed, but all the previous ones are.~~
PR is created as Draft to ensure the user is aware that it is an incomplete PR
Suggestion resolution message is added both in original PR and new PR to ensure the user take action and do not merge unresolved PR.
Might need to add label to emphasize even more that PR need to solve some conflict?

Dev Notes

Did some refactoring in the messages to try to make this as centralized as possible. I am open to suggestion here if this does not align with maintainers vision.

TODO

[x] add to ReadMe
[x] add unit test (open to suggestion here)
[x] Determine if backport_on_conflicts remains in experimental
[x] Determine if label is needed
[x] Add commit conflict logic

vermz99 commented 6 months ago

Issue identified

I overlooked that GH does not allow to create PRs for 2 identical branches.

[!IMPORTANT] This PR needs rework

I probably will commit the conflict like initially suggested in #386, but i'll provide instruction in the suggestion to reset last commit (i.e., git reset --hard HEAD^) and restart cherry-pick from the conflict followed by git push --force.

korthout commented 6 months ago

Thanks for the amazing contribution @vermz99. It looks very promising! I'll provide some more detailed comments in the coming week, but wanted to already share some early thoughts.

I probably will commit the conflict like initially suggested in https://github.com/korthout/backport-action/issues/386, but i'll provide instruction in the suggestion to reset last commit (i.e., git reset --hard HEAD^) and restart cherry-pick from the conflict followed by git push --force.

🤔 This is worsening the DX for the cases where some of the commits could be cherry-picked without conflicts. Or it introduces a special case where users need to interact differently with the created draft PR in relation to those cases I mentioned. Different interactions may be hard to get used to when using backport-action in a repo actively like our team does.

My current thought is that we should go with your first idea instead. We can make the feature flag a bit more flexible, for example by calling it conflict_resolution taking one of a few values fail (fails the backport) or partial_draft (your first idea with partial backport of as much it can do). Then, we can later expand this with other modes like partial_draft_allowing_empty (special case dealing with the first commit leading to conflicts) or draft_commit_conflicts (with the behavior described in #386).

With that in mind, we can keep the initial version simple. It will improve the situation for pull requests with multiple commits. What do you think?

Regarding labeling, I think an output may be more flexible. It would allow anyone to add labeling afterward, or do anything else they want.

vermz99 commented 6 months ago

Thank you for the initial feedback!

🤔 This is worsening the DX for the cases where some of the commits could be cherry-picked without conflicts. Or it introduces a special case where users need to interact differently with the created draft PR in relation to those cases I mentioned. Different interactions may be hard to get used to when using backport-action in a repo actively like our team does.

Regarding this, just to clarify my idea, I was thinking of always giving the git reset --hard HEAD^ set of instructions when a conflict is encountered. This will rollback only the conflict commit, not all the previous ones correctly cherry-picked. So it would always look like that, in cases where conflicts occur:

cherry-pick all up until, and including, the first conflict encountered
Create draft PR
Provide the following set of instructions on both PR (original and new one)
```
git fetch origin
git worktree add
cd .worktree
git switch
git reset --hard HEAD^ #To rollback only the first commit, which should always have a conflict
git cherry-pick sha1 sha2 sha3 ... shaN
git push --force
```
I can see how it might add another level of complexity if we handle all the cases differently (i.e., first commit conflict has a different behavior than conflict on subsequent commits), that's why I had in mind to always have the above behavior, regardless of the position of the conflict.

For now while we think a bit about it and to give other contributors a chance to see this, I'll rework the code to allow different flags for different behavior.

[!NOTE] If we indeed decide to have a partial_draft_allowing_empty option, I made some tests and we could amend nothing to HEAD to create a new commit, but that would for sure dirty the history a bit and may be problematic for some people. Something to think about...

korthout commented 6 months ago

So, if only a single commit abcdef12 is being backported and it conflicts, then the instructions have the same structure as a backport of many commits where any of them conflicts.

git fetch origin
git worktree add
cd .worktree
git switch
git reset --hard HEAD^ #To rollback only the first commit, which should always have a conflict
git cherry-pick abcdef12
git push --force

So, the DX differs for backports with conflicts, but it already does anyway (PR is draft, manual effort is required). I like it 👍

AlexVermette-Eaton commented 6 months ago

@korthout Ready for review.

What I did

Added the conflict_resolution flag, as suggested. I only implemented fail and draft_commit_conflicts for now. We can discuss if other cases are needed for first version of this feature. As this is a significant changes, I don't want to introduce too many things that can lead to bugs.
Added unit tests. I tried to cover as much as possible the new feature. Tell me if you need more.
Refactored a bit everything around suggestions. I think I might need #305 relatively soon so I tried to keep that in mind when I designed this part.

What I did not do

I did not add an output as I don't really know how it should be represented. I'm open to suggestion if we want this for the first version.
I did not implement partial_draft and partial_draft_allowing_empty.
Test all possible scenarios. This is pretty hard and time consuming to test so if you want to help me test it on your side, that would be awesome. I'm also open to deploy it in prod on my side (20+ devs) to test it before merging, if we think it's necessary.

korthout commented 6 months ago

Thanks @AlexVermette-Eaton 🙇 My apology for not having found the time to review it.

Please note that I'm currently moving internationally, and I'll review it as soon as I can, but I can't make promises now. If you need this functionality immediately, I suggest your team temporarily switches to the version in your fork until we merge this.

I hope to be back at full capacity in a few weeks, and who knows, I may still find a moment to sit down for this soon enough. Sorry for the inconvenience.

vermz99 commented 4 months ago

Thanks a lot for the review! 🚀

New Changes

I merged the new feature from main. I added some things to try and support it, but I think it still needs some work to adjust correctly the messages printed in the original PR and draft PR.
I added your suggestion for the while loop
I added the read me
I fixed the default value in the .yml file

Let me know if you want anything else added! Also, I would appreciate it if you could try it on your side and see if it behaves as it should 🙏.

Also, apologies for the 2 accounts shuffling, work forces us to use a different account.

AlexVermette-Eaton commented 4 months ago

Thanks for the review and thank you very much for the testing! 🔥🚀

New Changes

Replace --force with --force-with-lease
Fixed "BACKPORT-CONFLICT" to BACKPORT-CONFLICT

Again don't hesitate if you need additional changes.

korthout / backport-action