-
Notifications
You must be signed in to change notification settings - Fork 245
fix(ci): rearrange evergreen prepare function to install Node.js earlier COMPASS-9669 COMPASS-9683 COMPASS-9687 #7193
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
Conversation
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.
Pull Request Overview
This PR restructures the Evergreen CI setup to fix a Node.js availability issue during packaged app tests by installing Node.js earlier in the process.
- Split platform environment setup into a separate reusable script
- Moved Node.js installation before environment script evaluation to ensure Node.js is available
- Consolidated artifacts directory creation into the JavaScript environment script
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
.evergreen/set-platform-env.sh |
New script containing platform detection logic extracted from print-compass-env.sh |
.evergreen/print-debug-info.sh |
New script containing debug output logic extracted from install-node.sh |
.evergreen/print-compass-env.sh |
Refactored to source platform environment from separate script |
.evergreen/print-compass-env.js |
Added artifacts directory creation and updated require statements |
.evergreen/install-node.sh |
Simplified to focus on Node.js installation by removing debug output |
.evergreen/functions.yml |
Restructured setup steps to install Node.js before environment evaluation |
.evergreen/buildvariants-and-tasks.yml |
Updated task names from macos13 to macos-11 |
.evergreen/buildvariants-and-tasks.in.yml |
Updated template task names from macos13 to macos-11 |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| const npmCacheDir = path.resolve(__dirname, '..', '.deps', '.npm-cache'); | ||
|
|
||
| printVar('ARTIFACTS_PATH', `${newPWD}/.deps`); | ||
| const artifactsPath = path.resolve(newPWD, '.deps'); |
Copilot
AI
Aug 13, 2025
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.
The variable newPWD is used but not defined in the visible code. This will cause a ReferenceError when the script runs.
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.
probably because:
const originalPWD = process.env.PWD;
let newPWD = originalPWD;
which.. should be fine? I guess we can ?? '' to shut it up. Add an assertion if that makes it feel better.
edd2186 to
7d34d7c
Compare
6375ac8 to
31b196c
Compare
31b196c to
a597c9d
Compare
| <% for (const group of E2E_TEST_GROUPS) { %> | ||
| <% if (['test-packaged-app-macos-11-arm', 'test-packaged-app-macos-11-x64'].includes(buildVariant.name)) { %> | ||
| - name: test-packaged-app-macos13-<%= group.number %> | ||
| - name: test-packaged-app-macos-11-<%= group.number %> |
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.
Oof sorry. Indeed must have been a typo. All I can think of is that maybe my brain went "13 is < 14" since mongodb latest stopping being produced for mac < 14 at 8.0.6 onwards. And 11 happens to be the only version < 14 that we're running? I don't know 🤷
|
|
||
|
|
||
| .evergreen/print-compass-env.js | ||
| # We cannot rely on node from the PATH, as the script we're calling is setting up that PATH. |
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.
👍
lerouxb
left a comment
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.
Looks good. Nice splits & renames that we should have probably done years ago. I'm happy if evergreen is happy 😄
gribnoysup
left a comment
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.
Nice fix and a clean-up at the same time, thanks!
|
(I'll merge so we can do another beta and promote it to GA later) |
Description
This is a proposal to fix the following failure on CI when running the packaged app tests.
It seems to me that the last of these two errors are from this line indicating that $ARTIFACTS_PATH is not set correctly. I believe the reason for that, is this line failing because the machine no longer has as system-wide node.js installed.
Merging this PR will:
print-compas-env.shscript to allow Node.js to be installed before its usage byprint-compas-env.js.print-compas-env.shto split out the platform specific envs intoset-platform-env.shsince that's needed for theinstall-node.shas it is right now.Checklist
Motivation and Context
Open Questions
Dependents
Types of changes