search

opensafely-core / interactive-templates

Code to generate the reports generated by OpenSAFELY Interactive
Other
0 stars 0 forks source link

Build broken #363

Closed lucyb closed 2 months ago

lucyb commented 3 months ago

The build is broken on main and has been for a little while.

It's failing on the tag new release step, although it's unclear why

To https://github.com/opensafely-core/interactive-templates
 ! [rejected]        v2023.09.12.100407 -> v2023.09.12.100407 (already exists)
error: failed to push some refs to 'https://github.com/opensafely-core/interactive-templates'
hint: Updates were rejected because the tag already exists in the remote.
Error: Process completed with exit code 1.
lucyb commented 3 months ago

I am uncertain whether this is related or not, but it's worth being aware of just in case. https://github.com/opensafely-core/interactive-templates/issues/118

StevenMaude commented 3 months ago

From a very quick look, as I understand it, I think this is actually behaving as expected, although it's quirky, and may not be what we want.

My best guess (edited several times as my understanding improved)

  1. the tag-new-release step looks at the version file
  2. if that file is unchanged, then git tag uses the existing content of the version file as a tag
  3. the version file hasn't been edited since the previous time a tag was successful, that tag specified in version already exists in the full repository; however, it doesn't exist in the sparse checkout that GitHub Actions has carried out, so we end up tagging with the same version but on a more recent commit
  4. we try to push all tags and it thankfully fails because the tag already exists

I think the quick fix would be to run the checkout action to fetch the whole repository instead with fetch-depth: 0.

Is such a fix sufficient to address this issue, or do we also want a better way of updating than bumping the version file?

StevenMaude commented 3 months ago

I could also bump the version as part of this issue, which I can do afterwards. That would trigger a new release.

~Alternatively, we might want the route to release an update, being more like other repositories that we manage (which would be the "check for conventional commit message" way).~

lucyb commented 3 months ago

Thanks for digging into this. I understand your point about it being a bit quirky. Would using conventional commits allow us to easily tag a new release in the same way we're doing now? Job Server should be picking up any new tags and raising a PR for us, and ideally we'd want that to stay the same.

Also, Louis is one of the main committers to this repo. How would he feel about using conventional commits? I suspect he'd be fine, but we'd have to tell him about the change.

StevenMaude commented 3 months ago

@lucyb: Ah, looking around I didn't have the full context here.

The pipeline repository works in the same way with a version file.

While there's an job-server ADR for having this interactive-templates, it's not clear why the tags are date-based, and opposed to a sequential version, and I can't easily find out why. Maybe recording the date is useful in some way?

I'd add that the "conventional commits" system we have elsewhere is also quirky. Just that I'm more familiar with it since I'd seen it in the ehrQL repository and elsewhere.

So, rather than get embroiled in that, I'll propose that we:

  1. fix the build as stated above, by merging the PR
  2. update the version file to trigger a new release
  3. verify that all is well with the fix
  4. document the version bump process in the developer notes or README

but leave the versioning process alone for now.

If the specifics of handling updates are annoying enough, then we can open a new issue for that, but it's probably fine as these are probably infrequent.

Jongmassey commented 3 months ago

I just started looking at this because my actions failed for this reason. I think I agree with Steve's suggested solution

StevenMaude commented 3 months ago

I'll create a PR with just release to trigger a new version once the other two PRs are merged.

StevenMaude commented 3 months ago

Actually, I'll wait until #364 is merged as it looks nearly done, and then release.

There's also #368, but that might take a little longer to unpick.

StevenMaude commented 2 months ago

Reopening as the automatic PR to job-server failed. I'll take a look.

StevenMaude commented 2 months ago

opensafely-core/job-server#4437 is related to this.

StevenMaude commented 2 months ago

The build's fine; it's the creating PR workflow that's an issue now.

StevenMaude commented 2 months ago

Proposal to move the PR workflow to job-server: https://github.com/opensafely-core/job-server/pull/4442 — it means the PRs are not created on immediate release of a new version here, but does remove the use of a PAT, and is how things work for the ehrQL repository.

StevenMaude commented 2 months ago

Once opensafely-core/job-server#4456 is merged, that part is complete. The workflow there works.

What's left in this repository: