Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ci: a new prep-deps workflow with caching #29979

Open
wants to merge 1 commit into
base: self-hosted-benchmarks
Choose a base branch
from
+143 −257
Open

ci: a new prep-deps workflow with caching #29979

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions .github/workflows/add-release-label.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,11 @@ jobs:
runs-on: ubuntu-latest
if: github.event.pull_request.merged == true
steps:
- name: Checkout repository
uses: actions/checkout@v4
- name: Setup environment
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not merge the Checkout repository and Setup environment steps together. What if there is some occasion where you want only one? Feels like this step is doing too much at once

uses: metamask/github-tools/.github/actions/setup-environment@caching
with:
fetch-depth: 0 # This is needed to checkout all branches

- name: Setup environment
uses: metamask/github-tools/.github/actions/setup-environment@main

- name: Get the next semver version
id: get-next-semver-version
env:
Expand Down
7 changes: 3 additions & 4 deletions .github/workflows/benchmarks.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,10 @@ jobs:
container:
image: cimg/node:22.13-browsers
steps:
- name: Checkout repository
uses: actions/checkout@v4

- name: Setup environment
uses: metamask/github-tools/.github/actions/setup-environment@benchmarks
uses: metamask/github-tools/.github/actions/setup-environment@caching
with:
should-cache-restore: ${{ vars.USE_CACHING }}

- name: Download artifact prep-build-test-webpack
uses: actions/download-artifact@v4
Expand Down
4 changes: 3 additions & 1 deletion .github/workflows/build-beta.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,9 @@ jobs:

- name: Setup environment
if: ${{ steps.needs-beta-build.outputs.NEEDS_BETA_BUILD == 'true' }}
uses: metamask/github-tools/.github/actions/setup-environment@main
uses: metamask/github-tools/.github/actions/setup-environment@caching
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like setup environment has too many arguments and it's doing too much. I wonder if this could be separated to multiple smaller workflows?

with:
no-cache-for-release: 'true' # Turn off yarn cache as well

- name: Run beta build
if: ${{ steps.needs-beta-build.outputs.NEEDS_BETA_BUILD == 'true' }}
Expand Down
7 changes: 3 additions & 4 deletions .github/workflows/build-storybook.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,10 @@ jobs:
name: Build storybook
runs-on: ubuntu-latest
steps:
- name: Checkout repository
uses: actions/checkout@v4

- name: Setup environment
uses: metamask/github-tools/.github/actions/setup-environment@main
uses: metamask/github-tools/.github/actions/setup-environment@caching
with:
no-cache-for-release: 'true' # Turn off yarn cache as well

- name: Build storybook
run: yarn storybook:build
Expand Down
5 changes: 1 addition & 4 deletions .github/workflows/check-attributions.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,8 @@ jobs:
check-attributions:
runs-on: ubuntu-latest
steps:
- name: Checkout repository
uses: actions/checkout@v4

- name: Setup environment
uses: metamask/github-tools/.github/actions/setup-environment@main
uses: metamask/github-tools/.github/actions/setup-environment@caching
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should change from @main (or @caching) to @commithash-on-main


- name: Check attributions changes
run: yarn attributions:check
5 changes: 1 addition & 4 deletions .github/workflows/check-pr-labels.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,8 @@ jobs:
pull-requests: read

steps:
- name: Checkout repository
uses: actions/checkout@v4

- name: Setup environment
uses: metamask/github-tools/.github/actions/setup-environment@main
uses: metamask/github-tools/.github/actions/setup-environment@caching

- name: Check PR has required labels
env:
Expand Down
5 changes: 1 addition & 4 deletions .github/workflows/check-template-and-add-labels.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,8 @@ jobs:
check-template-and-add-labels:
runs-on: ubuntu-latest
steps:
- name: Checkout repository
uses: actions/checkout@v4

- name: Setup environment
uses: metamask/github-tools/.github/actions/setup-environment@main
uses: metamask/github-tools/.github/actions/setup-environment@caching

- name: Check template and add labels
id: check-template-and-add-labels
Expand Down
5 changes: 1 addition & 4 deletions .github/workflows/close-bug-report.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,8 @@ jobs:
runs-on: ubuntu-latest
if: github.event.pull_request.merged == true && startsWith(github.event.pull_request.head.ref, 'Version-v')
steps:
- name: Checkout repository
uses: actions/checkout@v4

- name: Setup environment
uses: metamask/github-tools/.github/actions/setup-environment@main
uses: metamask/github-tools/.github/actions/setup-environment@caching

