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

Failed git push does not fail release #385

Closed
LvffY opened this issue May 12, 2022 · 6 comments
Closed

Failed git push does not fail release #385

LvffY opened this issue May 12, 2022 · 6 comments

Comments

@LvffY
Copy link
Contributor

LvffY commented May 12, 2022

First of all : thanks for your tool :)

First, my issue

During a release, if a push fails, the release does not fail. This can make false positive CI/CD builds (i.e the pipeline return as green but the push did not went well, so it's in fact, a red pipeline).

Reproduce steps

  • Configure zest.releaser through setup.cfg file
[zest.releaser]
release = no # This one is important to see the logs
# I don't think other confs matter, but I'll leave it here, for the record
create-wheel = yes
encoding = utf-8
tag-format = {version}
extra-message = [zest.releaser] 🤖 [ci skip]
date-format = %%Y-%%m-%%d %%H:%%M:%%S
  • Make the release on a branch where you don't have the right to push (imagine having a branching name pattern setup in your Git manager)
  • run fullrelease --no-input

Expected result

The release should me marked as failed

Actual result

The release is marked as succeeded.

Here is the logs that I get :

  • Running fullrelease --no-input

Sans titre

  • In opposite, running the commands git tag test then try to push with git push origin my-branch --tags we can see that my command return a non-zero return code

Sans titre

Second, a feature request

Based on this case, it could be great to have a rollback feature, that could allow people to run some code in case of error.

I think that a default implementation could be : if something went wrong, try to remove the supposed created tag and, why not, the uploaded artifact

@LvffY
Copy link
Contributor Author

LvffY commented Aug 13, 2022

@mauritsvanrees Did you have time to check on this issue ?

@mauritsvanrees
Copy link
Member

@LvffY Sorry, no not yet. Let me have a look now.

  • In postrelease.py we call self._push.
  • This method is in baserelease.py.
  • It calls execute_command in utils.py.
  • When an executed command fails, the error is printed, but execution of the zest.releaser command continues.
  • This can only be influenced by passing allow_retry=True to execute_command. Then the user has to answer a question: do you want to retry, continue, or quit?

@reinout We do not actually use this allow_retry option anywhere, except in utils.txt where we test that this option works... I think this was mostly meant for retrying an upload, but we have the separate _retry_twine method for this now.

The easiest solution to the issue here would be to change the _push method to use allow_retry=True. Then the user will see that there is an error, and can retry, continue or quit.
I thought this would fail in CI, as this will run in no-input mode, but I see we have set things up so you get a RunTimeError then, so that would work fine.
What do you think?
In this case though, when called from CI, the CI without input would

@mauritsvanrees
Copy link
Member

@LvffY

Second, a feature request
Based on this case, it could be great to have a rollback feature, that could allow people to run some code in case of error.

I think the problem here would be: how far do you roll back? Do you revert the postrelease commit? The tag? The release commit? The prerelease commit? Commits by plugins? All of these?

And if you have used this successfully to upload a new release on PyPI, then I would not want to undo any of this. I would want to keep the exact commit and tag that were used to create this release. I can then manually push the changes to a new branch and create a PR, and that will likely solve the problem.

@LvffY
Copy link
Contributor Author

LvffY commented Aug 17, 2022

@mauritsvanrees on the rollback feature I think that we want to rollback everything.

You probably want to rollback the upload on the pipy repository as well.

But if the upload on pypi is not "rollbackable", may be you're right and we don't want to rollback anything ?

In this case, may be it could just be a documentation on how to do such a rollback if customer want to do so.

@reinout
Copy link
Collaborator

reinout commented Aug 17, 2022

The easiest solution to the issue here would be to change the _push method to use allow_retry=True. Then the user will see that there is an error, and can retry, continue or quit.

Some quick thoughts:

  • git push is the last step, so "continue" isn't really needed. Retry/quit could be nice.
  • Though the necessary "git push" command should be visible for manual re-try.
  • Some documentation about how to roll back a partially failed release could help. I would put it in the general documentation and not print it on the commandline. (Perhaps print the url of the doc to the commandline).

The actual error that you show is caused by insufficient rights due to a protected main branch. That's actually a nasty problem. The ones making releases have to be admins that are allowed to circumvent the protection.
My assumption is pretty much that the ones with pypi upload rights also have git push rights.

Anyway, such a protection isn't something that a retry is going to help with. Though... you could get someone to give you the rights and then retry it. So: the retry seems like a good idea.

reinout added a commit that referenced this issue Jul 25, 2023
@reinout
Copy link
Collaborator

reinout commented Jul 25, 2023

I have a PR #430 that (mostly) fixes this, I hope.

mauritsvanrees added a commit that referenced this issue Jul 25, 2023
Allow for 'git push' retry + a bit of documentation, fixes #385
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants