Skip to content

Conversation

@astrofrog
Copy link
Contributor

Prior to this PR, I think upload_to_pypi was potentially counter-intuitive because upload_to_pypi: false still means that sometimes things are uploaded to PyPI. I think it would be cleaner to simply specify that it is a boolean condition for the upload, defaulting to true if a v* tag is pushed.

I'm not sure if this will work though, as I don't know if a condition can be given as the default for the parameter. But I do think we should try and make it behave somehow like it is here, meaning that upload_to_pypi: false would never upload anything.

@ConorMacBride
Copy link
Member

It might work if the condition is surrounded with ${{ }} but I have a feeling conditions are not supported there.

@astrofrog
Copy link
Contributor Author

Should we make the parameter a string and then somehow evaluate it as a boolean later in the pipeline for it to work?

@astrofrog
Copy link
Contributor Author

@ConorMacBride - I probably won't have time to try and dig deeper into this today so feel free to take over the PR if you like, if you agree that we should tweak this option.

@ConorMacBride
Copy link
Member

A string would work. I can take a look later. I'll make upload_to_pypi accept 'true', 'false' and 'auto'. Or should we call it 'tag' instead of 'auto'?

@astrofrog
Copy link
Contributor Author

I guess the question is how would a user then specify that they want to upload on all tags, not just ones starting with 'v'? Basically we need a way for users to pass a completely custom condition?

@ConorMacBride
Copy link
Member

We could keep upload_to_pypi a boolean input and default to false. When the reusable workflow is called, it should be possible to do

uses: OpenAstronomy/github-actions-workflows/.github/workflows/tox.yml@main
with:
  upload_to_pypi: ${ github.event_name == 'push' && startsWith(github.event.ref, 'refs/tags/v') }

@ConorMacBride
Copy link
Member

If we do that I think we should make upload_to_pypi be required (no default) to reduce confusion (e.g. why does the publish workflow not publish by default?)

@astrofrog
Copy link
Contributor Author

@ConorMacBride yes we might have to just make it required. @Cadair, what do you think?

@ConorMacBride
Copy link
Member

I've updated the upload_to_pypi input to have the following behaviour. (With some testing here: https://github.com/ConorMacBride/test-workflows/blob/main/.github/workflows/test_upload.yml)

upload_to_pypi

Whether to upload to PyPI after successful builds.
The default is to upload to PyPI when tags that start with v are pushed.
A boolean can be passed as true (always upload) or false (never upload)
either explicitly or as a boolean expression (${{ <expression> }}).

Alternatively, a string can be passed to match the start of a tag ref.
For example, 'refs/tags/v' (default) will upload tags that begin with v,
and 'refs/tags/' will upload on all pushed tags.

@astrofrog
Copy link
Contributor Author

Nice! Thank you!

Copy link
Member

@ConorMacBride ConorMacBride left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merge?

@astrofrog
Copy link
Contributor Author

@ConorMacBride - could you rebase to resolve the conflict? Then feel free to merge.

The always() is needed for the condition to be evaluated in the case an optional dependency skips.
This is incase the `targets` job fails and the `upload_pypi` job runs anyway with no wheels to upload (as the builds are both skipped and technically not failed).
...because bool(str(false)) is true
@ConorMacBride ConorMacBride force-pushed the upload-to-pypi-update branch from ee945cf to 8c24e1d Compare March 11, 2022 22:41
@ConorMacBride ConorMacBride merged commit bdbc581 into OpenAstronomy:main Mar 11, 2022
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.

2 participants