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

ci: automate releases, add check for py3.7, and create pr checklist #201

Merged
merged 40 commits into from
Jan 16, 2024

Conversation

aryarm
Copy link
Member

@aryarm aryarm commented Dec 19, 2023

This PR lets us automate the process of releasing new versions using Google's Release Please Github action. It works by creating (and maintaining) a "release PR" which, when merged, will update the changelog, bump the version number, and upload our source distribution and wheel to PyPI.

how is the version number determined?

The version number is determined automatically according to 1) semantic versioning and 2) the titles of each PR (which should follow the conventional commits spec). For example, if all of the recently merged PRs are prefixed with fix: , then the system will infer that all recent changes have been bug fixes, so it will only bump the "patch" version. However, if even one of the recently merged PRs is prefixed with feat: , then the system will bump the "major" version because it will infer that a new feature has been added to master.
This makes our versioning more interpretable to our users while also redistributing the work of maintaining the version onto each developer of our toolkit.

how is the changelog maintained?

The changelog for each release is simply the contents of the text body of the release PR. These text entries will themselves come from the titles of each of the PRs that have been merged since the last release. For example, this is what the changelog entry would look like if the following two PRs were merged into master since the last release:

  • fix: NoneType error in Haplotypes.__iter__
  • fix: precision of phenotypes written to pheno file

Here is also a more recent example.

some caveats

  1. we will need to require that PRs get squashed in order for this to work. imo this makes the git history cleaner and easier to follow, anyway
  2. every PR that gets merged into master will have to have a prefix attached to it's title that follows the conventional commits spec. For example, if the PR is a bug fix, it will need to be prefixed by fix: . Or, if it is a new feature, it will need to prefixed by feat: . I've added another Github action to prevent merging of PRs that don't have this
  3. I had to convert the RELEASE_NOTES.rst file into a CHANGELOG.md file because the release please github action only supports markdown. This meant that I also had to remove the file from our documentation, but I could try to figure out if there's a way to still link to it, if we'd like

other, somewhat unrelated, things in this PR

  • adds py3.7 testing to the CI! hoorah! 🎉 I actually got it to work
  • uses "stable" instead of "latest" to refer to docs (in 94b39a5), since "latest" may refer to unreleased code that is sitting on master
  • moves configuration in pytest.ini and .pylintrc into the pyproject.toml

since they must be md

