Repository navigation
Detect npm and pnpm for apps validation - #6892
Conversation
Integration test reportCommit: 4b2f4fe
Top 17 slowest tests (at least 2 minutes):
|
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The implementation is focused, cross-platform, and comprehensively covered by unit and acceptance tests.
Review effort: Balanced
Findings: None
What changed in this PR
Adds npm/pnpm lockfile detection to app validation and deploy-time validation, with explicit handling for unsupported or conflicting lockfiles.
Changes:
- Detect npm or pnpm and execute manager-specific install/scripts.
- Skip absent scripts and reject unsupported/conflicting lockfiles.
- Add cross-platform unit and acceptance coverage.
| File | Description |
|---|---|
libs/apps/validation/package_manager.go |
Implements package-manager detection. |
libs/apps/validation/package_manager_test.go |
Tests lockfile detection and errors. |
libs/apps/validation/nodejs.go |
Runs validation with the detected manager. |
libs/apps/validation/nodejs_test.go |
Tests execution, skipping, and failures. |
libs/apps/validation/testdata/package-manager |
Adds Unix test stub. |
libs/apps/validation/testdata/package-manager.cmd |
Adds Windows test stub. |
cmd/apps/validate.go |
Documents package-manager behavior. |
acceptance/apps/validate/test.toml |
Configures acceptance testing. |
acceptance/apps/validate/out.test.toml |
Records generated test configuration. |
acceptance/apps/validate/script |
Exercises validation and deployment scenarios. |
acceptance/apps/validate/output.txt |
Captures expected acceptance output. |
acceptance/apps/validate/databricks.yml |
Defines the test bundle. |
acceptance/apps/validate/package.json |
Defines validation scripts. |
acceptance/apps/validate/pnpm-lock.yaml |
Selects pnpm in the main fixture. |
acceptance/apps/validate/bin/pnpm |
Adds Unix pnpm stub. |
acceptance/apps/validate/bin/pnpm.cmd |
Adds Windows pnpm stub. |
acceptance/apps/validate/app/package.json |
Supports --path coverage. |
acceptance/apps/validate/conflicting/package.json |
Defines the conflict fixture. |
acceptance/apps/validate/conflicting/package-lock.json |
Adds npm conflict lockfile. |
acceptance/apps/validate/conflicting/pnpm-lock.yaml |
Adds pnpm conflict lockfile. |
acceptance/apps/validate/unsupported/package.json |
Defines the unsupported-manager fixture. |
acceptance/apps/validate/unsupported/yarn.lock |
Exercises Yarn rejection. |
acceptance/apps/validate/invalid/package.json |
Exercises malformed JSON handling. |
.nextchanges/cli/apps-validate-package-managers.md |
Adds the user-facing changelog entry. |
Files not reviewed (3)
- acceptance/apps/validate/conflicting/package-lock.json: Generated file
- acceptance/apps/validate/conflicting/pnpm-lock.yaml: Generated file
- acceptance/apps/validate/pnpm-lock.yaml: Generated file
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
6e9fb17 to
435327e
Compare
simonfaltum
left a comment
There was a problem hiding this comment.
Thanks for this, the detection logic is easy to follow. Running the PR binary against real npm 11 and pnpm 10.30.3 turned up a few cases where they behave differently from the test stubs, and a couple where apps deploy now fails for projects that deploy fine today. Details inline.
The main one imo is the script lookup in nodejs.go. Delegating to <manager> run --if-present <script> would fix three of the inline comments at once.
The nits are small cleanups and not blocking 😄
df4b5ef to
589eed6
Compare
simonfaltum
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround, the run --if-present switch cleaned this up a lot. A few new things from the latest push, inline.
I noticed the packageManager and workspace-root lockfile threads were resolved without code changes. Are those planned as follow-ups?
Validation steps can rewrite package.json (e.g. type generation adding scripts), so reload the manifest before each script lookup instead of reading it once. Strip a leading UTF-8 BOM before decoding to match npm's parser. Co-authored-by: Isaac <no-reply@databricks.com>
Apps validation now runs each script step as `<mgr> run --if-present <script>`, so npm and pnpm decide whether a script is runnable instead of the CLI re-implementing that rule. The presence check that drives the "Skipped" line now decodes scripts into map[string]any and counts only non-empty strings, matching the managers: a script set to null, "", or a non-string value (like a "//" comment array) is treated as absent and no longer crashes decoding or forces a false failure. DetectPackageManager no longer fails on conflicting or unsupported lockfiles. A stray yarn/bun lockfile, or a second manager's lockfile, is logged and ignored (npm keeps precedence), so a lockfile a sibling command wrote never blocks a deploy that works today. The LookPath precheck now tolerates exec.ErrDot, so a manager resolved from a relative PATH entry such as node_modules/.bin is accepted; the shell that runs the command resolves it fine. Co-authored-by: Isaac <no-reply@databricks.com>
0539963 to
65f40f5
Compare
## Release v1.20.0 ### Notable Changes * Remove the Terraform deployment engine. `bundle.engine: terraform` and `DATABRICKS_BUNDLE_ENGINE=terraform` now error, and a failed migration of existing Terraform state is reported as an error instead of falling back to Terraform. To keep deploying with Terraform, use Databricks CLI v1.19.x. ([#6888](#6888), [#6889](#6889)) ### CLI * `databricks aitools install` now supports Kiro, installing Databricks agent skills into its skills directory. ([#6908](#6908)) * Fixed `databricks api` corrupting integers larger than 2^53 (such as job and pipeline ids) — request bodies and responses now preserve them exactly. ([#6884](#6884)) * Added `--auth-mode` and `--set <plugin>.<resourceKey>.authMode=obo|sp|both` to `databricks apps init` so AppKit resources can be accessed on behalf of the user, by the service principal, or both. The default stays service principal. ([#6886](#6886)) * `databricks apps init` now requires a value for every field a service principal resource binding references, prompting for missing values in an interactive terminal and otherwise failing with the `--set` key to use, instead of creating a project with unset variables. ([#6903](#6903)) * Add `databricks apps init --package-manager <npm|pnpm>` to select the package manager for Node.js templates. Infer the default quietly from template lockfiles and AppKit version, check prerequisites before creating files, and preserve template formatting and pnpm version pins. ([#6902](#6902)) * Select npm or pnpm from `packageManager` declarations and lockfiles for `apps validate` and project validation during `apps deploy`. ([#6892](#6892)) * Fix `auth docker host` reporting the credential helper as configured when its executable is missing from `PATH`. ([#6880](#6880)) * Warn when the CLI binary was built more than 6 months ago and recommend updating. ([#6898](#6898)) ### AI Runtime * Add an experimental rank-partitioned container images to AI Runtime jobs. ([#6841](#6841)) * Support snapshot fields directly under `code_source` without requiring `type` or a nested `snapshot` block. ([#6927](#6927)) * Map AIR priority and Unity Catalog image fields when converting run configurations to bundles. ([#6905](#6905)) * Add workspace backend validation to `air run --dry-run`. ([#6934](#6934)) ### Bundles * Warn that `bundle.terraform` is deprecated and has no effect since the Terraform deployment engine was removed. ([#6940](#6940)) * Direct engine now detects and applies an explicitly configured zero-value boolean or float (e.g. `gcp_attributes.use_preemptible_executors: false`, `azure_attributes.spot_bid_max_price: 0`) added to a resource first deployed without the field, matching the existing handling of an explicit integer zero. ([#6882](#6882)) * Fix `bundle deployment migrate` failing with "no such file or directory" when the Terraform state has no resources or the configuration no longer declares any of them. ([#6958](#6958)) * `bundle run` and `pipelines run` now send the per-update `development` parameter for pipelines in development mode targets. Setting `development` on a pipeline is deprecated and now emits a warning; use `mode: development` instead. ([#6863](#6863)) * Remove the hidden `bundle debug terraform` command. ([#6933](#6933)) * Add support for `run_as.group_name` at the bundle and target levels for jobs and pipelines. ([#6676](#6676)) * Fix recreating a secret scope that was deleted outside of the bundle with the direct deployment engine. ([#6970](#6970)) * Accept title-case booleans (`True`/`False`, as rendered by Azure Pipelines) for boolean variables, and accept the same boolean strings (`yes`/`no`, `on`/`off`, ...) in Python bundles as in YAML. ([#6942](#6942)) ### Dependency Updates * Bump `github.com/databricks/databricks-sdk-go` from v0.182.0 to v0.185.0. ([#6928](#6928))
Changes
Detect npm or pnpm from project lockfiles for
apps validateand project validation duringapps deploy. Warn and ignore Yarn/Bun lockfiles, prefer npm with a warning when npm/pnpm lockfiles conflict, and default to npm when no supported lockfile exists.Run optional scripts through
--if-presentso npm workspace configuration is honored, and resolve the package manager in the selected project directory.Why
Support current npm templates and upcoming pnpm templates while preserving workspace validation and
--pathbehavior.Tests
./task fmt,./task checks, and./task lint.This PR was written with Codex.