Meta: fix PR previews

[bakkot]

🤞

This posts a comment instead of making the status a link like we had previously; I don't think the old behavior is possible with this approach. It makes its own comment instead of editing the OP for you because I really dislike things which edit other people's messages.

This is based in part on @gibson042's work here but using our own deploy script instead (and avoiding some complexity by just rebuilding instead of re-using the artifact from the linting job).

[ljharb]

Hmm, the status link should still be settable (it's "view details" under the ellipsis) but a sticky comment is fine too, i suppose

[github-actions]

The rendered spec for this PR is available at https://tc39.es/ecma262/pr/3679.

[ljharb]

hm, i'm a bit confused how this is working securely without a pull_request_target workflow - couldn't anyone publish anything they want on tc39.es with this approach?

[bakkot]

New contributors need actions to be manually approved in order to run, so no.

If existing contributors are in your threat model, they could already have modified the actions to do that anyway; this doesn't change anything there.

[bakkot]

Though, possibly GitHub's security model for actions means this also wouldn't work for PRs created from forks, in which case I guess it will need to be switched to do something more complicated. Will test in the morning.

[ljharb]

Yes, action runs on forks is what I'm thinking of, since any commits in forks are also considered part of the source repo.

[michaelficarra]

I think this will do what we want with the details link: https://github.com/marketplace/actions/github-checks#details_url. I'd prefer that over a bot that leaves comments.

[ljharb]

If existing contributors are in your threat model, they could already have modified the actions to do that anyway; this doesn't change anything there.

They should be in everyone's threat model because of ATO, but that's why i'm asking about pull_request_target - where the workflow is only used from the base branch, not from the PR branch.

[bakkot]

OK, yeah, I don't think this is insecure, it just won't work for forks. We'll have to do the more complicated thing.

[ljharb]

makes sense, iirc that's why we did it that way in the first place.

[bakkot]

OK, switched to using pull_request_target, gated on the PR creator being (publicly) a member of the tc39 org (or jmdyck).

Ideally we'd also have some way to trigger it based on a label or comment on the PR but this is at least a start.

Edit: ok, made the "request preview" label also work.

[ljharb]

(needs a rebase tho)

[mhofman]

I don't think this is working as expected. https://tc39.es/ecma262/pr/3689/#sec-promise-resolve does not show the updated spec text, and references the commit of the merge of this PR, not of my branch.

Edit: I think the problem is that actions/checkout checks out the main branch, not the branch of the pull request that triggered the workflow.

[ljharb]

indeed; that's how pull_request_target works - good catch.