site-kit-wp icon indicating copy to clipboard operation
site-kit-wp copied to clipboard

Automate regular updates to browserslist

Open aaemnnosttv opened this issue 2 years ago • 13 comments

Feature Description

Browserslist is industry-standard to set target browsers for web projects.

The database of browsers it references needs to be upgraded on a regular basis in order to get the most benefit.

From https://github.com/browserslist/browserslist#browsers-data-updating

npx browserslist@latest --update-db updates caniuse-lite version in your npm, yarn or pnpm lock file. This update will bring data about new browsers to polyfills tools like Autoprefixer or Babel and reduce already unnecessary polyfills.

You need to do it regularly for three reasons:

  1. To use the latest browser’s versions and statistics in queries like last 2 versions or >1%. For example, if you created your project 2 years ago and did not update your dependencies, last 1 version will return 2 year old browsers.
  2. Actual browsers data will lead to using less polyfills. It will reduce size of JS and CSS files and improve website performance.
  3. caniuse-lite deduplication: to synchronize version in different tools.

One option might be to leverage a GitHub action to run on a schedule and open a PR automatically, e.g. https://github.com/marketplace/actions/browserslist-update-action


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • There should be a GitHub Action set up to run that automatically updates browserlist—say every week, preferably on a weekend when commits are not likely to be made by the team.

Implementation Brief

  • Hook up an action like https://github.com/marketplace/actions/browserslist-update-action to run on an automated schedule using GitHub Actions. The existing ${{ secrets.GITHUB_TOKEN }} used in other actions should be sufficient here to run this action, I think.
    • If not, a developer with admin repo access will need to create a new repo secret with the scopes required by that action.
    • To help with review/visibility, we should add this PR to the ZenHub board, specifically to the Merge Review column, using an action like https://github.com/marketplace/actions/zenhub-move-card. This will require creation of a ZenHub token that can move the card over to the Merge Review column.
    • Doing this will reduce the chances of us missing these PRs, so while it marginally increases complexity, I think it's worth it. 🙂

Test Coverage

  • No new tests needed.

QA Brief

  • No QA needed, aside from verifying the PR is created and card is moved during development.

Changelog entry

aaemnnosttv avatar Jun 14 '22 18:06 aaemnnosttv

IB :white_check_mark:

Although, I think the estimate of 3 could be a little optimistic, so am bumping to a 7.

techanvil avatar Sep 02 '22 16:09 techanvil

Exploration update:

Found out that the browserslist-update-action can only open PRs assuming the base branch is named master, which isn't valid in our case. This causes an error and the action fails (example). I am trying to find a workaround.

nfmohit avatar Sep 21 '22 06:09 nfmohit

Found out that the browserslist-update-action can only open PRs assuming the base branch is named master, which isn't valid in our case.

@nfmohit it looks like we can configure this with the base_branch input, no? 🤔 https://github.com/marketplace/actions/browserslist-update-action#action-inputs

aaemnnosttv avatar Sep 22 '22 00:09 aaemnnosttv

@aaemnnosttv Yep, now we can, thanks to my PR that I did yesterday in my free time 😅

There's still one little caveat though. After the action creates the PR, we need a way to get the number of the PR created to hook it up with a Zenhub action and move it to the Merge Review column. I wish the browserslist-update-action would output data of the PR that was created so that it could be used by dependent actions. Do let me know if you have any ideas. Thank you!

nfmohit avatar Sep 22 '22 14:09 nfmohit

Update to the above: Looks like the action author kindly sorted that as well (release) 🎉

nfmohit avatar Sep 22 '22 14:09 nfmohit

Amazing! Thanks @nfmohit 👍👍👍

aaemnnosttv avatar Sep 22 '22 19:09 aaemnnosttv