- name: Close release bug report issue
env:
Expand Down
7 changes: 2 additions & 5 deletions .github/workflows/fitness-functions.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,11 @@ jobs:
build:
runs-on: ubuntu-latest
steps:
- name: Checkout code
uses: actions/checkout@v4
- name: Setup environment
uses: metamask/github-tools/.github/actions/setup-environment@caching
with:
fetch-depth: 0 # This is needed to checkout all branches

- name: Setup environment
uses: metamask/github-tools/.github/actions/setup-environment@main

- name: Run fitness functions
env:
BASE_REF: ${{ github.event.pull_request.base.ref }}
Expand Down
70 changes: 27 additions & 43 deletions .github/workflows/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,62 +15,52 @@ on:
merge_group:

jobs:
prep-deps:
Copy link
Contributor

@itsyoboieltr itsyoboieltr Feb 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running prep-deps and making all other jobs dependent on it is a bit dangerous, especially given the limitations of gh actions we recently encountered (max 20 workflows in one file). It is very likely that in the near future (which probably means, when I get off release rotation in March) we will need to split main.yml into multiple smaller workflows (probably each workflow will go in its own file, or we need to batch them somehow, like the short suite), which means that they will not be able to depend on prep-deps.

Also keep in mind that we need to migrate 40 more workflows.

More context here: https://consensys.slack.com/archives/C087GPTR5HQ/p1738658608653149

runs-on: ubuntu-latest
steps:
- name: Setup environment
uses: metamask/github-tools/.github/actions/setup-environment@caching
with:
should-cache-restore: 'false'
should-cache-save: ${{ vars.USE_CACHING }}
# -- If you're a repo admin, you can change vars.USE_CACHING at:
# https://github.com/MetaMask/metamask-extension/settings/variables/actions
# -- If you're just a contributor, you can change vars.USE_CACHING by following this documentation:
# https://docs.github.com/en/rest/actions/variables?apiVersion=2022-11-28#update-a-repository-variable

lint-workflows:
name: Lint workflows
uses: metamask/github-tools/.github/workflows/lint-workflows.yml@benchmarks

test-lint-shellcheck:
name: Test lint shellcheck
uses: ./.github/workflows/test-lint-shellcheck.yml
uses: metamask/github-tools/.github/workflows/lint-workflows.yml@caching

test-lint:
name: Test lint
needs: prep-deps
uses: ./.github/workflows/test-lint.yml

test-lint-changelog:
name: Test lint changelog
uses: ./.github/workflows/test-lint-changelog.yml

test-lint-lockfile:
name: Test lint lockfile
uses: ./.github/workflows/test-lint-lockfile.yml

test-deps-audit:
name: Test deps audit
uses: ./.github/workflows/test-deps-audit.yml

test-yarn-dedupe:
name: Test yarn dedupe
uses: ./.github/workflows/test-yarn-dedupe.yml

test-deps-depcheck:
name: Test deps depcheck
uses: ./.github/workflows/test-deps-depcheck.yml
test-short-suite:
needs: prep-deps
uses: ./.github/workflows/test-short-suite.yml

test-storybook:
name: Test storybook
needs: prep-deps
uses: ./.github/workflows/test-storybook.yml

validate-lavamoat-allow-scripts:
name: Validate lavamoat allow scripts
uses: ./.github/workflows/validate-lavamoat-allow-scripts.yml

validate-lavamoat-policy-build:
name: Validate lavamoat policy build
needs: prep-deps
uses: ./.github/workflows/validate-lavamoat-policy-build.yml

validate-lavamoat-policy-webapp:
name: Validate lavamoat policy webapp
needs: prep-deps
uses: ./.github/workflows/validate-lavamoat-policy-webapp.yml

prep-build-test-webpack:
needs: prep-deps
runs-on: ubuntu-latest
steps:
- name: Checkout repository
uses: actions/checkout@v4

