-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
switch to deploy environment and configure for pypi oidc #10925
Conversation
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.
They recommend splitting the building and uploading steps to separate jobs, such that only the upload step gets the id-token: write
permission. But I can do this later.
LGTM!
good point, lets wait with the merge until i have it split |
i'll squash this one |
70d5e5b
to
e567afb
Compare
|
||
release-notes: | ||
|
||
# todo: generate the content in the build job |
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.
Can you leave a comment explaining why?
I ask because I think it is fine to do it (generate the contents + publish to GitHub releases) at this point.
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.
expanded the comment, ready for merge i think
Thanks @RonnyPfannschmidt! In #10870 it was mentioned the project needed to be part of the beta... is pytest part of the beta, or this is not a requirement anymore? |
As beta User im able to add the config |
Ahh per-user, makes sense. |
It's now public and open to all 🚀 https://blog.pypi.org/posts/2023-04-20-introducing-trusted-publishers/ |
Btw this change inspired me to change the deploy of pytest-mock -- deploy is manually triggered, and the tag is pushed by the workflow itself right after the package is published. I have not tested it yet, but seems like it is more clean that way. |
e567afb
to
76ae840
Compare
.github/workflows/deploy.yml
Outdated
contents: write | ||
|
||
timeout-minutes: 10 | ||
environment: deploy |
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 guess we do not need the deploy
environment to build the package?
.github/workflows/deploy.yml
Outdated
- name: Download Package | ||
uses: actions/download-artifact@v3 | ||
with: | ||
name: Packages | ||
path: dist | ||
|
||
- name: Publish package to PyPI | ||
uses: pypa/gh-action-pypi-publish@release/v1 |
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.
Might as well update to the latest version:
uses: pypa/gh-action-pypi-publish@release/v1 | |
uses: pypa/gh-action-pypi-publish@v1.8.5 |
(I just tested this version with pytest-mock
and it worked flawlessly to publish it to test-pypi with trusted-publishers).
@nicoddemus @bluetech i believe we need to cross-check a misunderstanding after the note i did a re-read on environments and it seems to me we cant actually use them for our deployments as they target branches, not tags based on my understanding i propose we postpone using environments as they seem to be intended for having long lived branches be deployed regularly instead of managing release brances as we intend to use them |
I'm OK with leaving environments out for now, however I think environments can work as we intend. I changed The workflow in
One important difference from the previous workflow (and pytest's) is that this workflow pushes the tag after the package is pushed to PyPI, instead of the workflow being triggered by a tag. This is cleaner than what we have today, because if anything wrong happens with the release, the tag is already in the repository. Here is the workflow file in case you want to take a look: https://github.com/pytest-dev/pytest-mock/blob/main/.github/workflows/deploy.yml Here is the environment configuration: One neat thing we gain by using Environments is a deployment history page: https://github.com/pytest-dev/pytest-mock/deployments (Not very useful in our case, but neat nonetheless). Having said all that, I'm fine with removing the use of |
Looks lovely We might want to adapt this for pytest In particular if we can let approval trigger the release we could do a setup where the next potential release is always prepared and we only need to have core do multi approval to get it out of the door We could probably even go as far as having a test pypi of those always be prepared |
So my proposal is that for this Mr I prepare the config without environments However we will leave the oidc config for the environment in pypi intact so we can add the enhanced release workflows as follow up whenever we want/can |
You mean whenever we merge to
Basically just remove the |
Exactly |
* the build step builds using baipp * the deploy step does only the pypi upload * the release-notes step udpdates the release notes ## needed followups * [ ] upstream release from artifact to pypi-publish * [ ] generate content of release notes in baipp step
* no more environments until its clean what they do * version pin changes to actions
76ae840
to
48e1b77
Compare
@nicoddemus @bluetech i finally implemented the review comments and rebased once this lands and we released pytest using oidc, im going to work on moving pytest to the pytest-dev org on pypi |
Sounds good. I would be happy to change the workflow in a separate PR to the one we have in |
@RonnyPfannschmidt gentle ping, just stumbled on this and thought would give a ping in case this was forgotten. |
@nicoddemus thanks for the ping, lets merge this once its green |
This needs to be manually backported:
On it. |
…1162) Closes #10871 Closes #10870 Co-authored-by: Ronny Pfannschmidt <[email protected]>
closes #10871
closes #10870