Concerns/Questions:

  1. Since this action needs to create a branch, make commits, and create a PR, what Git user should we use? Most GitHub actions use GitHub Action<[email protected]> as the user, should we go that route? If so, how do we handle Google CLA? One approach could be adding the cla label.
  2. Currently, my (draft) implementation uses text from the action example as the PR title and description. Are they appropriate, or should we define better PR title and description?
  3. In order to move the PR in the Zenhub board:
    • We need a Zenhub Access Token (for REST). Where could I get that?
    • There are other details like Workspace ID, Repository ID, and Pipeline ID that needs to be in our workflow. Should they be saved as secrets for security?
  4. During my initial tests with the browserslist-update-action GitHub action, my workflows were failing while running npm install/uninstall with peer-dependency conflicts (example). I bypassed it by making the workflow run npm config set legacy-peer-deps true. Should we investigate this error, or is it expected and we are okay to use legacy-peer-deps?

CC: @aaemnnosttv @tofumatt @techanvil

nfmohit avatar Sep 22 '22 23:09 nfmohit

  1. Since this action needs to create a branch, make commits, and create a PR, what Git user should we use? Most GitHub actions use GitHub Action<[email protected]> as the user, should we go that route? If so, how do we handle Google CLA? One approach could be adding the cla label.

I think that would be fine in this case. We accept commits from dependabot which would be a similar case. The CLA only applies to people, so I don't foresee it being a problem but that's a good shout. I guess we'll have to wait and see if it comes up. Just adding the label wouldn't be a solution though (I'm not even sure if that works as a manual override now that it's a GH check). It only works when added by Googlers. This should be okay though.

  1. Currently, my (draft) implementation uses text from the action example as the PR title and description. Are they appropriate, or should we define better PR title and description?

LGTM so far! I think we can review that as part of the CR too.

  1. In order to move the PR in the Zenhub board:

    • We need a Zenhub Access Token (for REST). Where could I get that?
    • There are other details like Workspace ID, Repository ID, and Pipeline ID that needs to be in our workflow. Should they be saved as secrets for security?

I would nix this part altogether. We only work with issues on the boards, not PRs so it wouldn't make sense to add them. We don't do this for dependabot PRs (which are arguably more important) but review them every sprint and merge them after we update main for the release. It would be fine to add these as something to check for around the same time, so they won't be missed 👍

  1. During my initial tests with the browserslist-update-action GitHub action, my workflows were failing while running npm install/uninstall with peer-dependency conflicts (example). I bypassed it by making the workflow run npm config set legacy-peer-deps true. Should we investigate this error, or is it expected and we are okay to use legacy-peer-deps?

My guess is that this is happening because npm is running with whatever version is pre-installed on the runner, which is probably newer than what we use with Node 14. Try using setup-node as we do in other jobs first and my guess is that should address the failure. Note we're also updating the way we use this in another issue around upgrading to v3 of this action so let's be sure to coordinate there to make sure it isn't missed 👍

aaemnnosttv avatar Sep 23 '22 21:09 aaemnnosttv

Thank you for all the clarifications and confirmations, @aaemnnosttv! I'll reflect them in my PR accordingly.

5. In order to move the PR in the Zenhub board:

* We need a Zenhub Access Token (for REST). Where could I get that?
* There are other details like Workspace ID, Repository ID, and Pipeline ID that needs to be in our workflow. Should they be saved as secrets for security?

I would nix this part altogether. We only work with issues on the boards, not PRs so it wouldn't make sense to add them. We don't do this for dependabot PRs (which are arguably more important) but review them every sprint and merge them after we update main for the release. It would be fine to add these as something to check for around the same time, so they won't be missed 👍

Agreed and understood, though a bit sad because I spent quite some time for this step starting from making the browserslist-update-action return outputs containing the data of the created PR, to exploring the Zenhub API to move the PR 😅 Nonetheless, good call though.

7. During my initial tests with the browserslist-update-action GitHub action, my workflows were failing while running npm install/uninstall with peer-dependency conflicts (example). I bypassed it by making the workflow run npm config set legacy-peer-deps true. Should we investigate this error, or is it expected and we are okay to use legacy-peer-deps?

My guess is that this is happening because npm is running with whatever version is pre-installed on the runner, which is probably newer than what we use with Node 14. Try using setup-node as we do in other jobs first and my guess is that should address the failure. Note we're also updating the way we use this in another issue around upgrading to v3 of this action so let's be sure to coordinate there to make sure it isn't missed 👍

Understood. I'll give this a try and get back if that still doesn't work. Thank you!

nfmohit avatar Sep 23 '22 21:09 nfmohit

  1. During my initial tests with the browserslist-update-action GitHub action, my workflows were failing while running npm install/uninstall with peer-dependency conflicts (example). I bypassed it by making the workflow run npm config set legacy-peer-deps true. Should we investigate this error, or is it expected and we are okay to use legacy-peer-deps?

My guess is that this is happening because npm is running with whatever version is pre-installed on the runner, which is probably newer than what we use with Node 14. Try using setup-node as we do in other jobs first and my guess is that should address the failure. Note we're also updating the way we use this in another issue around upgrading to v3 of this action so let's be sure to coordinate there to make sure it isn't missed 👍

Understood. I'll give this a try and get back if that still doesn't work. Thank you!

Okay, following up on the above discussion. It looks like browserslist/update-db only works with npm versions 7 or above. Since our project runs on Node 14 that comes with NPM 6, it throws an error and asks us to use the legacy npx browserslist@latest --update-db command, which is apparently deprecated and to be removed in a future major browserslist release.

We could still use the legacy npx browserslist@latest --update-db command until it is removed, but unfortunately, the browserslist-update-action GitHub action that we're trying to use utilises the new browserslist/update-db package. The action does have a legacy v1 version that uses the npx browserslist@latest --update-db command, but it will not work with our project since it can only open PRs assuming the base branch is named master (problem fixed in v2).

One potential solution could be bumping up the npm version in the workflow, but I fear that would make unnecessary changes to the package-lock.json file that we would despise.

@aaemnnosttv How do you suggest we proceed? Thank you!

nfmohit avatar Sep 24 '22 05:09 nfmohit

Hi @aaemnnosttv and @nfmohit please could we get an update on the ticket to say what we are planning here. If this is stalled, perhaps we need to remove from the sprint so as not to skew the capacity numbers. Please confirm if this is what you would like to do, thanks!

FlicHollis avatar Oct 07 '22 10:10 FlicHollis

@aaemnnosttv Just an update that I worked a bit with the author of the GH action over the weekend to get the fix for the base branch on v1, with a patch release. The action works now and I think we're ready to test it. Assigning it to you for CR.

nfmohit avatar Oct 17 '22 13:10 nfmohit

Thanks @nfmohit !

aaemnnosttv avatar Oct 17 '22 18:10 aaemnnosttv

QA: :warning:

The automated/scheduled PR was created successfully. However, while that was already open, I tried to run the action manually and it resulted in errors related to merge conflict. Not sure if that will also happen when the existing PR is not merged but the action runs on next schedule (seems like a likely case when the PR gets buried) or the PR is merged but the Merge Reviewer forgot to delete the branch.

Any thoughts?

cc @tofumatt @aaemnnosttv

kuasha420 avatar Nov 01 '22 08:11 kuasha420

Very interesting finding. Thank you, @kuasha420!

Here is the PR that was created by the scheduled workflow.

There are two issues:

  • Merge conflicts as @kuasha420 mentioned above.
  • The cla/google check failing on the automatically created PRs. I see Felix manually added the cla: yes label, but do we need to do something that would address this automatically?

@aaemnnosttv @tofumatt What do you think (also assigning you for feedback)?

Thanks folks!

nfmohit avatar Nov 02 '22 09:11 nfmohit

Ohhh, I see. I guess that's because the browserlist PR is using the same branch name all the time. The best way around that I'd think would be to have the branches have a date or something else unique in the branch name.

Adjusting this https://github.com/google/site-kit-wp/blob/develop/.github/workflows/browserslist-db-update.yml#L33 to have a variable in it should prevent merge conflicts… maybe with a variable like GITHUB_RUN_ID. GITHUB_RUN_ID would be unique every run, so even when a workflow is re-run there wouldn't be the chance of conflicts. Eg. update/browserslist-db-${{ GITHUB_RUN_ID }}

It could be nice if this automatically deleted other update/browserslist-db branches and delete them but no biggie there.

In terms of the CLA key… it needs to be a Google account that sets this label, so I don't think the GitHub action can set it and have it affect the CLA bot. I'm not sure that's a big issue, as a merge reviewer will be checking the contribution beforehand and it's a bot, not a person so I don't think the CLA agreement really applies.

Let's do a follow-up PR that handles unique branch names.

tofumatt avatar Nov 02 '22 11:11 tofumatt

Thank you for the brilliant tip there, @tofumatt!

One follow-up question: what would happen to the existing PR(s) in that case? With each new branch, a new PR would be created. Wouldn't that create confusion about having multiple PRs?

Thanks!

nfmohit avatar Nov 02 '22 12:11 nfmohit

Ideally, we would delete the old PRs… something like https://github.com/dawidd6/action-delete-branch (that's archived, so maybe not that exact one), that would delete all old update/browserslist-db-* branches before making the new one.

tofumatt avatar Nov 02 '22 12:11 tofumatt

  • The cla/google check failing on the automatically created PRs. I see Felix manually added the cla: yes label, but do we need to do something that would address this automatically?

I asked @felixarntz about this but don't have an answer yet about whether or not the CLA check failure would be a problem. The check doesn't prevent us from merging the PR but hopefully this won't be a problem for us – standing by to hear from Felix.

aaemnnosttv avatar Nov 02 '22 13:11 aaemnnosttv

@tofumatt I've opened #6101 which deletes the existing branch on every run (thus closing the associated PR if open) and creates a new branch and PR.

Let's do a follow-up PR that handles unique branch names.

I didn't implement this part because:

  • We are deleting the existing branch anyway.
  • Since we need to delete the existing branch, it would be complicated to lookup the branch name with the unique run ID.

What do you think?

CC: @aaemnnosttv @kuasha420

nfmohit avatar Nov 02 '22 21:11 nfmohit

Hey @nfmohit, taking looking at this in lieu of @tofumatt - seems to me your proposed PR solves the problem well enough without needing to make the branch name unique.

If we do run into a situation where the static branch name is a problem we have a good idea of how to address is. But I think we can hold off for now and see how we get on.

techanvil avatar Nov 03 '22 11:11 techanvil

@felixarntz would you please advise as to how we should address the CLA issue here? Can we not consider these manually approved/ignored failures as we would by adding the "cla: Yes" label in the past?

Either way I don't think it needs to block the issue here but curious if we need to create a bot (user?) or not.

aaemnnosttv avatar Nov 04 '22 22:11 aaemnnosttv

@techanvil @nfmohit @tofumatt @aaemnnosttv Providing an update here: Per Google policy we will not be able to use code "authored" by the github-actions user. We will have to create a Google-owned bot account (e.g. something like site-kit-bot) and configure that one with the GitHub repository. There are internal instructions available for how to do that.

@aaemnnosttv will open a follow up issue to track that work. Most of it will have to happen internally, but the final bit will require changes in the plugin codebase (GitHub action). I will close this issue now as by itself it works as expected (except of course for the above, which however was not part of this issue's definition).

For now, this means the caveat is that we shouldn't merge/accept those automated PRs for the time being. If we prefer to not get the "noise" of those PRs for now, we could also deactivate the action for now until we have addressed the above.

felixarntz avatar Nov 07 '22 17:11 felixarntz