Reduce the frequency and number of amphtml package upgrades

[rsimha]

Background:

The amphtml repo has >150 dependencies in package.json. We use renovate to keep them up to date.

Advantages:

• Allows us to use the latest open-source tools
• Allows our node environment to be auto-updated (see Discussion to avoid node version incompatibility with LTSs #19050, Upgrade node from ^10.0.0 to ^12.0.0 #25161 (comment), Use renovate to auto-update Node versions in package.json files #25209, and 📦 Update Node.js to 12 #28123)
• Helps us avoid security vulnerabilities (🏗 Enable package updates for E2E and visual diff tests #23427 (comment), 🏗🐛Manually update build-system/tasks/visual-diff/yarn.lock to address security vulnerabilities #23789 (comment), 🏗 Manually upgrade transitive validator dependencies with security vulnerabilities #25445 (comment), 🏗📦 Switch renovate updates for amphtml from opt-in to opt-out #27692 (comment)).

Disadvantages:

• We need to manually merge a large number of renovate PRs each week
• Most amphtml PRs come from renovate
• AMP release notes are filled with noise (example)
• Test speed-ups are less effective because of frequent cache busting
• There are no clear guidelines around what kind of dependencies we can / can't add (Clarify processes or guidelines around introducing devDependencies #24550)

Purpose of this issue:

To identify a way to reduce the number of PRs while continuing to keep dependencies up to date.

Some ideas:

1. Reduce the number of amphtml dependencies: This is easier said than done. We do frequent clean-ups of unnecessary packages, but our toolchain is large, and rewriting the functionality natively is not an option.
2. Merge all upgrades at night: Today upgrades that pass Travis are merged manually, so we can't rely on the presence of a developer during non-working hours. Also, this doesn't reduce the PR count.
3. Allow upgrades that pass Travis to be auto-merged at night: This will require giving write permissions to renovate, which we've been hesitant to do. This too doesn't reduce the PR count.
4. Consolidate all MINOR and PATCH upgrades into a single PR per day: This will reduce the number of PRs, but can substantially increase the trouble involved in debugging and fixing a breaking change. It can also require frequent auto-rebasing by renovate.
5. Find some other meaningful grouping of package upgrades: Renovate has some native grouping functionality, but it isn't clear how we can use it.

Other ideas?

[rsimha]

Related: ampproject/amp-github-apps#284

/cc @ampproject/wg-infra

[rcebulko]

• Most amphtml PRs come from renovate

Does this matter enough that it should really factor into a decision?

• AMP release notes are filled with noise (example)

We already group commits by component/directory, and there's a dedicated section for package updates. Given how the release notes are used (ie. often a developer looking at a release to see if their PR is present), maybe we could remove all the renovate commits from the list (possibly indicating at the end of the list that N were removed).

• Test speed-ups are less effective because of frequent cache busting

😮

2. Merge all upgrades at night: Today upgrades that pass Travis are merged manually, so we can't rely on the presence of a developer during non-working hours. Also, this doesn't reduce the PR count.
3. Allow upgrades that pass Travis to be auto-merged at night: This will require giving write permissions to renovate, which we've been hesitant to do. This too doesn't reduce the PR count.

This seems like something worth considering. I don't think the PR count should factor too heavily into the decision; this alone could bring a significant workflow change for the on-duty and, judging by the discussion linked above, a significant performance improvement for Travis builds

4. Consolidate all MINOR and PATCH upgrades into a single PR per day: This will reduce the number of PRs, but can substantially increase the trouble involved in debugging and fixing a breaking change. It can also require frequent auto-rebasing by renovate.

Subtle but useful distinction: I think separately grouping MINOR and PATCH into two separate PRs will reduce the frequency of "one bad apple spoiling the bunch" situations, with the PATCH PRs likely to be more stable than the MINOR PRs.

5. Find some other meaningful grouping of package upgrades: Renovate has some native grouping functionality, but it isn't clear how we can use it.

By my count, there are 13 package.json files in this repo. At least 10 of them get regular updates by renovate. Maybe the owners of those package.json files and their dependencies have some opinions/preferences on how that sub-project dependency set should be managed.

I suspect that some of these subpackages are less sensitive to changes than, say, the build system. Perhaps select subpackages can get auto-updates or grouped updates more liberally than core packages like build-system, validator, etc.

Other ideas?

Use a separate branch for all dependency updates, allowing renovate to merge updates that are green, then manually merging that branch in every N days. This has the benefit of preserving the state of the Travis cache while still ensuring each upgrade is green at HEAD before merging. This does still carry the downside of increased debugging difficulty, along with added complexity and room for error.

[jridgewell]

Allow upgrades that pass Travis to be auto-merged at night: This will require giving write permissions to renovate, which we've been hesitant to do. This too doesn't reduce the PR count.

If we don't want to give renovate merge rights, perhaps we can create a bot that will scan for PRs from renovate that are approved and merge them?

[rsimha]

Thanks for the comments. As a start, I think we should consolidate amphtml's renovate upgrades into at most one MINOR and one PATCH PR per day per package.json file. MAJOR upgrades can still be tested separately given the likelihood of being a breaking change. (@rcebulko since you initially proposed something similar in ampproject/amp-github-apps#284 (comment), can I ask you to do this when time permits?)

Once that's done, we can look at the PR load and determine if it's worth pursuing the other ideas. SG?

[rcebulko]

The above PR takes the first step of grouping by MINOR and PATCH type. As for grouping by package.json, I was thinking of a grouping like the following:

Root:
./package.json

Validator:
./validator/package.json
./validator/nodejs/package.json
./validator/webui/package.json
./validator/webui/dist/webui_appengine/package.json
./validator/gulpjs/package.json

Other:
./extensions/amp-access/0.1/iframe-api/package.json
./extensions/amp-viewer-integration/0.1/messaging/package.json
./third_party/amp-toolbox-cache-url/package.json

Build System:
./build-system/tasks/visual-diff/package.json
./build-system/tasks/performance/package.json
./build-system/tasks/e2e/package.json
./build-system/tasks/storybook/package.json

Purifier:
./src/purifier/package.json

Within each group, we'd combine MINOR and PATCH updates into PRs for each. For example, all PATCH updates for the Validator and subpackages would get one PR, same for build-system, etc. In some cases (Misc, Purifier) we might even combine MINOR and PATCH. We could then assign reviewers for some of these groups as well, such as wg-caching for all validator updates, wg-runtime for Purifier, wg-infra for Build System, etc.

If we group and assign appropriately, the subpackage dependency updates could be owned by the OWNERS while the build cop would only be responsible for the root package.json

[rsimha]

I like your idea of separating updates into groups! Root, Validator, Other, and Build System SGTM, but I think Purifier should be called Runtime, so that it can include other potential additions inside src/. Also, +1 to your idea for how to assign updates for review.

👏 👏 👏

[rcebulko]

The config has been updated; we'll see tonight at midnight how it shakes out :)

[rsimha]

The config has been updated; we'll see tonight at midnight how it shakes out :)

Thanks for all the work! I'll merge the backlog of PRs today so we can start afresh tomorrow.

[rcebulko]

With recent updates, renovate PRs will now be grouped by category and, where relevant, labeled with the appropriate working group label. Patch and Minor updates will get separate PRs.

Due to known permissions issues with renovate, it cannot assign a GitHub team as a reviewer. However, all of the package.json files live in directories which are already owned by the relevant team in the corresponding OWNERS file. The result is that each renovate PR will be automatically assigned by the Owners Bot. This will have the effect of spreading out the renovate PR workload across a working group which, combined with the significantly lower PR volume, should significantly improve developer experience and review load.

If needed, owners files can be adjusted for specific package.json files.

[rcebulko]

I'm thinking it may make sense to also group all changes, across all packages in the repo, to eslint, prettier, and related extensions. For example, #28488 has a handful of innocuous changes, but the eslint update causes Travis to fail the initial checks due to a new lint error. Instead, we can group all lint/prettier updates together, and let renovate run gulp lint --fix --local_changes for us before creating the PR

Edit: Because we don't self-host Renovate, we can't make it run arbitrary commands on our behalf 😢 For now I'm added an explanatory comment indicating the reviewer should run the lint fix command and commit the changes.

[rcebulko]

Closing this for now, as it seems the work around package grouping and reviewer assignment via Owners Bot has addressed these issues