DRIVERS-3590 Improve dev workflow - #815
Draft
blink1073 wants to merge 11 commits into
Draft
Conversation
Add `make install` (pre-commit + python CLIs) and `make lint` targets so contributors can bootstrap and lint without going through the orchestration setup script. Repurpose `make test` to check connectivity to a running server, moving the old stub JSON writer to `make test-results`. Node.js is now installed on-demand inside mongodb_runner.py only when mongodb-runner is actually invoked and npm isn't already available, instead of being installed unconditionally by orchestration/setup.sh. This means `make install` no longer requires Node.
…ndling Keep `make test` as the original Evergreen-compatible stub (this repo's own CI relies on it writing test-results.json via a global post-task step), and move the real connectivity check to a new `make test-connect` target instead. Also make the `install` target's pre-commit setup robust regardless of the caller's PATH: pin UV_TOOL_BIN_DIR to the repo-local .bin dir (already used for the bootstrapped uv binary) and run the uv install + pre-commit install steps in a single shell so the PATH/UV_TOOL_BIN_DIR overrides actually apply to both.
Replace the tests.yml workflow's inline mongosh ping with `make test-connect`, so CI exercises the same connectivity-check path a contributor would run locally. check-connection.sh now accepts optional extra mongosh args via $MONGOSH_EXTRA_ARGS (used here for the SSL cert flags), rather than positional args, since it's sourced (not just executed) by atlas/setup.sh and serverless/setup.sh where inherited $@ would leak unrelated caller arguments into the mongosh call. Left the pre-commit and eslint jobs as-is: pre-commit/action already installs+runs+caches pre-commit more efficiently than make install/lint would (which would also pull in the unrelated Python CLI installs), and eslint is unrelated to the new Python-focused targets.
Replace the duplicated inline mongosh ping calls in test-ocsp.sh and test-data-lake.sh with the shared check-connection.sh, passing the TLS/binary/URI overrides via env vars (MONGODB_URI, MONGODB_BINARIES, MONGOSH_EXTRA_ARGS) instead of duplicating the mongosh invocation. test-mongodb-runner.sh's connect_mongodb() is intentionally left as-is: it needs custom eval commands and an expected-failure assertion (the auth-rejection check) that check-connection.sh doesn't support, so it isn't a good fit for consolidation. .evergreen/config.yml needed no changes, since it only dispatches to these scripts and has no inline connectivity-check logic of its own.
blink1073
changed the title
…ismatch npm's "latest" dist-tag now resolves to npm@12, which requires Node >=22.22.2/24.15.0/26.0.0 and fails outright against the Node 20 this script installs by default. npm@10 explicitly supports Node >=18.17.0 || >=20.5.0, so extend the existing pinning logic to cover the 18-21 range instead of only 18-19, and reserve "latest" for Node >=22 where it's actually compatible. Reproduced locally: `npm install --silent --global npm@latest` against Node 20 fails with EBADENGINE; `npm install --silent --global npm@10` succeeds cleanly.
test-mongodb-runner-partial still fails on some hosts with "--experimental-global-webcrypto is not allowed in NODE_OPTIONS", even though node_major should be 20 (not <19) and shouldn't trigger this flag at all. Reproduced Node v20.20.2 locally and confirmed the flag works fine via NODE_OPTIONS on that exact version, so something in this environment is causing the node_major check to behave unexpectedly. Logging node_bin/PATH/node_ver/node_major to see the real values on the next CI run. To be removed once the cause is confirmed.
Diagnostics showed the real bug: on some hosts (amazon2023, rhel8-power8) an unrelated, ancient Node (v8.11.3, bundled at /opt/node for other tooling) sits on PATH ahead of anything we'd install ourselves. The old check only asked "does npm exist anywhere on PATH", so it found that stale npm and skipped installing Node entirely. mongodb-runner's own `npm install` step still succeeded against it (old npm can still resolve and download packages), but actually running mongodb-runner then failed, since Node 8 is far too old to even recognize the --experimental-global-webcrypto flag our code correctly tries to add for Node <19 - hence "not allowed in NODE_OPTIONS". _install_mongodb_runner() now checks that node itself resolves and is >=16 (_node_is_usable()) before trusting it, instead of just checking for npm's presence. This forces our own Node install whenever the node on PATH is too old, regardless of what else is present. Removes the temporary diagnostic logging added in the previous commit now that the cause is confirmed.
…lint # Conflicts: # .evergreen/install-node.sh # .evergreen/orchestration/mongodb_runner.py
There was a problem hiding this comment.
Pull request overview
This PR improves the contributor/dev workflow by introducing Makefile targets that mirror CI behavior (install/lint/connectivity), and by refactoring connectivity checks into a shared script while making Node.js installation lazy (only when mongodb-runner is invoked).
Changes:
- Add
make install,make lint, andmake test-connecttargets and update CONTRIBUTING docs to point at them. - Centralize mongosh connectivity validation in
.evergreen/check-connection.shand reuse it from GitHub Actions + Evergreen test scripts. - Make Node installation lazy for mongodb-runner by installing Node from
mongodb_runner.pyonly when needed (and removing eager Node setup from orchestration setup).
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| Makefile | Adds install/lint/test-connect targets for local workflows. |
| CONTRIBUTING.md | Updates contributor instructions to use the new Make targets. |
| .github/workflows/tests.yml | Uses make test-connect to validate cluster connectivity in CI. |
| .evergreen/tests/test-ocsp.sh | Reuses shared connectivity check instead of inline mongosh invocation. |
| .evergreen/tests/test-data-lake.sh | Reuses shared connectivity check instead of inline mongosh invocation. |
| .evergreen/orchestration/setup.sh | Removes eager Node installation; documents lazy Node behavior for mongodb-runner. |
| .evergreen/orchestration/mongodb_runner.py | Adds lazy Node installation logic when mongodb-runner is installed/used. |
| .evergreen/check-connection.sh | Adds support for passing extra mongosh args (e.g., TLS flags) via env var. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Makefile: `make lint` now puts the repo-local .bin dir on PATH, so it can find the pre-commit that `make install` installs there. Previously a contributor with no global pre-commit got exit 127 from `make lint`. Also switch $$PWD to $(CURDIR) and join the install steps with && . - mongodb_runner.py: align _MIN_NODE_MAJOR with mongodb-runner's actual Node >=18 requirement (was 16, so a Node 16/17 host was considered "usable" while _mongodb_runner_supported() rejected it), and factor the duplicated version parsing into _node_major_version() so both checks share one constant. - test-ocsp.sh / test-data-lake.sh: run check-connection.sh as a subprocess instead of sourcing it, so it no longer overwrites the caller's SCRIPT_DIR or imposes `set -eu` on the caller. - test-data-lake.sh: fix the cleanup path, which concatenated two absolute paths and so never removed the download dir. - orchestration/setup.sh: use $_HERE instead of the never-set $HERE in the node-artifacts check, which expanded to /node-artifacts and made the guard always fall through to reinstalling Node. - check-connection.sh: document why MONGOSH_EXTRA_ARGS is unquoted and add an explicit SC2086 disable.
_mongodb_runner_supported() ran before any Node install and judged support from whatever "node" happened to be first on PATH. With the eager install removed, hosts that ship an unrelated ancient Node (amazon2023 and the Windows hosts have Node 8 at /opt/node) were reported unsupported, so run() silently fell back to mongo-orchestration. The test-mongodb-runner tasks stayed green while no longer exercising mongodb-runner at all, and the Node install inside _install_mongodb_runner() was never reached on exactly the hosts it was written for. Split the ensure-Node step into _ensure_usable_node() and call it from the support check, after the cheap RHEL7 platform test so old-glibc hosts still short-circuit without downloading anything. A host with a stale Node now installs a supported one and uses mongodb-runner; a host that genuinely cannot get Node 18+ still degrades gracefully to mongo-orchestration instead of raising. Also fold the duplicated node --version parsing in start_mongodb_runner() into _node_major_version(), and restore an explicit error when npm is missing next to an otherwise usable Node.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ref: DRIVERS-3590
Summary
Adds
make install,make lint, andmake test-connecttargets so contributors have simple, documented commands for local setup, linting, and connectivity checks that mirror what CI does. Node.js installation is now lazy (only whenmongodb-runneris actually used), somake installdoes not require Node. GitHub tests and the Evergreen test scripts are updated to use the new targets/shared connectivity-check logic for consistency.This PR is a step toward adding an
AGENTS.mdfile with clear instructions for running tasks.Test plan
make installworks with no Node,uv, orpre-commitpreinstalledmake lintandmake test-connectbehave as expectedmake teststill produces the stub results file Evergreen CI depends on