- name: Setup environment
uses: metamask/github-tools/.github/actions/setup-environment@main
uses: metamask/github-tools/.github/actions/setup-environment@caching
with:
should-cache-restore: ${{ vars.USE_CACHING }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was the background behind choosing an env variable over harcoding?

Copy link
Contributor Author

@HowardBraham HowardBraham Jan 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pro: It's way fewer lines of code to do it this way. Setting a variable in main.yml and passing it in to a bunch of workflows is harder, much less clean, and easier to accidentally mess up.

Con: You lose some control and repeatability of workflows.

I'm kind of in a divided mind myself.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see what you mean. Other than slower performance due to the extra yarn install, are there any issues if the settings were accidentally set out out of sync?

I suspect most of the time we will want caching for these, and only in the odd case we may set a couple workflows to be false. In which case ${{ vars.USE_CACHING }} could be seen as being synonymous to true (but with a var that can be flipped without going through the peer review flow).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If prep-deps had USE_CACHING set to false, and later steps had USE_CACHING set to true, it would be a problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose another option is to just set all of these to true, and then turning it off has to happen in a PR with about 10 files, but maybe that's okay?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would probably also hard-code true. This also allows us to 100% trust that it will be cached (or not), as someone cannot silently change it in the background without anyone knowing - it will be shown in the commit history.


- run: yarn webpack --test --no-lavamoat --no-cache --browser=chrome --lockdown --sentry --snow --env production
env:
Expand All @@ -85,6 +75,7 @@ jobs:

run-tests:
name: Run tests
needs: prep-deps
uses: ./.github/workflows/run-tests.yml

run-pageload-benchmark:
Expand Down Expand Up @@ -150,21 +141,14 @@ jobs:
runs-on: ubuntu-latest
needs:
- lint-workflows
- test-lint-shellcheck
- test-lint
- test-lint-changelog
- test-lint-lockfile
- test-yarn-dedupe
- test-deps-depcheck
- test-storybook
- validate-lavamoat-allow-scripts
- test-short-suite
- validate-lavamoat-policy-build
- validate-lavamoat-policy-webapp
- run-tests
# - run-pageload-benchmark # Will enable this as a required job in a later PR
# - run-user-actions-benchmark # Will enable this as a required job in a later PR
- wait-for-circleci-workflow-status
- build-storybook
- build-beta
outputs:
PASSED: ${{ steps.set-output.outputs.PASSED }}
Expand Down
8 changes: 3 additions & 5 deletions .github/workflows/publish-prerelease.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,11 @@ jobs:
name: Publish prerelease
runs-on: ubuntu-latest
steps:
- name: Checkout repository
uses: actions/checkout@v4
- name: Setup environment
uses: metamask/github-tools/.github/actions/setup-environment@caching
with:
fetch-depth: 0 # This is needed to get merge base to calculate bundle size diff

- name: Setup environment
uses: metamask/github-tools/.github/actions/setup-environment@main
no-cache-for-release: 'true' # Turn off yarn cache as well

- name: Get merge base commit hash
id: get-merge-base
Expand Down
28 changes: 12 additions & 16 deletions .github/workflows/run-tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,10 @@ jobs:
matrix:
shard: [1, 2, 3, 4, 5, 6]
steps:
- name: Checkout repository
uses: actions/checkout@v4

- name: Setup environment
uses: metamask/github-tools/.github/actions/setup-environment@main
uses: metamask/github-tools/.github/actions/setup-environment@caching
with:
should-cache-restore: ${{ vars.USE_CACHING }}

- name: test:unit:coverage
run: yarn test:unit:coverage --shard=${{ matrix.shard }}/${{ strategy.job-total }}
Expand All @@ -39,11 +38,10 @@ jobs:
name: Webpack tests
runs-on: ubuntu-latest
steps:
- name: Checkout repository
uses: actions/checkout@v4

- name: Setup environment
uses: metamask/github-tools/.github/actions/setup-environment@main
uses: metamask/github-tools/.github/actions/setup-environment@caching
with:
should-cache-restore: ${{ vars.USE_CACHING }}

- name: test:unit:webpack:coverage
run: yarn test:unit:webpack:coverage
Expand All @@ -60,11 +58,10 @@ jobs:
name: Integration tests
runs-on: ubuntu-latest
steps:
- name: Checkout repository
uses: actions/checkout@v4

- name: Setup environment
uses: metamask/github-tools/.github/actions/setup-environment@main
uses: metamask/github-tools/.github/actions/setup-environment@caching
with:
should-cache-restore: ${{ vars.USE_CACHING }}

- name: test:integration:coverage
run: yarn test:integration:coverage
Expand All @@ -88,11 +85,10 @@ jobs:
current-coverage: ${{ steps.get-current-coverage.outputs.CURRENT_COVERAGE }}
stored-coverage: ${{ steps.get-stored-coverage.outputs.STORED_COVERAGE }}
steps:
- name: Checkout repository
uses: actions/checkout@v4

- name: Setup environment
uses: metamask/github-tools/.github/actions/setup-environment@main
uses: metamask/github-tools/.github/actions/setup-environment@caching
with:
should-cache-restore: ${{ vars.USE_CACHING }}

- name: Download artifacts
uses: actions/download-artifact@v4
Expand Down
18 changes: 0 additions & 18 deletions .github/workflows/test-deps-audit.yml
View file
Open in desktop

This file was deleted.

18 changes: 0 additions & 18 deletions .github/workflows/test-deps-depcheck.yml
View file
Open in desktop

This file was deleted.

27 changes: 0 additions & 27 deletions .github/workflows/test-lint-changelog.yml
View file
Open in desktop

This file was deleted.

Loading
Loading