Build-size's resolve link should force a recheck about amp-github-apps HOT 8 CLOSED

That makes sense. I wonder if the PR approval model is the best way, or whether we should move to an explicit approval of the check using an action button (which is a relatively new feature for GitHub Apps, that's why it wasn't considered before)

wdyt?

from amp-github-apps.

I like being able to approve a PR with a general PR approval, though. Would this require me to hit a second button?

from amp-github-apps.

It would (from the PR you'd have to click the Details link on the check, and then the Approve link). So you prefer the current approach

from amp-github-apps.

Hmm... the problem with re-running the check to verify the approvals is that it can't tell these two scenarios apart:

1. bundle-size approval is required → PR is approved → some error causes the approval not to be registered
2. bundle-size approval is required → PR is approved → commit is pushed → bundle-size approval is required again

from amp-github-apps.

One advantage of separating general PR approval from size-increase approval is that a reviewer who happens to be a bundle-size owner a review a PR without inadvertently allowing the size increase to sneak through. I'll defer to @danielrozenberg's judgement on this, since he's more aware of the corner cases.

While on the topic of approvals, I'd like to make a case for #141, which proposes that we switch the size increase approvers from a hard-coded list to the runtime working group. WDYT?

from amp-github-apps.

Shouldn't it be checking the order of commits/approval?

from amp-github-apps.

I'll have to check whether the API supports that, but it does complicate the app's code 1,000× >__>

from amp-github-apps.

We discussed this offline in the infra team and came to agree that fixing this is not worth the effort, given the rarity of the root cause (GitHub API/webhook failures) - the easiest solution in those cases is to rebase the last commit or close/reopen the PR. I'm going to close this issue as a won't-fix

from amp-github-apps.