Enforce rollback safety PR checks - #5731
Conversation
This reverts commit 2c10c76.
…rgo-ci-merge-5687 # Conflicts: # tools/ci/README.md # tools/ci/src/main.rs
Signed-off-by: Zeke Foppa <196249+bfops@users.noreply.github.com>
| echo "This PR is not based on master. Please wait until the base PR merges." | ||
| exit 1 | ||
|
|
||
| release_dependencies: |
There was a problem hiding this comment.
this is an odd place to add this job, but this is kind of where we have put job(s) that depend on PR metadata rather than just commit contents. Thoughts? ci.yml doesn't run on PR edit events, but this workflow does
Signed-off-by: Zeke Foppa <196249+bfops@users.noreply.github.com>
| on: | ||
| pull_request: | ||
| types: [opened, edited] | ||
| types: [opened, reopened, edited] |
There was a problem hiding this comment.
unrelated fix: run properly on reopened PRs
…ub.com:clockworklabs/SpacetimeDB into bfops/rollback-safety
|
|
||
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.event.inputs.pr_number || format('sha-{0}', github.sha) }} | ||
| cancel-in-progress: true |
There was a problem hiding this comment.
unrelated fix: have later jobs cancel previous ones, e.g. if you edit the PR description several times quickly
… into bfops/rollback-safety
… into bfops/rollback-safety
|
|
||
| # Must be released | ||
|
|
||
| <!-- List PRs that must be included in a release before this PR can merge. Supported forms include #123, SpacetimeDB#123, clockworklabs/SpacetimeDB#123, and GitHub PR URLs. Leave this section empty if there are none. --> |
There was a problem hiding this comment.
Rather than "Leave this section empty if there are none," I would rather have, "include 'n/a', in this section if this PR is safe to roll back after deployment and has no impact on the rollback-safety of any prior PRs," and recognize the case where that string (case-insensitive, preferably) appears in this section outside of a comment. I want to reject any PR where the author doesn't demonstrate that they've thought about this.
There was a problem hiding this comment.
I'd also like this description to be more clear that the burden is not just that the linked PR must be released before this can merge, but also that this PR cannot be deployed until the linked PRs:
- Are deployed.
- Have been sufficiently observed and/or tested in the live deployment that we are confident they will never roll back.
It would be nice to have verbiage about the types of changes that may impact rollback safety, namely:
- If this PR newly writes to a ControlDB table or reducer, system table, or on-disk data format that was previously introduced but unused.
- If this PR clears, deletes, incompatibly changes, or renders unsupported a ControlDB table or reducer, system table, or on-disk data format that was previously present or available.
(The ControlDB stuff is only relevant for PRs to the private repo, but I think it's fine to list here and it'll make things easier if we use the same template in both places.
There was a problem hiding this comment.
That sounds great. What do you think of this version?
There was a problem hiding this comment.
4 participants
Description of Changes
This is the first PR in what will be a small series of PRs geared at enforcing our strategy for rollback-safe deploys.
Updates the PR template with a new section for noting PRs that must be released before the current PR can merge. We also add a CI check that this field has been updated, and to enforce that those PRs have actually been released.
Also adds a separate utility for finding what release included a particular PR. This is based on commit subject lines, so it can technically be spoofed.
API and ABI breaking changes
None
Expected complexity level and risk
2
Rollback safety impact
n/a
Testing