-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[WIP] Create GitHub Actions Release workflow, refactor steps using GitHub Actions composite steps #1423
Conversation
982b79c
to
c80f0ff
Compare
|
awesome :) |
3fcbec3
to
db4f54e
Compare
| name: Release Twisted | ||
|
|
||
| on: | ||
| release: |
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 couldn't actually test this, because I would need to trigger a Release event via
GitHub. But we can play with this and tune this when we do the next Twisted release.
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.
You can create a test release and then delete it.
When creating the release, you can use an existing tag, to prevent creating a new tag inside the repo.
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.
What tag can I use? Can I use a tag that does not have this release.yaml file?
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.
Another idea, is to have this workflow triggered by each PR.
In this way we have Continuous Integration and we make sure that a PR is not breaking the release script.
The workflow can have a precondition for the final publish step, to skip the publish if a new tag was not created.
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 see that the workflow already has this condition for the publish...so is best to enable it on any PR and not only on release.
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.
What tag can I use? Can I use a tag that does not have this release.yaml file?
Good question. I think the answer is now.
So I think that you can create a 20.4.0.dev0 tag.
I am not sure what is the best way to mark a version as DEV. that is non-production and not even pre-release.
I would say that you can try with .dev0 as this is PEP440... it this doesn't work, try 20.4.0-dev0 based on semver schema.
Thanks!
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.
https://twisted.readthedocs.io/en/latest/core/development/policy/release-process.html#version-numbers says that the release numbers need to be time based, so wouldn't that mean 20.10.0.dev0 ?
According to https://twisted.readthedocs.io/en/latest/core/development/policy/release-process.html#announce , the dev0 postfix is added by incremental.
|
the 'mypi / test_run' can be removed. The release secret PyPI token is not yet in the repo. You can trigger the pre-release and we should see that it fails. But as part of this PR we can list the files which are planned to be pushed to PyPI and debug then. |
|
FYI, I had to disable the MyPy branch protection check in order to merge #1269. Please re-enable it when you're done here, or let me know what it's now called so that I can do so. |
db4f54e
to
f6fa486
Compare
It's gone. |
|
The publish step should be executed only once. I feel that the current GitHub Action implementation is over-engineered. For example the linter and documentation can continue to stay in their own separate worflows without having the linter or docs steps shared in other workflows. The release can be it its own workflow and have the following:
I know that for 100% correctnes you can automatically block a release is a test is failing. So I think that we should have separate tests for each environment. For the automated release, rather than triggering the workflow on a release, I think is best to trigger it when a new tag is created. The reason for that is that if the release process is triggered by a new I think that it might help to start with a description of the release process.
|
|
Your review is annoying. Originally, I had release as a separate workflow, which did what you said, but then you said you didn't want to duplicate the logic. Now I changed it to what you said, and now you changed your mind, and want me to go back to what I had before Can you hold off any further reviews of this workflow for a while? I'm trying to focus on getting the release UI in GitHub working, and pushes to PyPI working. While I appreciate your previous help on reviews, in this case you are not being helpful. I also don't appreciate your comment about this workflow being overengineered. While being helpful on reviews, you have done none of the work to make this happen. |
|
I changed things back to what I had before with having the |
|
@glyph In order to move forward with this PR, I need help to push out a new version of Incremental, and I need to have a Pypi token stored in GitHub Secrets. I mentioned this on the mailing list: https://twistedmatrix.com/pipermail/twisted-python/2020-October/065274.html |
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 think this will be greatly simplified by moving C builds for manylinux and macos into a separate project
The c builds for manylinux and macos are only useful in Twisted self tests, and there's no point in shipping them to pypi
|
@graingert Thanks for the feedback, but for this initial go-around of trying to do the release, I'm not going to incorporate your suggestion. |
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 feel pretty strongly that we should not be executing both the release and all of the automation that facilitates it at once.
It's a shame that we need to run so many random YAML files that have nothing to do with Twisted's own functionality through our own code review process, rather than having a common tool that can do this without so much human oversight. Thanks for pushing through this, craig.
I think that @graingert is working on landing this in smaller chunks so perhaps this WIP will be fine without much in the way of changes besides being broken up…
|
The work from Thomas was move here into a twisted/twisted branch and is ready for review #1464 I think that this can be closed. |
Contributor Checklist:
tox -e black-reformatto format my patch to meet the Twisted Coding Standardreviewto the keywords field in Trac, and putting a link to this PR in the comment; it shows up in https://twisted.reviews/ now.Notes
I looked at the following pages to get ideas for how to do this: