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

Automated publishing to PyPI and GitHub Releases on PR to release branch #26

Merged
merged 8 commits into from
Oct 18, 2023

Conversation

AdamJCrawford
Copy link
Member

No description provided.

@nickrsan
Copy link
Contributor

Hey Adam,

This looks good. A few thoughts:

  • It looks like this builds the distribution on every push to the release branch. I think that makes sense in many ways, but also could mean we're not do anything with lots of builds (everything untagged). I'm wondering if we should limit the scope of the action to tags that start with release- . I think we can do that, but haven't found the doc yet. Otherwise, is there a good reason to always build on push?
  • Gut check - good idea or bad idea? I'm wondering about this workflow. When you create a release on GitHub, you can have it simultaneously add a tag. If we create a release with the tag release-2023.09.25 I'd love if the workflow ran (starts with release- and then edits its own copy of the version values in init.py and in setup.cfg to be the value in the tag before it builds and uploads them. That way we know we got the version inserted and consistent (assuming the pipeline isn't brittle). Is this a bad idea? What do you think of that?
  • Should we even be triggering builds from release tagging, or should it go the other way that any push to the release branch should be ready to be packaged and uploaded? We'd want to put even tighter guardrails on it then, such as forcing a check for new version assignment. This might be the smarter way. Then we build and upload and automatically tag a release, which is something I've done on other repositories (trigger a release with artifacts on push to release branch)

@AdamJCrawford
Copy link
Member Author

AdamJCrawford commented Sep 26, 2023

What you your thoughts on the release branch? Is it the case they every time something is pushed/merged into release, PyPI should be updated and use the development branch as the place for intermediate changes? Or will there be time that things are pushed to the release branch but PyPI doesn't need to be updated? If we want PyPI to be the same as the release branch then building on every push seems reasonable. We could also just move the build call into the block that handles the tag/PyPI stuff. In case you were wondering I got the original action from: https://packaging.python.org/en/latest/guides/publishing-package-distribution-releases-using-github-actions-ci-cd-workflows/ at the bottom there is an arrow to "... display the entire GitHub Actions CI/CD workflow definition."

I think the idea of automatic versioning based on the tag is a great idea. No idea how to do that currently, but that could save us a decent amount of time and many errors with the version number.

Why are we using the date as a version number? Do you like it because you immediately know when the code was written vs a seemingly random set of numbers that other projects use: 1.4.21.15?

Edit:
Another thought: Might be good to have the build step run even if we don't want to push to PyPI to make sure a change didn't break the build process.

@nickrsan
Copy link
Contributor

I meant to also link to our item that automatically creates a build and publishes it as a GitHub release too: https://github.com/Water-Systems-Management-UCM/Stormchaser/blob/release/.github/workflows/build_to_release.yml - not perfect, but in case it's useful.

One other item we should consider is if we can make this action wait to run until after all the tests have completed. Theoretically nothing gets to the release branch unless tests pass, but just in case? We could also just strengthen branch protections if there's no way to make this action only run after the others complete.

So, I'm wondering if maybe rather than having it based on manual tagging of releases, we instead have something as part of our pull request actions that checks to make sure the version is greater than the version available on PyPI? I don't know - I'm going around in circles on this.

To answer your question. I like date based because it's simple and it makes clear to people that it doesn't mean anything other than the date it was published. We should probably prefix it with a leading 0. and remove any already published items from PyPI so that if we want to switch off of it to something like Semantic Versioning, that's possible. But I feel like I see so many projects try to make semantic versioning work, and it often doesn't make sense to me unless the project is big and you need to broadcast major breaking changes. So we could do that, but we'd also maybe want to get more careful at some point about version numbering, and we can handle deprecations without it, etc.

And I wondered about that same thing you mentioned about builds running regardless. I think that's sensible.

@nickrsan
Copy link
Contributor

nickrsan commented Sep 27, 2023

Also, please add me as an Owner on PyPI when you get a chance: https://pypi.org/manage/project/eedl/collaboration/ - my PyPI username is nicksan (not the same as my GitHub)

@nickrsan
Copy link
Contributor

Thanks for the quick action on that. I just deleted the existing release so that we don't get version issues if we change version schemes. I'm going to prefix the versions with a 0 right now so that we can always opt for another versioning scheme later by going to version 1.0.

@nickrsan
Copy link
Contributor

I'm thinking more and more that we shouldn't do the method I discussed about only releasing when a specific tag is added. That seems more complicated and brittle. I think we should release whenever a commit is merged to the release branch - that should then upload a build/release to GitHub, a build/release to PyPI, and a build/release to conda-forge (when we get there - I don't see that as a blocker for this PR). Does that sound good to you?

Then, we can strengthen any checks we need for what gets released by ensuring it's all run as an action, and we prevent committing directly to release (big no-no - and maybe the upload script only runs on PRs to release to ensure that code committed directly doesn't make it)

@nickrsan nickrsan changed the title Automated publishing to PyPI when a tag is added. Automated publishing to PyPI and GitHub Releases on PR to release branch Oct 2, 2023
@nickrsan
Copy link
Contributor

nickrsan commented Oct 2, 2023

We've settled on a strategy:

  • Add a new main branch that will be the defaults for documentation builds and GitHub action definitions (Nick)
  • Publish a new release to PyPI whenever anything is merged from a PR to the release branch (Adam)
  • Publish a new release to GitHub Releases at the same time (Adam)
  • Add branch protection rules for both main and release branches, require version bumps only for release branch (Nick)

We agree to keep using date-based version strings for now.

We want to publish released packages whenever merging to the release branch, but not when we make changes to t
he documentation or other repository/developer specific features. This strategy accommodates that.

@nickrsan
Copy link
Contributor

Hey Adam - this looks good overall so far.

My only comment is to ask whether you can turn off the part of your code reformatting that is automatically capitalizing the first letter of comments. I don't think it adds to clarity, and it's going to make any tracing of bugs a pain by messing up git blames. If you think it is helpful, then we can set a style guide and I can add that to my own, but I don't think that's necessary unless you do.

@nickrsan nickrsan self-assigned this Oct 18, 2023
@nickrsan nickrsan added the enhancement New feature or request label Oct 18, 2023
@nickrsan nickrsan merged commit 68edb1d into release Oct 18, 2023
14 checks passed
@nickrsan nickrsan deleted the Adam_Test_Packaging branch October 18, 2023 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants