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

Extend CI workflow to include PyPI dist publishing #100

Closed
wants to merge 2 commits into from

Conversation

Zymrael
Copy link

@Zymrael Zymrael commented Jul 20, 2021

Minor PR, title says it all.

Publishing is triggered only by release events. To work, the addition requires setup of a PyPI password in repo secrets.

Addresses this.

@google-cla
Copy link

google-cla bot commented Jul 20, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added the cla: no label Jul 20, 2021
@Zymrael
Copy link
Author

Zymrael commented Jul 20, 2021

@googlebot I signed it!

@google-cla google-cla bot added cla: yes and removed cla: no labels Jul 20, 2021
@patrick-kidger
Copy link
Collaborator

patrick-kidger commented Jul 20, 2021

So I note that this will trigger on quite a lot of things, including a cron schedule. It would be preferred to have it trigger only when we merge a pull request into master; moreover it'd need to check that the version number has been incremented, to prevent PyPI failures.

That said, I'd be keen to have this happen, as it's realistic that waiting for me or @lxuechen to push a button will be a bottleneck in the process... :D If you can rewrite it as above I'd be happy to have this in.

@Zymrael
Copy link
Author

Zymrael commented Jul 20, 2021

It sure will trigger the entire workflow but it will skip the publishing step. That last step (atm) is only triggered via release events. There seems to be a check for version increases in the current workflow put into place by @lxuechen.

Of course if you'd prefer we can split the current workflow into two. Remove the current release check, and instead have a secondary publishing workflow that only triggers on PR merges if the existing version check passes.

Should be quick but since there are roughly infinity ways of doing this I'll await for you to state your preference first :)

@patrick-kidger
Copy link
Collaborator

patrick-kidger commented Jul 20, 2021

Ah yes, I see the if statement now. I think the if statement will never trigger though, because the on: section above doesn't include release events?

So I think creating a separate workflow that does this, and triggers on release events, would probably be smoothest?

Pinging @lxuechen as the only one who can actually add the necessary secret (the PyPI key) for this.

@lxuechen
Copy link
Collaborator

Ah yes, I see the if statement now. I think the if statement will never trigger though, because the on: section above doesn't include release events?

So I think creating a separate workflow that does this, and triggers on release events, would probably be smoothest?

Pinging @lxuechen as the only one who can actually add the necessary secret (the PyPI key) for this.

I agree with Patrick here. What's more, it seems this publishing step will be ran more than once due to the multiple run environments constructed by the build matrix. Splitting the publishing step to a separate workflow is a good idea. Enforcing dependencies among workflows is a little tricky, but can be done with workflow_run.

I have created the necessary secret based on an API token, but might need Patrick to add me to the project on PyPI.

@patrick-kidger
Copy link
Collaborator

Right, I think I've done everything that should be necessary to get this working. I've opened three small PRs (#102,#103,#104) to add all the necessary functionality, in place of this one.

@Zymrael
Copy link
Author

Zymrael commented Jul 23, 2021

I will be closing this PR then.

@Zymrael Zymrael closed this Jul 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants