I2I: AMP Bundle Size GitHub App

[danielrozenberg]

Objective

Create a GitHub app to replace the existing bundle-size check, and make the check differential instead of absolute.

mock screenshot of the new bot

Background

This design doc supersedes the changes proposed for the bundle size checks in #17043, which proved insufficient to resolve developers' complaints

The AMP project checks the size of the compiled JavaScript bundle with every pull request to the GitHub project, as part of the promise to keep the size bundle size small. This check runs on the Travis continuous integration platform, executed by the gulp task executor.

The existing check is not well liked, since it blocks when a PR pushes the bundle size above a certain absolute maximum size. It also reports of changes only in the Travis logs, reducing visibility on the bundle size growth.

This document summarizes offline discussions on a solution to both of these issues in the form of a GitHub app.

Overview

The GitHub app will be written using Probot and will run on AppEngine. The app will have the following API points:

• POST /v0/commit/<commit_sha>/report
  • Accepts a JSON from Travis builds that report the absolute bundle size of the PR and the differential size loss/gain compared to the PR's branch off point.
    • If the differential size is ≤ 0.1KB, update the status to green
    • If the differential size is > 0.1KB, update the status to red and comment on the PR that with instructions to the reviewer
• POST /v0/commit/<commit_sha>/skip
  • Sets the status to green
• POST /v0/webhook (GitHub webhook)
  • For new pull requests events (or new commits on the PR) update the status to yellow
  • For approved reviews event:
    • If the status is red (because the differential size is > 0.1KB) update the status to green
    • If the status is yellow or green, do nothing

See security considerations in the section below.

Messages

The status messages on the app should be concise, since there is not a lot of space in the UI

• ✕ AMP bundle-size — Δ +0.5KB | approval required
• •  AMP bundle-size — Calculating new bundle size for this PR…
• 🗸 AMP bundle-size — Δ +0.5KB | approved by @somereviewer
• 🗸 AMP bundle-size — Δ +0.1KB | no approval necessary
• 🗸 AMP bundle-size — Δ 0.0KB | no approval necessary
• 🗸 AMP bundle-size — Δ -0.3KB | no approval necessary
• 🗸 AMP bundle-size — bundle size check skipped for this PR

The text of the comment given a large increase in the bundle size:

• This PR increases the bundle size by Δ +0.5KB. Reviewer: approving this PR will approve this bundle size increase.

Changes to gulp bundle-size

Modify the gulp bundle-size action with the following flags. Remove all other actions that this check performs:

• --store (maybe rename to --master?)
  • Keep as is; stores the new bundle size in the AMP artifacts storage
• --skip <commit_sha>
  • Send a POST request to /v0/commit/<commit_sha>/skip; this should be called from Travis checks that would not, in the existing implementation, run the bundle-size check. This is required so that the GitHub app can be set as a required check.
• --report <commit_sha>
  • Send a POST request to /v0/pr/<commit_sha>/report using the existing method of determining the bundle size and the delta. Note that sometimes the PR's branching point will be missing from ampproject/amphtml-build-artifacts. Possible solutions:
    • Dig deeper in the commit log for parent^1, parent^2, etc… up to a reasonable max, and the delta based on that. This "indirect parent" should be reported in BIG BOLD LETTERS in the app's status
    • Give up (change the status to red) and allow the reviewer to make a decision (e.g., skip the bundle size for this PR by approving it, manually add the bundle size to the artifacts storage and rerun the Travis check, etc.)

Open sourcing & future work

GitHub Checks can give us a lot of power and expose more information to contributors. We'll create a new GitHub repository (ampproject/amphtml-github-bots) to maintain the code for this and all new GitHub bots that we intend to write, and to document their deployment and development.

Security Consideration

The GitHub app should be secured to prevent malicious users from modifying PR statuses, by using the Travis IP addresses to validate that the requests are coming from Travis, especially for the /v0/commit/* API points, and by using secret tokens on the app server to validate GitHub webhook requests. This is a minor security consideration, and an unlikely vector of attack (the gain for the attacker is minimal, and can be easily reverted.)

The GitHub app will not have access to read or modify the source code or approve the PR. Regardless, Probot will have to go through a security review.

Privacy Considerations

No PII is involved.

[ssantosms]

I like the idea of tracking the Delta on PR instead of absolute size.
I just wonder if there is still a desire to keep the bundle under a certain size and if that will still be enforced even if it's a higher threshold. Was that discussed?

[danielrozenberg]

@ssantosms we had an offline discussion about this, the gist of it is that we prefer to keep an eye on the absolute bundle size without having it block PR-level changes. We're also discussing whether we should create a GitHub Team whose members will be the exclusive approvers of large increases. I'd love to have input on these decisions!

[ssantosms]

@danielrozenberg I think not blocking the PR for this reason is fine, but the absolute bundle size should be tracked and there needs to be a plan of action in case it increases to a certain threshold. I know it's very unlikely to happen but if several PRs are approved around the same time with exceptions, it could cause the overall bundle size to explode and we would only know about that when generation a new ship candidate. But again, this is probably a corner case.
Regarding the GitHub team to approve the large increases, that needs to be in place in my opinion.

By the way, I like this change a lot. I think it improves the developer experience and it is a good way to measure the impact of the bundle size when the PR is created.