-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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: Adding node-linux-medium executor #25476
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. |
683fc5c
to
2eedad6
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #25476 +/- ##
========================================
Coverage 70.44% 70.44%
========================================
Files 1274 1274
Lines 44078 44078
Branches 12453 12453
========================================
Hits 31049 31049
Misses 13029 13029 ☔ View full report in Codecov by Sentry. |
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'll need to follow up on this PR in a week or so to measure credit usage to be sure we are saving $$$$$s
Builds ready [2eedad6]
Page Load Metrics (170 ± 227 ms)
Bundle size diffs
|
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.
LGTM
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.
This should reduce credits per workflow, since Linux VM medium costs 10 credits per minute, and Docker Medium+ costs 15 credits per minute. As long as we don't somehow expand our minutes by 50%.
https://circleci.com/product/features/resource-classes/
There are a few more jobs that I noticed can use node-linux-medium
:
- prep-build
- prep-build-mv2
- prep-build-mmi
- prep-build-flask
- prep-build-flask-mv2
- prep-build-test-mmi-playwright
.circleci/config.yml
Outdated
@@ -13,6 +13,12 @@ executors: | |||
resource_class: medium | |||
environment: | |||
NODE_OPTIONS: --max_old_space_size=3072 | |||
node-linux-medium: | |||
machine: | |||
image: ubuntu-2204:2024.04.4 |
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're in an Ubuntu LTS transition period. I think we might as well go with the next version, which is
ubuntu-2404:2024.05.1
0d2c654
2eedad6
to
0d2c654
Compare
Builds ready [0d2c654]
Page Load Metrics (50 ± 4 ms)
Bundle size diffs
|
0d2c654
to
e4472a3
Compare
image: ubuntu-2404:2024.05.1 | ||
resource_class: medium #// linux medium: 2 CPUs, 7.5 GB RAM, 10 credits/min | ||
environment: | ||
NODE_OPTIONS: --max_old_space_size=6144 |
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.
Curious that we're allowing for a larger heap here than on medium+
. I guess there should be adequate room though, as long as we're not running multiple Node.js processes in one job.
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.
LGTM!
Builds ready [e4472a3]
Page Load Metrics (220 ± 245 ms)
Bundle size diffs
|
Description
This PR implements a suggestion from @davidmurdoch to use linux machine image to reduce CI costs on the build steps in our pipeline. Starting with the build steps, we want to optimize for cost for any tests that can support a linux executor.
Related issues
Fixes: #25229
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist