Add dry-run option and twine check --strict option

[mayeut]

Hello,

First, thanks for the action !

While investigating the possibility of using cibuildwheel and GitHub Actions for the cmake package, I noticed that the old CircleCI job had a specific job to run twine check --strict dist/*.

Given that gh-action-pypi-publish was already included in the new workflow, it seemed quite a duplicate effort to add yet another job to do that.

Proposals:

• Add a dry_run option to allow running the action even from pull_request events (where secrets are not defined)
  This would effectively just skip the user/password check and the twine upload steps.
• Add a verify_metadata_strict option to run twine check --strict instead of just twine check

I implemented the dry_run idea to see how it would integrate with the workflow.

I can contribute pull requests if there's some interest.

[webknjaz]

Add a dry_run option to allow running the action even from pull_request events (where secrets are not defined)

That's an interesting suggestion but I'm worried about the feature creep. The main purpose of this action is to publish stuff and the pre-check is a small addition. A dry run would shift that goal, I think.

Add a verify_metadata_strict option to run twine check --strict instead of just twine check

This one is probably more reasonable for us. I'd probably accept such a PR. OTOH it sounds like it'd be a good default just to have it strict unconditionally.

Given that gh-action-pypi-publish was already included in the new workflow, it seemed quite a duplicate effort to add yet another job to do that.

I see why you may feel like that but I prefer having a separate linting/verification job/task. There are a few reasons to do so. First, you most likely want to be able to run the same check locally, so you'd already have a tox env or something similar to do that. Then, if you follow the recommendation of first building the dists, then testing them, and only then publishing, you could run such a check activity in parallel with tests rather than later. And finally, from the UX/DX point of view, it'd nice to be able to see a separate job with the metadata checks and immediately know what's failing rather than having to dig into a specific job step's log.

[mayeut]

if you follow the recommendation of first building the dists, then testing them, and only then publishing

yep, followed. The publishing job is itself a dist check then publish.

I can understand your argument about UX/DX but I'd like to add the following:

you could run such a check activity in parallel with tests rather than later

I'd rather not, most likely because installing twine depends on cryptography which is not installable on manylinux1 (well the latest versions at least) and you now have to explain that pinning this security critical package is not an issue because you're not really using it at this step. So probably a separate job that checks just this, as was done on CircleCI.

from the UX/DX point of view, ...

Well, if it's arguable that it's a good idea to run this check in the publish action for all the reasons you mention, there's also a point to be made why it would be good for the publish action UX itself to have a dry-run (other than allowing to run the check as a convenience because of bad design).
As you said, it's better to detect issues sooner rather than later, allowing a dry run would allow to run the step in any case and detect mis-configuration way before publication (if you're not doing continuous deployment, or just to check PRs are not breaking deployment really badly).

Simple example:

Add a verify_metadata_strict option to run twine check --strict instead of just twine check

This one is probably more reasonable for us. I'd probably accept such a PR. OTOH it sounds like it'd be a good default just to have it strict unconditionally.

If this is made the default option, then:

• My guess is that it'll be detected only upon release of a number of packages wondering why their publish step failed. Even if you pin your actions, it'll only ever run on release for a number of packages meaning the dependabot PR updating your pinned published action saying everything's OK is wrong, it will break, you just can't know it yet. Having a dry-run option would prevent this.
• Why would it be the default option if the recommendation is to have a separate job to do this check ? Only hard errors should be checked here to prevent partial upload (not sure if it really applies, haven't check twine behavior).

Yet another reason is that anyway, you want to list all that would be published in a single job log and I expect that to be the publish action to avoid errors duplicating a job that just list the dist. As far as I'm concerned, the only step that knows what would be published is the publish action, it should print this list (arguably, using twine check for this is just a hack).

[webknjaz]

I'd rather not, most likely because installing twine depends on cryptography which is not installable on manylinux1 (well the latest versions at least) and you now have to explain that pinning this security critical package is not an issue because you're not really using it at this step. So probably a separate job that checks just this, as was done on CircleCI.

I usually upload artifacts from the build jobs, then I can download them in separate test and linting, and publishing jobs. With this, you could do a wildcard twine check to verify everything in one go and there's no need to do this in each build job. So if you set up the job dependencies correctly, the publish step wouldn't be the only one knowing about all the dists.

[webknjaz]

how it would integrate with the workflow.

By the way, looking at the workflow, I see a few potential problems with it:

1. You build wheels that you're about to ship from the source Git repository but it's better to build them from sdist.
2. Then, in your test jobs, you only install sdists which builds wheels for that runtime under the hood and so you end up testing wheels that are different from what you've created in your build jobs

Is that intended?

[mayeut]

I usually upload artifacts from the build jobs, then I can download them in separate test and linting, and publishing jobs. With this, you could do a wildcard twine check to verify everything in one go and there's no need to do this in each build job.

That's exactly what I was suggesting with "So probably a separate job that checks just this, as was done on CircleCI.

You build wheels that you're about to ship from the source Git repository but it's better to build them from sdist.

That's not supported by cibuildwheel yet

Then, in your test jobs, you only install sdists which builds wheels for that runtime under the hood and so you end up testing wheels that are different from what you've created in your build jobs

There's "one workflow" for sdist and "one workflow" for wheels. cibuildwheel takes care of running tests on the built wheels so everything is tested: built wheels, installing from sdist, installed sdist.