AIR CLI: address review feedback on renderer, hyperlink, and test config#5968
Merged
Conversation
…#5818) ## Changes `bundle validate` (and therefore `plan`/`deploy`) now reports two UC fields that the SDK models as optional but the backend treats differently: - **`sql_warehouses.*.name`** — error `sql_warehouse name is required` (backend-confirmed as required; whitespace-only names are also rejected, matching the backend's `name.trim.nonEmpty`). - **`grants[*].principal`** — warning `grant principal is required`, on every securable that supports grants (catalogs, schemas, volumes, external_locations, registered_models, vector_search_indexes). Kept as a warning since the backend contract is unconfirmed. Implemented in the existing `validate:required` mutator (runs in `phases.Initialize()`, covering validate/plan/deploy), following the existing dashboard bespoke-validation precedent. Validation diagnostics are sorted deterministically so multiple messages have stable ordering. ## Why These fields are modeled as loosely-typed/optional (`json:"...,omitempty"`), so `bundle validate` and `bundle plan` pass while the deploy is rejected by the backend with late, low-context 400s. Discovered via fuzz testing; affects both the Terraform and direct engines. Reporting the offending field by name at validate time is much more actionable. ## Tests - New acceptance tests under `acceptance/bundle/validate/`: - `sql_warehouse_required_name/` (both engines; covers a missing name and a whitespace-only name) - `grants_required_principal/` (verifies a missing principal warns while a valid grant passes) - Refreshed the `empty_resources` golden files that surface the `sql_warehouse name is required` error.
## Changes Point the aitools skills version at thedatabricks-agent-skills v0.2.10 release, which graduates databricks-data-discovery from experimental/ to stable skills/. --------- Signed-off-by: Lennart Kats <lennart.kats@databricks.com>
## Changes Run the Changelog Preview workflow on every push to main ## Why To allow preview at any state after we fragmented changelog entries into .nextchanges/
Follow-up refinements from the [#5832](#5832) review (all were left as non-blocking there and deferred with a "will address in a follow-up" reply). The `local-env` command remains hidden until #5835, so there is no user-visible changelog entry. ## Changes - **uv install consent** (`libs/localenv/uv.go`) — `EnsureAvailable` no longer runs the remote uv installer (`curl … | sh` / `irm … | iex`) silently. It now requires consent: a truthy `DATABRICKS_LOCALENV_AUTO_INSTALL_UV` opt-in for non-interactive runs (CI/IDE), or an interactive `y/N` prompt via `cmdio.AskYesOrNo`. A non-interactive run without the opt-in returns an actionable error instead of downloading and executing an installer. (The `--debug` log of the exact installer command from #5832 is kept.) - **serverless job version** (`cmd/localenv/compute.go`) — `GetJobSparkVersion` now reads `Environments[0].Spec.EnvironmentVersion`, so a serverless `--job` resolves to its actual `serverless-vN` instead of always defaulting to v4. Empty still falls back to v4. - **double bundle load** (`cmd/localenv/sync.go`) — skip `bundleTarget` when an explicit `--cluster/--serverless/--job` flag is set. `ResolveTarget` only consults the bundle as a fallback, so the second `TryConfigureBundle` load (and its re-printed load-time diagnostics) was wasted for the explicit-flag case. - **robust `Validate` parse** (`libs/localenv/uv.go`) — the `uv run` probe now prints `PYVER:` / `DBCVER:` sentinels and parses by prefix instead of line position, so a stray stdout line from uv doesn't shift the parse. - **cleanup** (`libs/localenv/uv.go`) — fold single-caller `newUvManager` into `NewUvManager`; collapse the triplicated `resolveIndexURL` + conditional `WithEnv` into one `runUv` helper (the conditional stays so an already-set `UV_INDEX_URL` isn't clobbered). ## Tests Adds unit tests for the install consent gate (`TestConfirmUvInstall`) and the sentinel parse (`TestLineWithPrefix`). Full `libs/localenv` suite passes, including under a CI-like environment with `UV_INDEX_URL`/`PIP_INDEX_URL` set. This pull request and its description were written by Isaac.
## Changes Remove `src` folder from immutable folder paths ## Why API won't unpack the snapshot into `src` folder and instead will unpack in the root of snapshot path. ## Tests Covered by acceptance tests <!-- If your PR needs to be included in the release notes for next release, add a changelog fragment: create .nextchanges/<section>/<name>.md with a one-line description (e.g. .nextchanges/cli/quickstart.md). See .nextchanges/README.md. -->
## Summary - The two subtests (`volume_doesnot_exist`, `volume_not_deployed`) already run locally via their own `Local = true` overrides; only the group's parent `test.toml` still defaulted to `Local = false`. - Flip the parent to `Local = true` so the default matches how the tests actually run and no stale cloud-only default remains. - `RecordRequests = false` stays: it intentionally overrides the `RecordRequests = true` set by the artifacts parent `test.toml`.
## Why - The command's user-visible behavior — text and `--json` output, and every error path — needs end-to-end coverage against the real CLI. - `cmd/localenv/` carries no unit tests by design, so acceptance tests are where that surface is verified (repo convention: user-visible CLI output is covered by acceptance tests). ## What - **`acceptance/localenv/`** — 9 scenarios driven through the (hidden) command against the in-process fake server: `help` (three-level tree), `no-target` (`E_NO_TARGET`), `flag-conflict` (Cobra mutual-exclusion), `manager-unsupported` (conda project → clean P1 exit), `env-unsupported` (404 → `E_ENV_UNSUPPORTED` at fetch), `json-error` (`--output json` error object), `serverless-check` (dry-run plan), `serverless-json` (`--json` plan), and `constraints-only`. - Scripts use `local-env python sync` and the `DATABRICKS_LOCALENV_CONSTRAINT_SOURCE` override; goldens show the `local-env python sync` command field and managed marker. No source changes. ## Testing strategy - Goldens generated with `-update` and verified **stable on a clean re-run** (no `-update`); all 9 subtests pass. - `musterr` guards the five expected-failure scenarios; `trace` shows the three output-producing ones. - Full acceptance suite run to confirm no regressions elsewhere (only pre-existing, environment-specific failures unrelated to this change). - Diff confined to `acceptance/localenv/`. - Independently verified by a review subagent (PASS — goldens, scripts, stubs, stale-refs, hygiene) and by codex (no issues). --- ## About this stack This is one of a series of small, stacked PRs that together add the `databricks local-env python sync` command — it provisions a local Python environment (Python version, `databricks-connect` pin, and dependency constraints) matched to a selected Databricks compute target. The work was split from one large branch into single-concern layers so each is independently reviewable; the command is kept hidden until the final PR so nothing is user-visible mid-stack. **Review bottom-up.** Layers 1–5 have merged (the original layer-5 PR #5828 was split into 5a/5b/5c during review). | # | PR | What | Status | |---|----|------|--------| | 1 | #5823 | foundation: result types + env-key mapping | merged | | 2 | #5824 | compute-target resolution | merged | | 3 | #5826 | constraint fetch + offline cache | merged | | 4 | #5827 | formatting-preserving pyproject.toml merge | merged | | 5a | #5850 | package-manager interface + detection | merged | | 5b | #5851 | six-phase pipeline orchestrator | merged | | 5c | #5854 | --check cache purity, greenfield name, dbc insertion | merged | | 6 | #5832 | uv backend + CLI command (registered hidden) | | | 7 | **#5833 ← you are here** | acceptance tests | | | 8 | #5835 | unveil (unhide + help + changelog) | | This pull request and its description were written by Isaac.
## What `acceptance/bundle/deploy/readplan/grants-remove-principal` was failing intermittently on `main` (seen on the `linux, direct` job of the post-merge `build` run for dce783d). The diff was a pure reordering of the two entries in `remote_state.__embed__`: ``` - "principal": "[USERNAME]", + "principal": "extra@example.test", ... - "principal": "extra@example.test", + "principal": "[USERNAME]", ``` ## Why The testserver deliberately randomizes grant-assignment order — it builds the list by iterating a Go map (`libs/testserver/grants.go`), with a comment noting this is intentional because the Azure backend behaves the same way. The test wrote `remote_state.__embed__` straight into the golden without normalizing that order, so the golden matched only when map iteration happened to emit the principals in the checked-in order. This is a flaky test, not a product regression: `macos, direct` passed on the same commit, and the test passes locally on repeated runs. ## Fix Sort `__embed__` by principal in the `jq` step, matching how the sibling `acceptance/bundle/resources/grants/schemas/out_of_band_principal` test handles the same non-deterministic array. Verified with 40 consecutive local runs of both `EnvMatrix` variants. This pull request and its description were written by Isaac.
) ## Why The code in `libs/sync` conflated **listing** the files selected for sync (git-aware + include/exclude) with **syncing** them (snapshot diff, upload, progress). Callers that only want the list had to reach through the syncer: #5254 faked a partial `Sync` to call the listing method, and `validate:files_to_sync` builds a full `Sync` via `sync.New`, running side effects a validation pass shouldn't (writes `.databricks/.gitignore`, can `MkdirsByPath` the remote path). This extracts the real primitive so listing no longer depends on a syncer. ## What - Add `FileList` (`libs/sync/filelist.go`): owns the filesets and the `(git ∪ include) − exclude` merge. `NewFileList(...)` builds it, `Files(ctx)` lists. - `Sync` composes a `*FileList`; `(*Sync).GetFileList` delegates to it. - Remove the free `GetFileList`; its one caller (snapshot zip builder) builds a `FileList` directly. - Drop the implicit `["."]` default: empty paths now select nothing, matching the default engine. Every real caller already passes paths explicitly. ## Behavior Pure refactor, except an immutable-folder deploy with `sync: {paths: []}` now zips zero files instead of the whole root, aligning it with the default engine. This pull request and its description were written by Isaac.
…5948) ## Why Reading `bundle validate`'s remote-path handling (#5528) surfaced that we have no read-only way to tell a user, early, that they likely can't write to their `workspace.file_path`. Today that only fails at deploy. Reasoning about folder permissions was also split: `folder_permissions.go` owned the ancestor-walk traversal while `bundle/permissions` owned the ACL model. This lands one place for it and the reusable core for a follow-up writability warning. ## What - Consolidate the traversal and ACL model into `bundle/permissions` as `FolderACL` (walks up to the closest existing ancestor for a not-yet-created folder). - `CanWrite(user)`: CAN_EDIT+ directly or via a known group; conservative for admins and unknown group membership. - `CheckWritable(...)`: three-valued (writable / not-writable / unknown), since reading a folder ACL itself requires manage access, so "cannot write" usually surfaces as a 403 on the read. Groundwork only: no validator is wired up yet (the consuming validator is a follow-up, alongside #5528), and existing behavior is unchanged. `ValidateFolderPermissions` now calls `ResolveFolderACL` but produces the same diagnostics. Confined to `bundle/permissions`. This pull request and its description were written by Isaac.
## Changes The fake pipelines handler decoded the create/update body into `pipelines.PipelineSpec`, which has no `parameters` field — it lives on `CreatePipeline`/`EditPipeline` and is echoed back on `GetPipelineResponse.Parameters`. Decode that request-only field separately and mirror it on the stored `GetPipelineResponse`, matching backend behavior. ## Why Because the test server dropped `parameters` on read-back, a re-read never returned it and the direct engine planned a perpetual update for any pipeline that set `parameters`. This gap was found by fuzz testing. ## Tests Adds an acceptance test that deploys a pipeline with `parameters`, asserts `bundle plan` is a no-op, and confirms a redeploy issues no update call. Verified to fail without the fix.
This pull request and its description were written by Isaac.
Prep for new test envs that are all UC-enabled. The UC-gated `bundle/invariant` test was skipped on GCP only because the old GCP env had no metastore; once every env is UC-enabled it would run there and its Lakebase v1 templates fail with "Lakebase is not enabled" (400), since Lakebase v1 isn't on GCP. Exclude `database_instance`/`database_catalog`/`synced_database_table` (Lakebase v1) from all cloud runs via `CONFIG_Cloud=true`, matching the `postgres_*` block (Lakebase v2). Follow-up will add per-cloud (AWS+Azure) enablement across all Lakebase v1/v2 resources. This pull request and its description were written by Isaac.
…TORE_NAME] golden (#5956) Adds a GCP storage-URL normalization repl to `grants/volumes/test.toml` (mirroring the existing Azure one) so the `s3://[METASTORE_NAME]` golden holds on the new GCP UC test env; without it, the raw `gs://` location fails the `Cloud = true` test. No-op for AWS/local/Azure. This pull request and its description were written by Isaac.
## Why Several `bundle/generate` acceptance tests create directories on the **real workspace** (they have `Cloud = true`) and never reliably remove them, so leftovers accumulate on shared test workspaces (e.g. `azure-prod-ucws-is`), bloating `workspace list /`. This was observed contributing to a workspace-listing integration test hanging. Two distinct leaks: 1. **`genie_space`, `dashboard`, `alert`** each ran `workspace mkdirs /Workspace/test-$UNIQUE_NAME` at the **workspace root** and used it as the `parent_path` for the resource they create. Nothing deleted it: no `script.cleanup`, and the `eng-dev-ecosystem` env cleaner only sweeps `/Users`, never the root. The alert test also frequently times out (`TimeoutCloud = "5m"`, #4221). 2. **`auto-bind`** deployed its bundle to `/Workspace/tmp/$UNIQUE_NAME`. Its `bundle destroy` removes that on the happy path, but the inline `trap` only cleaned the `/Users/.../python-*` notebook dirs — so a failure before `destroy` leaked the root into the shared, unswept `/Workspace/tmp` namespace. ## What Every generate test that creates workspace state now cleans up after itself, in the swept user tree: - Relocated the three root dirs to `/Workspace/Users/$CURRENT_USER_NAME/test-$UNIQUE_NAME`, and added a `script.cleanup` to each that recursively deletes it (removing the genie space / dashboard / alert created inside). - Moved `auto-bind`'s bundle root to `~/.bundle/auto-bind-test-$UNIQUE_NAME`, matching the `~/.bundle` convention every other bundle acceptance test uses. Output goldens regenerated with `-update`; the full `bundle/generate` suite passes locally. This pull request and its description were written by Isaac, an AI coding agent.
…thon-files flag (#5892) ## Changes Re-applies #5799 (reverted in #5837) and puts the behaviour behind a new opt-in flag. `bundle generate job` only downloaded notebook tasks; files referenced by `spark_python_task` were left as absolute `/Workspace/...` paths, so the source file was never downloaded and the config wasn't portable. This PR restores the download+rewrite path (reusing the same `markFileForDownload` helper as pipeline libraries) but gates it behind a new `--download-spark-python-files` flag on `bundle generate job`, defaulting to **off**. Git-sourced files (`source: GIT`) and cloud URIs (`dbfs:/`, `s3:/`, `adls:/`, `gcs:/`) are left untouched. The flag is threaded through the `Downloader` as a functional option (`WithSparkPythonFiles`), so the pipeline, app, and import callers are unaffected. ## Why #5799 was reverted in #5837 because a Python file often imports sibling files that the downloader does not capture, so downloading only the entry point can produce a job that fails at runtime with missing imports — whereas leaving the absolute workspace path alone "just works". Making the download opt-in keeps the default safe while letting users who know their `spark_python_task` is self-contained pull the file into their bundle. ## Commits 1. **Re-apply #5799** — a single re-revert of #5837, restoring the original change verbatim (only the `NEXT_CHANGELOG.md` conflict resolved). 2. **Gate behind `--download-spark-python-files`** — the new flag, functional-option plumbing, tests, and regenerated help/changelog. ## Tests - Unit tests in `bundle/generate/downloader_test.go`: the download+rewrite path (with the option), the skipped cases (cloud URI, `source: GIT`), and a new test asserting the default-off behaviour makes no requests. - Acceptance test `acceptance/bundle/generate/spark_python_task_job` exercising the full CLI with the flag: a workspace-file task is downloaded and rewritten, a `dbfs:/` cloud-URI task is preserved. Identical output on both `terraform` and `direct` engines. - Regenerated the `bundle generate job --help` golden file. This pull request and its description were written by Isaac, an AI coding agent.
## Changes - Flip to `Local = true` / `Cloud = false`. The bare `ssh connect` (serverless CPU) now runs against the in-process testserver, whose driver-proxy `/ssh` websocket is backed by a real `sshd` — so the test asserts both the submitted bootstrap job and a full handshake + remote exec over the tunnel. - Linux-only (`[GOOS]`), skipped when `sshd` is absent, matching the sibling SSH tests. - Add a `Repls` for `databricks-cpu-<hash>`: with no `--name`, the connection name is derived from a hash of the workspace host (a random localhost port locally), so it's normalized to keep the submitted job payload deterministic. - Keep the `CLOUD_ENV` branch so the real serverless-CPU path is preserved for when cloud `ssh connect` is re-enabled. ## Why `Cloud = false` reflects that cloud `ssh connect` is currently disabled (#4838); the real coverage now comes from the local run. Consistent with `connect-serverless-gpu` and `connection`.
…te (#5958) On GCS-backed UC Volumes, a directory created implicitly via CreateParentDirectories has no standalone object, so it disappears when its last child file is deleted. `recursiveDelete` deletes files first and directories after, so on GCS the directory-delete returns 404 and fails the whole operation even though its goal (directory and contents gone) is already met. This surfaced as `databricks fs rm -r` failing on GCS volumes. Map that 404 to a not-found error via `apierr.IsMissing` and skip not-found directories in `recursiveDelete`. Correct on every backend: on AWS/Azure the directory-delete still returns 204, so the tolerant branch is never taken. Cross-verified on live test environments: `TestFilerReadWrite/files` and `TestFilerRecursiveDelete/files` now pass on GCP (previously failed) and stay green on AWS. This pull request and its description were written by Isaac. --------- Co-authored-by: Jan N Rose <janniklas.rose@gmail.com>
- Remove a hardcoded email from render_test.go (use user@example.com). - list_tui.go: use the existing cmdio.IsPromptSupported instead of the air-added IsPagerSupported; drop IsPagerSupported from libs/cmdio/io.go since it is no longer used. - format.go: standardize on termenv.Hyperlink and drop the hand-rolled osc8Link helper (and its test). - Centralize EnvMatrix.DATABRICKS_BUNDLE_ENGINE = [] into a shared acceptance/experimental/air/test.toml; remove the per-dir duplication (deleting the test.toml files left with nothing else). Co-authored-by: Isaac
Contributor
Approval status: pending
|
Collaborator
Integration test reportCommit: f7111b4
8 interesting tests: 4 RECOVERED, 4 SKIP
Top 10 slowest tests (at least 2 minutes):
|
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
8 participants
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.
Post-merge cleanup for the experimental AIR CLI (follow-up to #5847, which squash-merged
air-cliintomain). This commit was made after that merge, so it is not yet inmain.render_test.go(useuser@example.com).list_tui.go: use the existingcmdio.IsPromptSupportedinstead of the air-addedIsPagerSupported; dropIsPagerSupportedfromlibs/cmdio/io.gosince it is no longer used.format.go: standardize ontermenv.Hyperlinkand drop the hand-rolledosc8Linkhelper (and its test).EnvMatrix.DATABRICKS_BUNDLE_ENGINE = []into a sharedacceptance/experimental/air/test.toml; remove the per-dir duplication.Stacked below the
air logsport branch (air-logs-m4), which depends on the centralizedtest.tomlintroduced here.This pull request and its description were written by Isaac.