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

switch to deploy environment and configure for pypi oidc #10925

Merged
merged 4 commits into from
Jul 3, 2023

Conversation

RonnyPfannschmidt
Copy link
Member

closes #10871
closes #10870

Copy link
Member

@bluetech bluetech left a 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!

@RonnyPfannschmidt
Copy link
Member Author

good point, lets wait with the merge until i have it split

@RonnyPfannschmidt
Copy link
Member Author

i'll squash this one

@RonnyPfannschmidt RonnyPfannschmidt force-pushed the RonnyPfannschmidt-pypi-oidc-deploy-env branch from 70d5e5b to e567afb Compare April 18, 2023 11:49

release-notes:

# todo: generate the content in the build job
Copy link
Member

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.

Copy link
Member Author

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

@nicoddemus
Copy link
Member

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?

@RonnyPfannschmidt
Copy link
Member Author

As beta User im able to add the config

@nicoddemus
Copy link
Member

Ahh per-user, makes sense.

@hugovk
Copy link
Member

hugovk commented Apr 20, 2023

It's now public and open to all 🚀

https://blog.pypi.org/posts/2023-04-20-introducing-trusted-publishers/

@nicoddemus
Copy link
Member

nicoddemus commented Apr 21, 2023

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.

@RonnyPfannschmidt RonnyPfannschmidt force-pushed the RonnyPfannschmidt-pypi-oidc-deploy-env branch from e567afb to 76ae840 Compare April 21, 2023 12:25
contents: write

timeout-minutes: 10
environment: deploy
Copy link
Member

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?

- 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
Copy link
Member

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:

Suggested change
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
Copy link
Member

Question:

image

Our prepare-release-pr creates branches in the form release-X.Y.Z, so I believe this configuration should be in the form release-[0-9].[0-9].[0-9]?

@RonnyPfannschmidt
Copy link
Member Author

@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

@nicoddemus
Copy link
Member

nicoddemus commented Apr 21, 2023

I'm OK with leaving environments out for now, however I think environments can work as we intend.

I changed pytest-mock deployment to using a manual deploy trigger, which is expected to be executed only in branches in the form release-X.Y.Z (this is configured in the environment as release-*, but can be updated to release-[0-9].[0-9].[0-9] of course).

The workflow in pytest-mock is then:

  1. Open a PR preparing a release, in a branch named release-VERSION, updating the changelog (similar to pytest's except there this process is manual).
  2. Once all tests pass, trigger the deploy workflow manually, in the release-VERSION branch. The deploy step is dependent on the deploy environment, so its restrictions apply: it can only be triggered over branches named release-*, and it requires approvals.
  3. Once approved, the workflow starts.

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:

image

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 Environment now, and changing the workflow later to be triggered manually.

@RonnyPfannschmidt
Copy link
Member Author

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

@RonnyPfannschmidt
Copy link
Member Author

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

@nicoddemus
Copy link
Member

nicoddemus commented Apr 21, 2023

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

You mean whenever we merge to master or to the latest maintenance branch (7.3.x now), we automatically run the prepare-release-pr workflow, to update existing PRs which is always up-to-date and ready for release? Sounds good. It can be done easily today I think, we just need to change the triggers for prepare-release-pr and small adjustments to automatically find the version based on current tags/branch.

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

Basically just remove the environment keys from the build and deploy jobs, correct? If so I agree. 👍

@RonnyPfannschmidt
Copy link
Member Author

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
@RonnyPfannschmidt RonnyPfannschmidt force-pushed the RonnyPfannschmidt-pypi-oidc-deploy-env branch from 76ae840 to 48e1b77 Compare May 13, 2023 05:45
@RonnyPfannschmidt
Copy link
Member Author

@nicoddemus @bluetech i finally implemented the review comments and rebased
additionally i added the oidc for no environment

once this lands and we released pytest using oidc, im going to work on moving pytest to the pytest-dev org on pypi

@nicoddemus
Copy link
Member

Sounds good.

I would be happy to change the workflow in a separate PR to the one we have in pytest-xdist.

@nicoddemus
Copy link
Member

@RonnyPfannschmidt gentle ping, just stumbled on this and thought would give a ping in case this was forgotten.

@RonnyPfannschmidt
Copy link
Member Author

@nicoddemus thanks for the ping, lets merge this once its green

@nicoddemus nicoddemus merged commit bea56b3 into main Jul 3, 2023
24 checks passed
@nicoddemus nicoddemus deleted the RonnyPfannschmidt-pypi-oidc-deploy-env branch July 3, 2023 15:52
@nicoddemus
Copy link
Member

This needs to be manually backported:

To https://github.com/pytest-dev/pytest
! [remote rejected] backport-10925-to-7.4.x -> backport-10925-to-7.4.x (refusing to allow a GitHub App to create or update workflow .github/workflows/deploy.yml without workflows permission)

On it.

nicoddemus added a commit that referenced this pull request Jul 3, 2023
…1162)

Closes #10871
Closes #10870

Co-authored-by: Ronny Pfannschmidt <opensource@ronnypfannschmidt.de>
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

Successfully merging this pull request may close these issues.

Use separate GitHub environment for deploy workflows Use PyPI Trusted Publishers feature to publish releases
4 participants