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

Add to publish.yml test install on push #237

Merged
merged 5 commits into from
Nov 22, 2022

Conversation

JulianGoeltz
Copy link
Contributor

Responding to the second request of #235 .

Two comments:

  • how to test this commit? I could temporarily add a
  pull_request:
    branches: [ feature_testPyPiInstall ]

to test it here, and adapt the event_names?

  • To be sure the local files are not used, I resorted to deleting the yalafi folder. The alternative used in Feature install action #236 is also possible but would need some changes to the packaging. As that would be more invasive I settled for rm for now.

What do you think?

@torik42
Copy link
Owner

torik42 commented Nov 10, 2022

I tested the CI actions locally using act and removing the if statements. One can then just call it without committing.

Content wise, I would like a separate job with needs: statement cycling through the python versions as in test.yml. And I would just take the full version (matching the stuff inside " "), so that also prereleases would be possible in general. As a bonus, one could compare the git tag with the version specified in pyproject.toml.

@JulianGoeltz
Copy link
Contributor Author

Sounds good, will implement soon. One thing:

And I would just take the full version (matching the stuff inside " "), so that also prereleases would be possible in general

for this the on part triggering the action that is currently

  push:
    tags: ['[0-9]+.[0-9]+.[0-9]+']

would have to be changed as well, right? I guess you don't want to git-tag differently than the pyproject version? So,

  push:
    tags: ['[0-9]+.[0-9]+.[0-9]+[a-z]?']

ok?

@torik42
Copy link
Owner

torik42 commented Nov 11, 2022

I think \d+.\d+.\d+((a|b|rc)(\d+)?)? is enough, and I would like the final digit (don’t care whether \d or [0-9]).

I have also had a look into your gridspeccer workflow again. This gave me two more ideas: (1) can we create a draft release automatically when we publish to test.pypi, (2) add the .wheel and tar.gz as release artifacts and then (3) just publish the artifacts to pypi without rebuilding (which would also mean that they are properly tested already) whenever the draft release is actually released.

What do you think?

@torik42
Copy link
Owner

torik42 commented Nov 11, 2022

I still see one more problem (which we might just ignore). Even test.pypi doesn’t allow overwriting published versions. Hence, if a problem occurs, we cannot run the workflow again using the same version number.

* checks git tag
* creates draft release
@JulianGoeltz
Copy link
Contributor Author

Made a new commit, hope I included everything:

  • as separate job with needs and if
  • checks git-tag vs pyproject version (done before pushing to test.pypi)
  • creates draft release with ncipollo/release-action@v1 like gridspeccer

I'm not sure how easy it is to reuse the .tar.gz and .whl for the actual release. Would you want to do that manually, or with the on: release part in the workflow?

Regarding not being able to overwrite the version on test.pypi, I would just ignore that for now. If the case arises, one can always add a version number, or try to fix stuff manually.

- remove publishing to pypi and only prepare release
- fix regex
- fix formatting
- use full test suite
- add sleep before running tests
- only run after releasing a GitHub release
- uploads the assets to PyPI
@torik42
Copy link
Owner

torik42 commented Nov 17, 2022

I decided to just test the workflows in another repo because it wasn’t possible to test creating and publishing releases locally. This way I could iterate and eliminated some mistakes. I also added a workflow which uploads the release assets to PyPI.

I had some weird errors with the automatic tests in prepare-release.yml: Sometimes, YaLafi couldn’t be installed (hence the two Minutes wait), but sometimes even the checkout action failed. The regex by the way didn’t work, only a limited set of options is allowed.

It would be great, if you could check, whether you spot any obvious mistake. So far, my tests went quite well.

- increase GITHUB_TOKEN permission (to set `content: read` as default)
- add release body
@JulianGoeltz
Copy link
Contributor Author

I've gone through the changes, and it looks good. I like splitting up the release and publish part to avoid all the ifs 👍
Just two questions:

  • why did you go from on: release: types: [published] to on: release: types: [released], I couldn't really make sense of the doc.
  • should we maybe stay consistent for multi-line run commands (between | and >, I just learned about the second one https://stackoverflow.com/a/66809682)

I can't guarantee it works, but I didn't see any errors, let's hope for the best.

@torik42
Copy link
Owner

torik42 commented Nov 21, 2022

Thank you for looking into it.

To my understanding the type released will only trigger for releases but not prereleases. And I think, if I want to publish to PyPI, I shouldn’t use a prerelease on GitHub. (See third Note box here.)

Have we mixed up | and >somewhere? Would you prefer to put all the commands with > into one line? I didn’t know the details before you pointed out that resource. But as far as I understand, we need | to run multiple commands, and > to have command line options separated. Now I am more wondering about >- as I cannot see any advantage over > in the workflow. (The comments to your linked post say it doesn’t work on GitHub. But it seems to work, and the current docs also say that workflows just need to be YAML files (which allow >-).)

In the current state, the workflows seem to work, see prepare-release and publish-release both tested with test.pypi in a different repo.

@JulianGoeltz
Copy link
Contributor Author

JulianGoeltz commented Nov 21, 2022

To my understanding the type released will only trigger for releases but not prereleases. And I think, if I want to publish to PyPI, I shouldn’t use a prerelease on GitHub. (See third Note box here.)

sounds good

Have we mixed up | and >somewhere? Would you prefer to put all the commands with > into one line? I didn’t know the details before you pointed out that resource. But as far as I understand, we need | to run multiple commands, and > to have command line options separated. Now I am more wondering about >- as I cannot see any advantage over > in the workflow. (The comments to your linked post say it doesn’t work on GitHub. But it seems to work, and the current docs also say that workflows just need to be YAML files (which allow >-).)

You're correct regarding | and >, my bad. I also saw the remark on >-, but as you say: it is working currently, maybe that was an old statement.

So just one thing left: on a release page in the test repo, why is there only the source code, and not the .whl? Did you delete this manually or am I missing something, as this file is made an artifact in the draft release step. I was able to install it from test.pypi, so that is not a problem, I guess it's just my understanding that is lacking.

@torik42
Copy link
Owner

torik42 commented Nov 21, 2022

No, that’s my fault. I deleted all releases at some point (there were only two which were indeed released). They would be on the release page where I still see two draft releases, but you can’t see them. The assets are not listed on the tagged commits which you linked to. Even the .tar.gz is not the .tar.gz which will be available on PyPI, but just the full GitHub repo as a .tar.gz.

@JulianGoeltz
Copy link
Contributor Author

Ok, then I guess everything is correct and you can submit. Thanks for the explanation.

@torik42 torik42 closed this in c69d634 Nov 22, 2022
@torik42 torik42 merged commit c69d634 into torik42:master Nov 22, 2022
@JulianGoeltz JulianGoeltz deleted the feature_testPyPiInstall branch January 16, 2023 18:38
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