if we want, we could keep them by using and installing mdinclude (https://sphinx-mdinclude.omnilib.dev/en/latest/example.html)
since latest may refer to code that's still in development
my strategy is to use whatever was the current version at the time the package was added as a dependency
since they must be md

if we want, we could keep them by using and installing mdinclude (https://sphinx-mdinclude.omnilib.dev/en/latest/example.html)
since latest may refer to code that's still in development
my strategy is to use whatever was the current version at the time the package was added as a dependency
@aryarm aryarm marked this pull request as ready for review December 20, 2023 21:13
@aryarm aryarm changed the title build: automate releases ci: automate releases Dec 22, 2023
@aryarm aryarm marked this pull request as draft December 22, 2023 18:37
@aryarm aryarm marked this pull request as ready for review January 2, 2024 05:24
@LiterallyUniqueLogin
Copy link
Contributor

Hey Arya, thanks for doing this!

Some minorish points:

I personally liked having a detailed change log, explaining everything that's happening. Having it just be one-liners that link to PRs means you have to go digging to figure out what actually changed. Is there a way to copy the bodies of the PRs into the changelog in addition to the one line summaries?

Regardless of the above, having PRs be self documenting for the change log is a big change for my own PR style and presumably a change for many other lab members. Normally I write them either sloppily, not thinking they'll be looked at much, or I write them in a detailed way for the reviewer. Now I'll need to write them for an unknown future audience with little presumed background so they can understand it as part of the changelog history. If we're making this change, we need these new requirements of PR etiquette to be very clearly stated somewhere in the TRTools documentation, and we should make a brief announcement to the lab that everyone will need to change their PR style. Wherever we document this, we also need to state that in a PR, if the if the scope of the PR changes, the original PR commit message will need to be edited to be kept up to date. People shouldn't have to read through the conversation history of a PR to understand what happened, and now they can't look to a manually written changelog for that purpose.

Between steps 2 & 3 in PUBLISHING.rst there should be a description of what the person doing the review should be looking for. I would at least have them make sure that the changes have been documented appropriately in that PR. I'm open to having other review tasks their as well if you can think of some.

Instead of deleting the release notes page, can we just have it consist of a single line link to the CHANGELOG on the github repo?

@aryarm
Copy link
Member Author

aryarm commented Jan 3, 2024

Thanks for writing out all of your thoughts, @LiterallyUniqueLogin !

I agree that one liners aren't ideal because they require extra digging. I don't know if there's a way to include the entire PR body in the release notes, but one thing that I forgot to mention is that you can edit the contents of the text body of the release PR before it gets merged. And whatever you include there will get included in the CHANGELOG. I haven't used this (undocumented?) feature yet myself, but you can see an example of it in the release PR and release notes for Snakemake 8.0, which also uses the release-please GitHub action. So we can still manually write the CHANGELOG, if we'd like.

Regarding your broader point about PR etiquette:
I think a lot of projects have pull request templates to help encourage PR etiquette. You can include checklists and guidelines for the person writing the PR. For examples, here are snakemake's and haptools'. Maybe we could create one for TRTools? That might be a better way to target our communication to people who make PRs for TRTools?

I'm glad you also brought up guidelines for reviewers because I often struggle to identify the best way to review a PR myself. I'll try to add a section to the TRTools docs with a list of suggestions for PR reviewers, but I would feel better if there was also some input about all of this from others in the lab, perhaps on our Slack?

I also think it would be nice to have some documentation, perhaps on the lab wiki?, that lists some general suggestions for pull request creators and editors, since a lot of this will also apply to haptools and other projects in our lab. What do you think?

@LiterallyUniqueLogin
Copy link
Contributor

All good points! You really know your way around github best practices, that's a very useful skill. How about this then,

Three more changes for this PR:

  • Can you create a PR template for TRTools. It should mention
    • the conventional commits spec, maybe a brief example
    • The PR should be legible for unknown future audience with little presumed background that are trying to understand TRTool's changelog history
    • The PR author should keep the main PR title/body up to date if the scope of the PR changes
    • Any other points you find helpful (especially from your Haptools experience).
  • a short blurb in the TRTools contributing section saying PR reviewers should be looking to check that the PR conforms to the template guidelines in their reviewing (or edit the PR into line themselves), in addition to reviewing the code
  • a short blurb in the publishing section saying that the publisher should edit the auto-generated text-body of the PR release to incorporate additional details from the underlying PRs as needed.

and then this PR should be good to go.

Outside of this PR, we can chat with Melissa and decide if we want lab-wide PR creation/review guidelines, and if so, do we want to write them up for the lab wiki and/or solicit feedback.

@aryarm
Copy link
Member Author

aryarm commented Jan 7, 2024

done! thank you for all of the comments and suggestions, @LiterallyUniqueLogin !

And regarding lab-wide PR creation/review guidelines: your plan sounds good to me

@aryarm aryarm changed the title ci: automate releases ci: automate releases, add check for py3.7, and add pull request checklist Jan 8, 2024
@aryarm aryarm changed the title ci: automate releases, add check for py3.7, and add pull request checklist ci: automate releases, add check for py3.7, and create pr checklist Jan 8, 2024
@LiterallyUniqueLogin LiterallyUniqueLogin merged commit 1d61fd1 into master Jan 16, 2024
6 checks passed
@LiterallyUniqueLogin LiterallyUniqueLogin deleted the build/automated-releases branch January 16, 2024 22:08
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