-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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.
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
base: main
Are you sure you want to change the base?
Conversation
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
c9f1e1e
to
6266945
Compare
0f6ed27
to
1283bfe
Compare
Builds ready [1283bfe]
Page Load Metrics (1641 ± 69 ms)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left one question & reviewed usage of caching from a security POV. No concerns from our end. I'll let the others chime in with regards to reviewing the functionality
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 }} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
(Not blocking) Are there plans to version changes? While for now pointing directly to the main branch allows for immediate updates, it can result in challenges down the road if a breaking change is introduced. Could be handy to have things set up now to save time in the long run. |
@@ -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 |
There was a problem hiding this comment.
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
.github/workflows/build-beta.yml
Outdated
@@ -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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I see what you mean. I split this into two actions: setup-environment
and setup-environment-secure
.
- name: Setup environment | ||
uses: metamask/github-tools/.github/actions/setup-environment@main | ||
uses: metamask/github-tools/.github/actions/setup-environment@caching |
There was a problem hiding this comment.
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
@@ -15,62 +15,52 @@ on: | |||
merge_group: | |||
|
|||
jobs: | |||
prep-deps: |
There was a problem hiding this comment.
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
workflow_call: | ||
|
||
jobs: | ||
test-short-suite: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the performance gains, but it does not feel "clean" to do so many things in one workflow
1283bfe
to
2bfd0b8
Compare
6266945
to
abcd5a1
Compare
b6bed4a
to
cca0088
Compare
2bfd0b8
to
b6e58b6
Compare
Builds ready [b6e58b6]
Page Load Metrics (1643 ± 73 ms)
|
e1404b6
to
61028ec
Compare
b6e58b6
to
8dce587
Compare
c786ac5
to
1fa54b9
Compare
Builds ready [1fa54b9]
Page Load Metrics (1729 ± 53 ms)
Bundle size diffs
|
Description
Major changes
prep-deps
runs first and caches the result, thenprep-build-test-webpack
makes an artifact, then the benchmarks run.prep-deps
setup-environment@caching
and deleting the independent "Checkout repository" stepmain.yml
for how to toggle it. We can discuss implementing this in a different way, but passing this variable through different workflows is actually kind of a pain (that's the first way I implemented it). Several of the jobs are 30-60 seconds faster when this is turned on.if: always()
statements make all independent tests run, even if one fails.test-lint-shellcheck, test-lint-changelog, test-lint-lockfile, test-deps-audit, test-yarn-dedupe, test-deps-depcheck, validate-lavamoat-allow-scripts
Prerequisites to merging this PR
caching
branch ofgithub-tools
: https://github.com/MetaMask/github-tools/blob/caching/.github/actions/setup-environment/action.ymlsetup-environment@caching
back tosetup-environment@main
Related issues
Split off from: #29955