opensafely-core / research-template-docker

This provides the devcontainer Docker image used by repos created from the OpenSAFELY research template.
Other
0 stars 0 forks source link

The `publish` action should increment the minor version component #16

Open iaindillingham opened 2 months ago

iaindillingham commented 2 months ago

At present, the publish action tags a new image as "v0", meaning that "v0" can resolve to different images at different times.

https://github.com/opensafely-core/research-template-docker/blob/6ec03c172fecf27665e6e1059e11c0a44a59648d/.github/workflows/main.yml#L70-L76

Semver considers using major version zero for initial development to be optional:

Major version zero (0.y.z) is for initial development. Anything MAY change at any time. The public API SHOULD NOT be considered stable.

However, it also considers modifying the package's contents after the package is released to be an absolute prohibition:

Once a versioned package has been released, the contents of that version MUST NOT be modified. Any modifications MUST be released as a new version.

So, whilst we are doing the right thing by using major version zero 🙂, we are doing the wrong thing by repeatedly tagging new images as "v0" 🙁. Instead, we should be incrementing the minor version component.

We took a similar approach when developing ehrQL. A user specified "v0" in their project.yaml, with the minor version component resolving to the most recent available release in the execution context (the local context tended to lag the secure backend context). The inconvenience of a backwards-incompatible change was deferred by specifying the minor version component. The user was expected to refactor at a more convenient time, as the most recent available release in the secure backend context really was the most recent available release overall.

lucyb commented 1 month ago

At present, the publish action -- which is run on dispatch, on PR, and on push to main

Oh no, I wasn't expecting that to be the case. I was intending for the publish action to only run when something was merged to main. I thought this line would do that:

if: github.ref == 'refs/heads/main'

Do we need to push any images if they're on a branch? I can see how that might be useful, but it adds quite a bit of complexity. Maybe it would be better to fix the publish action, so that it does what I originally intended instead.

iaindillingham commented 1 month ago

Sorry for the confusion, @lucyb. You are correct! I will remove "which is run on dispatch, on PR, and on push to main" from the description. However, the issue I'd intended to raise is that one user's "v0" won't necessarily be another user's "v0" because dev containers can live for 14 days' when run in Codespaces and indefinitely when run locally.

This is undesirable from our perspective because it breaks semver. In other words, this is undesirable because it violates the principle of least surprise. This is also undesirable from a user's perspective, because the user can't pin the version of a dev container. (We should recognise that whilst it makes our life easier if they don't do so, it won't make their life easier.)

lucyb commented 1 month ago

I'm taking this off the board while we discuss it.

we are doing the wrong thing by repeatedly tagging new images as "v0" 🙁. Instead, we should be incrementing the minor version component.

I disagree. Tagging new images as "v0" shows they are the latest version of that major version. This is desirable because users will get the latest version without having to make any changes.

However, I do think we need to tag images with the minor version as well. This would match up with the versioning proposal document. This would allow users to pin to an unchanging minor version if they wanted.

Is part of the problem that we're using "v0"? Should we actually be moving to v1 at this point?

dev containers can live for 14 days' when run in Codespaces

Just to clarify, a Codespace can live for longer than 14 days. They are automatically deleted when they've been inactive for 14 days. I'm unsure what changes are pulled in when a Codespace is restarted though, which will happen more frequently (at least daily, I imagine).

iaindillingham commented 1 month ago

Thanks, @lucyb.

I think that we've been circling around the implementation of a version and the concept of a version. I agree that we should tag each image with its major and minor components. For example, by executing both docker tag ...:v0 and docker tag ...:v0.1. If we do, then a user can access the version of the image that's important to them. Here, "tagging" is how we implement the version. Conceptually, however, we should identify an image by its major and minor components. At present "v0" could mean "v0.1", "v0.2", or "v0.k"

I do not think we should be using "v1" at present. "v0" gives us flexibility. If we move to "v1", then we need to be very careful not to break backwards compatibility. I don't think our tests are comprehensive enough at present.

StevenMaude commented 1 month ago

One consideration here I just noticed. As things stand, as I understand it, any merge to main results in the publication of a new image, regardless of whether anything changed in the image or not.

It might be that the image hasn't actually changed since the last publication; for example, editing the tests, which would give an incremented vX.Y.Z version number that's potentially confusing when nothing has changed in the image.

iaindillingham commented 1 month ago

I don't think there's anything wrong with incrementing the patch version under that scenario. It means you don't have to remember to use a conventional commit, right after you forget to use a conventional commit.