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

Fix cherry-pick aborting #480

Closed
wants to merge 2 commits into from
Closed

Fix cherry-pick aborting #480

wants to merge 2 commits into from

Conversation

Jackenmen
Copy link
Contributor

@Jackenmen Jackenmen commented Aug 6, 2021

While I can't really confirm whether this has been an issue for miss-islington since I'm not running it myself (I am however working on a bot for a different project, which also uses CherryPicker API and I have ran in these kinds of issues), I believe this should fix some issues related to backport task trying to abort the cherry-pick when the backport fails.

If I haven't missed something, before this change abort_cherry_pick() would fail to run due to CherryPicker's initial_state not being BACKPORT_PAUSED. initial_state is, as the name suggests, initial and therefore it doesn't get updated when a new state is set.

This means that when CherryPickException is raised, the initial_state still is set to UNSET rather than BACKPORT_PAUSED therefore not allowing to abort. To solve this, I make another instance of CherryPicker after this exception is raised.

As for BranchCheckoutException, currently when this is raised, the state that cherry-picker ends up with is BACKPORT_LOOP_START which is why instead of making another instance of CherryPicker (which would lead to an error due to it being an unallowed state; note: this can also be an issue when running cherry-picker from CLI), I resorted to a bit more hacky solution of just changing its state to a valid one. I know it would be better to solve this in cherry-picker itself in some way but I wasn't exactly sure how the maintainers of it would want to proceed about it. See python/cherry-picker#38

@Jackenmen
Copy link
Contributor Author

Jackenmen commented Aug 6, 2021

While I can't really confirm whether this has been an issue for miss-islington since I'm not running it myself

After digging a bit more, I found that this has definitely been an issue for miss-islington back in 2019 so it most likely still is a problem based on the fact that this part of error handling wasn't touched since the linked comment was made.

@Jackenmen
Copy link
Contributor Author

It's very possible that the PR I made on the cherry-picker repo (python/cherry-picker#39) is enough to solve this and this PR is unnecessary but I failed to mention whether it's enough when I wrote it (damn you, past me 😄) and I can't actually say it fixes it for sure.

I might try figuring that out later this week but no guarantees there.

@codecov
Copy link

codecov bot commented May 12, 2022

Codecov Report

Merging #480 (35eaa56) into main (30c4a00) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #480   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           10        10           
  Lines         1009      1009           
  Branches        71        71           
=========================================
  Hits          1009      1009           

@Mariatta
Copy link
Member

I think let's start by fixing the cherry-picker CLI first and see if the problem still persists.

@Mariatta
Copy link
Member

Mariatta commented Oct 4, 2022

Closing the PR. I plan to release cherry-picker later this week. Thanks!

@Mariatta Mariatta closed this Oct 4, 2022
@Jackenmen Jackenmen deleted the patch-1 branch January 5, 2023 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants