Restore nextstrain.org PRs

[victorlin]

Description of proposed changes

Follow-up to db85805.

Heroku is building review apps for nextstrain.org now.

Related issue(s)

• Closes reenable nextstrain.org PR action #1564

Testing

• Manual trigger using gh workflow run "Make PRs for Nextstrain projects which depend on Auspice" --repo nextstrain/auspice --ref victorlin/restore-nextstrain.org-prs succeeds (link)
• The resulting PR looks good, and a new PR doesn't get created upon updates
• Figure out why runs are taking so long (>15m) not worth it

[tsibley]

Heroku is still not building review apps

Uh, yes they are? Maybe we forgot to re-enable them for Auspice, but they work for nextstrain.org. [Edit: Ah, this is for review apps for nextstrain.org PRs not (directly) Auspice PRs, so now I'm really confused why it appears to not be working.]

[tsibley]

+1 for the re-enabling here.

Not the fault of this PR, but I'm still not thrilled that we're passing decently powerful credentials to some random third-party code:

auspice/.github/workflows/make_prs_for_other_repos.yaml

Lines 29 to 31 in 5ba0b26

	uses: peter-evans/create-pull-request@v3
	with:
	token: ${{ secrets.NEXTSTRAIN_BOT_PAT }}

[jameshadfield]

Good timing! As per #1564 I'd recommend making this a workflow dispatch rather than automatically making a (nextstrain.org) PR for each auspice PR -- in practice this resulted in too many unnecessary PRs which became confusing.

[victorlin]

@tsibley oh you're right, they are working on nextstrain.org PRs. I updated the commit/PR descriptions to reflect that.

[tsibley]

@victorlin

From the message of af56b03:

Two notes:

1. The title and description of the PR have been updated to use the
   source ref (e.g. branch, tag) rather than PR number, since that is
   not guaranteed or retrievable with the manual trigger.
2. The commit to install Auspice from is changing from the
   GitHub-managed PR merge HEAD to the source ref HEAD. This can have
   implications when run on a PR's source branch if there are notable
   changes in the target branch unincorporated in the source branch. To
   mitigate this, make sure to update the source branch (e.g. with a git
   rebase) before triggering this workflow.

To avoid both of these issues, can we instead make the workflow accept a PR number? e.g.:

gh workflow run "Make PRs for Nextstrain projects which depend on Auspice" \
  --repo nextstrain/auspice \
  -f pr=1565

The workflow would then have the PR number to use and be able to turn refs/pull/${{inputs.pr}}/merge into a commit id (SHA) and use that as it previously did.

[victorlin]

@tsibley I've opted for automatic retrieval of PR number using gh pr view: 9c9bfd3 Still WIP but the PR number retrieval should work. Good point about using refs/pull/${{inputs.pr}}/merge, I'll add that back.

[jameshadfield]

@victorlin is your intention to run these on the head commit of the branch (which is what I did when I set up the action) or the merge commit of the branch into master (which is GITHUB_SHA, as I understand it)? I don't know which is the best for this purpose, but I thought it'd be difficult to try to debug a failing nextstrain.org build if the problem was only introduced by the merge and thus didn't exist on the auspice PR branch.

[victorlin]

@jameshadfield oops, I got your original intention backwards in my commit message. You're right, GITHUB_SHA for pull_request events reflect the GitHub-managed PR merge and your implementation.

In the currently pushed implementation, I intend to run these on the head commit of the branch (not merge), which is simply GITHUB_SHA for the workflow_dispatch event. So this would be the same as before.

However, @tsibley noted from my (incorrect) commit description that it's still possible to get the merge ref from the PR number. This would change behavior as you mention:

I thought it'd be difficult to try to debug a failing nextstrain.org build if the problem was only introduced by the merge and thus didn't exist on the auspice PR branch.

I'd actually be in favor of surfacing these failures, since they are bound to come up once actually merged, and better to catch them sooner than later.

[jameshadfield]

I'd actually be in favor of surfacing these failures, since they are bound to come up once actually merged, and better to catch them sooner than later.

Fair enough. Perhaps add a message to this effect in the PR body, as this will catch someone out!

the head commit of the branch (not merge), which is simply GITHUB_SHA for the workflow_dispatch event

I guess this makes some sense, but I find it super confusing to have GITHUB_SHA refer to different things depending on the trigger. I didn't actually know a branch-merge-commit (which isn't found anywhere in your local git tree) even exists until I wrote the original action here.

[victorlin]

Updated to only allow branches with PRs, for simplicity and parity with previous behavior.

[victorlin]

This is ready for review again. I went down a deep rabbit hole on the last task in the PR tasklist - figuring out why runs are taking so long [1] [2], and trying alternatives unsuccessfully. 15 minute npm installs aren't ideal, but I think we can live with it.

[jameshadfield]

Looks good @victorlin -- thanks for persevering 😄