Cache activity before opening retirement PRs - #120
Conversation
MattSturgeon
left a comment
MattSturgeon
left a comment
There was a problem hiding this comment.
Awesome!
I won't pretend to have fully digested the bash impl, so don't be offended if I spot something worth commenting on later.
SGTM overall, though. Nice idea to mitigate GitHub's flakiness!
|
Every one of your comments makes me want to rewrite it in github-script, @MattSturgeon :-) Thanks for taking a look. I'll cogitate on these comments. |
What does it do? In short, we persist the latest observed merge activity and the first inactive observation between workflow runs using a GitHub workflow artifact. A missing `/commits` response now starts a confirmation window instead of immediately opening a retirement PR. The result is that cached positive activity can keep active committers from being retired when GitHub returns an empty result spuriously. Fixes NixOS#100
|
I rewrote it in GitHub script. |
| In CI, `retire.js` runs via [`actions/github-script`](https://github.com/actions/github-script). | ||
| Locally it can be run with `scripts/retire-local.js`, which needs Node.js (`pkgs.nodejs`) and uses the `gh` CLI for API access instead. |
There was a problem hiding this comment.
Could we add a nix-shell shebang to require-local.js, to allow users to run it without nodejs dependency setup? Or a separate shell.nix?
They'd still need to configure gh auth, though.
| // The artifact download endpoint always serves a zip archive | ||
| const zip = ( | ||
| await github.request(`GET /repos/${owner}/${repo}/actions/artifacts/${artifact.id}/zip`) | ||
| ).data; | ||
| fs.mkdirSync(dir, { recursive: true }); | ||
| const zipPath = path.join(dir, `${artifactName}.zip`); | ||
| fs.writeFileSync(zipPath, Buffer.from(zip)); | ||
| execFileSync('unzip', ['-o', zipPath, '-d', dir], { stdio: 'inherit' }); | ||
| fs.rmSync(zipPath); | ||
| core.info(`Restored activity cache from run ${runId}`); |
There was a problem hiding this comment.
Uploading with archive: false would significantly simplify this bit.
(I don't have permission to unresolve #120 (comment), but see my reply there)
| @@ -0,0 +1,74 @@ | |||
| #!/usr/bin/env node | |||
| // Local runner for scripts/retire.js, providing an Octokit-compatible shim | |||
There was a problem hiding this comment.
| // Local runner for scripts/retire.js, providing an Octokit-compatible shim | |
| // Local runner for scripts/retire.js, providing an Octokit shim |
Let's avoid implying the shim is fully compatible, since it only covers the subset we use.
| return Math.floor(absolute / 1000); | ||
| } | ||
|
|
||
| const d = new Date(); |
There was a problem hiding this comment.
Minor correctness issue: each time we parade a date we'll be doing so relative to a different now epoch.
Since we're dealing with weeks and months, a few milliseconds are unlikely to cause real issues. But ideally, we'd have a way to synchronize now across multiple calls. E.g., with an input parameter:
function parseDate(spec, now = Date.now())2 participants
I hate those "whoops" issues that #100 creates. Here's one kind of dumb take on fixing it.
Sorry for the bash overload. I didn't want to rewrite this in github script first.I rewrote the thing in GitHub script.
What does it do? In short, we persist the latest observed merge activity and the first inactive observation between workflow runs using a GitHub workflow artifact. A missing
/commitsresponse now starts a confirmation window instead of immediately opening a retirement PR. The result is that cached positive activity can keep active committers from being retired when GitHub returns an empty result spuriously.I don't really love the resulting code. I'd sort of like to rewrite the whole thing.I rewrote the whole thing.
Fixes #100