CLI installer doesn't work for failed PR builds that still have artifacts

[victorlin]

Current Behavior

Build artifacts for https://github.com/nextstrain/cli/pull/333 aren't downloadable via the /cli/download/pr-build endpoint, but are via the /cli/download/ci-build endpoint. This is because:

1. The PR's CI run failed but only after uploading artifacts.
2. The /cli/download/ci-build endpoint simply downloads the build artifacts if they are available, without checking if the run was successful. The /cli/download/pr-build endpoint checks that the run is successful: https://github.com/nextstrain/nextstrain.org/blob/bd9846a51cfd33f8c2606dd6890bffdfc06baf45/src/endpoints/cli.js#L54-L61

Expected behavior

The /cli/download/pr-build endpoint should allow downloads as long as the artifact is available, even if the run was not successful.

Possible solution

Change or remove the status: "success" search param.

[tsibley]

Ah, hmm. I think we want to change status: "success" to status: "completed", which includes both status: "success" and status: "failure" (and maybe one or two other run conclusions).

The API we're querying uses the status query param to select on a union of the status and conclusion fields of a workflow run record. success is a conclusion value. completed is a status value.

[tsibley]

Here's a patch I whipped up…

From 3735bac7a27e7a880f2bc3b5e8392dc5d3bbf73e Mon Sep 17 00:00:00 2001
From: Thomas Sibley <tsibley@fredhutch.org>
Date: Thu, 11 Jan 2024 14:59:24 -0800
Subject: [PATCH] =?UTF-8?q?endpoints/cli:=20Pick=20the=20last=20=5Fcomplet?=
 =?UTF-8?q?ed=5F=20CI=20run=20for=20a=20PR=E2=80=A6?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

…not the last _successful_ run.  Failed CI runs may still have uploaded
the build artifacts.

The API we're querying uses the status query parameter to select on a
union of the status and conclusion fields of a workflow run record.
"success" is a conclusion value.  "completed" is a status value, and
includes runs where conclusion=success or conclusion=failure (and maybe
a few others).
---
 src/endpoints/cli.js | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/endpoints/cli.js b/src/endpoints/cli.js
index 48e1f7e1..86488520 100644
--- a/src/endpoints/cli.js
+++ b/src/endpoints/cli.js
@@ -51,12 +51,12 @@ export async function downloadPRBuild(req, res) {
   const prRef = pr.head.ref;
   const prRepo = pr.head.repo.full_name;

-  // Last 100 successful CI runs matching the PR's head branch name.  This may
+  // Last 100 completed CI runs matching the PR's head branch name.  This may
   // apply to multiple PR ids, so we limit by PR head repo too after the fetch.
   const params = new URLSearchParams({
     event: "pull_request",
     branch: prRef,
-    status: "success",
+    status: "completed", // includes runs where conclusion=success and conclusion=failure
     page_size: 100,
   });
   const runsResponse = await fetch(`https://api.github.com/repos/nextstrain/cli/actions/workflows/ci.yaml/runs?${params}`, {headers: {authorization}});
@@ -68,7 +68,7 @@ export async function downloadPRBuild(req, res) {
   const runId = prRuns[0]?.id;

   if (!runId) {
-    throw new NotFound(`No CI run for PR ${prId} found in last ${params.get("page_size")} successful CI runs for PRs`);
+    throw new NotFound(`No CI run for PR ${prId} found in last ${params.get("page_size")} completed CI runs for PRs`);
   }

   // Set the CI run id for downloadCIBuild()
-- 
2.43.0

but on further consideration, I'm thinking maybe we do want to keep this limited to successful runs. It seems useful to have a way to install "the last good CI run for a PR" even though some failures are still installable.

[victorlin]

maybe we do want to keep this limited to successful runs. It seems useful to have a way to install "the last good CI run for a PR" even though some failures are still installable.

Yeah, I think fine to leave it as-is. There's an easy workaround with the /cli/download/ci-build endpoint.

[tsibley]

Alright. I'll close this then. We can revisit in the future if we want.