-
Notifications
You must be signed in to change notification settings - Fork 28
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
Add conflict_resolution
input
#417
Conversation
merge_on_conflicts
input - from #386merge_on_conflicts
input
Issue identifiedI 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., |
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.
🤔 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 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. |
Thank you for the initial feedback!
Regarding this, just to clarify my idea, I was thinking of always giving the
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 |
So, if only a single commit
So, the DX differs for backports with conflicts, but it already does anyway (PR is draft, manual effort is required). I like it 👍 |
* changed param name * refactor for new commit conflict_resolution flag * refactor for new commit conflict_resolution flag * Fix unit test --------- Co-authored-by: Alex Vermette <vermz@hotmail.ca>
Signed-off-by: Alexandre Vermette Lefebvre <AlexVermette-Eaton@users.noreply.github.com>
@korthout Ready for review. What I did
What I did not do
|
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apology for the long wait @AlexVermette-Eaton @vermz99, but I'm fully back again now! After my move, I first reviewed and merged #405. And today, I found time to review this great addition!
🙌 First off, thanks for the changes! I really like how this has evolved.
🙇 Sadly, some conflicts were introduced by merging #405. Let me know if you need some help to resolve these.
💭 As your changes refer to origin
in some places, it will not function correctly together with the new downstream_repo
feature (from #405). That's totally fine for now. We can improve that in a separate PR later, as both these features are experimental. Let's ship first, and then improve.
🔧 I also have a few suggestions, but nothing about the functionality itself. Please have a look.
👍 I'm happy how you've managed to separate the experimental parts from the existing code, this makes it easy for me to approve and merge.
👍 Regarding tests, I'm happy to see that you've added some unit tests. That's really enough for me. We can consider adding an E2E case in the future. As this is experimental, I'd rather ship it in an early form than to have comprehensive tests in place. The same goes for outputs, labeling, other input options, etc.
So, please have a look at my suggestions and resolve the conflicts. Then we should be good to ship it
const { exitCode } = await this.git("cherry-pick", ["-x", sha], pwd); | ||
|
||
if (exitCode !== 0) { | ||
if (exitCode === 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is a conflict the only reason for 1
as exit code? If not, we could consider matching the stdout or stderr to be sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on what I read, a conflict should always return 1
, but the other way around is not true. 1
might now always indicate a proper conflict. For example, I believe an "empty" cherry-pick will also return 1
. I assumed here that 1
would equal a conflict since it's very hard to be sure that a proper conflict occured that would make sense with our resolution suggestion. There are so many different ways that a conflict can occur based on git unit test: https://github.com/git/git/blob/master/t/t3507-cherry-pick-conflict.sh. I am open to suggestion, I just don't know in what way I should parse stdout
. Also there might be a solution using the output of git status
, but i'm not too familiar with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When there is a conflict, the stdout will contain hint: After resolving the conflicts, mark them with
, which we could use as an indication of a conflict. It's not so pretty, but it should work and it's unlikely that this will change anytime soon.
However, I'm also okay with just using exitCode === 1
for now. It's just as unlikely that the exit code changes, and it doesn't need to be perfect at this time.
…tracking with backport_on_conflicts
…yml default value.
Thanks a lot for the review! 🚀 New Changes
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's great! Thanks for the fast changes! @vermz99 @AlexVermette-Eaton 🚀
✅ I ran a successful test here (backporting to a branch called conflict
): korthout/backport-action-test#371
- It first ran with defaults, producing this comment
- Then it ran with
"conflict_resolution": "draft_commit_conflicts"
, resulting in this comment, and this pr
❌ I noticed the following that we should change before merging:
- We should suggest the safer
git push --force-with-lease
rather thangit push --force
- The commit with the conflicts has double quotes surrounding its commit message
🙇 Please adjust this still, and then I'll merge this soon. Or if I find some time during the weekend, I might just pick up these last two items. Anyways, thanks for all the hard work. This looks great!
Also, apologies for the 2 accounts shuffling, work forces us to use a different account.
No worries, I'll just tag both :)
Thanks for the review and thank you very much for the testing! 🔥🚀 New Changes
Again don't hesitate if you need additional changes. |
backport_on_conflicts
inputconflict_resolution
input
When there is a checkout failure, there was no conflict resolution applied. The default should be such that we don't have to care about this parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀 I've tested the feature again, and it works well!
💭 I hope we get feedback from users about it.
💭 I feel improvements remain in the suggestions code, but this will change with #415 anyway. There is no need to spend more time on it now.
LGTM 👍
Summary
The goal of this PR is to allow creating draft PR for backport that resulted in merge conflict.
Similar to #386
Consideration
cherry-pick
is stopped at first commit conflict.The commit in conflict is not committed, but all the previous ones are.Dev Notes
TODO
backport_on_conflicts
remains inexperimental