Automatic package release should start from the state of the repo that was reviewed

[dontcallmedom]

as discussed in #144, there is a risk that the diff being reviewed in the PR that bumps the package version no longer match the state of the repo when that PR is merged.

The GH action that does the release should start from the state of the repo at the time of the diff.

We could for instance tag the repo as pre-x.y.z when generating the diff, and use that tag for the checkout in the release action.

[foolip]

This would suggest actually tagging the very branch that was pushed as a PR. But then the version bumps would always be on a branch and need to be duplicated on master somehow.

One option is to bump the version right after a release. This is how versioning in Chromium works, the version in the tree is the version that current changes will be included in, not the version of an already cut release.

[dontcallmedom]

hmm... you're right on the problem of tagging master with that approach, I had not gone fully through the steps.

I'm not sure I understand how the tag-at-release time would help if we keep the manual diff review process though - if master changes between the time of the diff & the release, there is no longer a state where master can be tagged with the version number and the expected state of the repo?

[foolip]

What we'd end up doing is actually to tag a commit already on master, and then bump the version after that. The tagged commit isn't guaranteed to be the last with that version however.

What I'm suggesting doesn't work well as a PR workflow though.

Maybe release from a branch and merge that into master with a merge commit? Not sure how to enforce that though.

[dontcallmedom]

I guess if we want to ensure a reliable way to retrieve the state of the repo that matches what get shipped, we probably want to have two stable branches: master (to be renamed main) which reflects the latest crawl, and something like releases that gets updated only with what ships in released packages and where the relevant tags would live.

[saschanaz]

Would "Require branches to be up to date before merging" GitHub setting prevent the state issue? Merge commits in the PR branch would then trigger test again to make sure things are ok. (Although the initial commit by GH Action somehow doesn't trigger CI, any following commits will. Actually the initial commit can also trigger CI after closing-reopening the PR.)

[foolip]

@saschanaz I think that would avoid the problem, yeah. If changes do happen on the main branch, some mechanism to recreate the PR is needed, and I'm not sure how to do that.

[tidoust]

Would "Require branches to be up to date before merging" GitHub setting prevent the state issue?

Where is that setting? Is it the "Require linear history" branch protection rule?

If changes do happen on the main branch, some mechanism to recreate the PR is needed, and I'm not sure how to do that.

It should be relatively straightforward in a workflow to find the open PR using the GitHub API, and to re-create it. I'm not crazy about the noise that this is likely to generate in the closed PR list, but that seems doable. Perhaps the job could rebase the PR and update the diff directly instead of closing/reopening.

Alternatively:

What we'd end up doing is actually to tag a commit already on master, and then bump the version after that. The tagged commit isn't guaranteed to be the last with that version however.

What I'm suggesting doesn't work well as a PR workflow though.

We don't necessarily need a PR, but we do need something like a yes/no question. That can be achieved with an issue, e.g. "add a label to say yes then close, otherwise just close", but that's a bit tedious. The good thing about a PR is that it gives that out of the box.

An adjusted workflow could be:

1. Version in packages/idl/package.json represents the version that will be released.
2. The job that runs every Thursday creates a PR that bumps the version in packages/idl/package.json, details the expected diff in the body of the PR description, and creates a pre-release tag on the latest commit on master at the time when the PR is created.
3. One of us reviews the PR, checks the diff, checks the need to release a minor / major version instead:
   • If any change is needed, the PR needs to be closed, and the job needs to run again after needed changes have been pushed to master.
   • If all looks good, the PR can be merged.
4. When the PR is merged, the release job takes over. It reacts to the PR merge, looks for the latest prerelease tag on master, releases the package at that commit, and also adds a proper release tag at that commit. The version number that gets published is not the bumped one but that is by design.

One possible hiccup: if the release job fails for some reason, we end up in a situation where the version has already been bumped on master but package has not been released.

[saschanaz]

Where is that setting? Is it the "Require linear history" branch protection rule?

It's below that rule"Require status checks to pass before merging", if you check it then it will appear.

[tidoust]

One remaining bug tracked in #244, and a few possible improvements tracked in #226, #232, #237 but new workflows are now in place and documented in the README. New workflows guarantee that the package released is the package that was reviewed.

Closing this issue accordingly.