opensafely-core / cohort-extractor

Cohort extractor tool which can generate dummy data, or real data against OpenSAFELY-compliant research databases
Other
38 stars 13 forks source link

Automate version bumping #179

Closed evansd closed 4 years ago

evansd commented 4 years ago

Remembering to update the VERSION file is a bit fiddly and also has the potential for conflicts when multiple PRs are in flight.

The PR description could contain a magic string e.g. semver: patch or semver: major (first thing that came into my head, might be much better options) to indicate the level of change and then the action could automatically determine the new version number based on the previous number and the levels of changes since.

A test could enforce that every PR has such a piece of text somewhere in the description.

A quick search indicates I am clearly not the first person to have thought of this.

sebbacon commented 4 years ago

Some options.

This GH action uses conventional commits. In short, a commit with a subject prefixed fix: will increase the patch number, feat: will increase the minor number, and a commit with BREAKING: in the footer increases the major number. However, this appears to be tied to npm in some way (expects a package.json which can be updated)

This action is similar, but without any npm assumptions.

This action is closer to @evansd 's suggestion. We can configure it such on push (or merge) to master, the action will increment the latest tag based on the highest of #major',#minorand#patch` in any of the commits in the PR.

This is almost identical.

sebbacon commented 4 years ago

My inclination is for number two. If we want to use semver, then it makes sense to invoke version bumps via semantic commits. I would propose we require the type prefix for every commit, and not rely on any default bumping behaviour.

From its documentation, here is an example of the release type that will be done based on a commit messages:

Commit message Release type
fix(pencil): stop graphite breaking when too much pressure applied Patch Release
feat(pencil): add 'graphiteWidth' option Minor Release
perf(pencil): remove graphiteWidth option

BREAKING CHANGE: The graphiteWidth option has been removed.
The default graphite width of 10mm is always used for performance reasons.
Major Release
inglesp commented 4 years ago

From the GitHub Tag Action README I can't see anything about how you require a prefix. Would we enforce with with a pre-commit hook?

It's also not clear from the README what indicates a patch/minor/major release -- I can guess, but it'd be good to know for sure.

sebbacon commented 4 years ago

Re the former, how about this app: image

Re the latter, what's not clear from the table I reproduced about? it's fix/feat/BREAKING CHANGE no?

evansd commented 4 years ago

I think I'm -1 on Conventional Commits at the moment, or at least I'd need some convincing. It's not a totally transparent and obvious syntax, and commit titles are already quite limited in terms of characters if you're trying to be expressive and I don't really want to be forced to waste characters.

The app referenced above does allow you to only include the status in the PR title, but then it wants to squash the PR so you lose the individual commits which doesn't seem great to me.

evansd commented 4 years ago

Git already has a convention around "trailers" which are key-value pairs in (roughly) email header format which sit at the end of the commit message. There's built in tooling to support these. And some pre-existing conventions within the kernel team.

These feel to me like the right way of flagging the semver level of PRs or commits, rather than using the title.

sebbacon commented 4 years ago

So I take your point about losing 4 chars from a title.

Less convinced it's not transparent or obvious: it's well-documented and there are literally only three conventions we'd need to use for patch, minor and major.

I would be happy to use the trailers instead (which is what's used by the BREAKING CHANGE convention) but I'm less happy about having to maintain our own github actions fork to support it!

sebbacon commented 4 years ago

So to summarise I share your dislike for the commit-title approach, but I do like a semantic approach, and I don't want to write my own parser. In the absence of an existing alternative I prefer I'd personally be happy to try this approach, but entirely open to either (a) deciding this isn't sufficiently better than the status quo to bother with, or (b) finding a different, already-implemented approach we like.

evansd commented 4 years ago

Fair enough, I'm happy with that. I guess the major change then is removing the VERSION file from the repo and having the build-and-publish action create it based on the value on the tag.

sebbacon commented 4 years ago

Yes it's

sebbacon commented 4 years ago

Oh wait - how would an installed cohortextractor know what version it was?!

sebbacon commented 4 years ago

Aha https://pypi.org/project/setuptools-scm/

evansd commented 4 years ago

I was thinking that an appropriate VERSION file would be created as part of the publish action, just before the package gets built and then pushed up to PyPI.

setuptools-scm looks good but possibly a little heavyweight and my heart sinks when I start reading things like the below: pypi org_project_setuptools-scm_

(Not a comment on this tool, just the state of Python packaging generally)