Uh oh!
There was an error while loading. Please reload this page.
[VPEX][6b] local-env: address #5832 follow-up review comments - #5936
Conversation
Follow-ups deferred from the #5832 review, now that the command has landed (still hidden until #5835): - uv install consent (libs/localenv/uv.go): EnsureAvailable no longer runs the remote uv installer silently. It now requires consent — a truthy DATABRICKS_LOCALENV_AUTO_INSTALL_UV opt-in for non-interactive runs, or an interactive y/N prompt (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 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 falls back to v4. - double bundle load (cmd/localenv/sync.go): skip bundleTarget entirely 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 does not 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 is not clobbered). Adds unit tests for the install consent gate and the sentinel parse. Co-authored-by: Isaac
…ions Two correctness findings from the review of the follow-up changes: - confirmUvInstall could panic: EnsureAvailable is a library entry point reachable with a context that has no cmdio (e.g. a Pipeline built with context.Background()), where cmdio.IsPromptSupported panics. Guard with cmdio.HasIO and treat a missing cmdio as non-interactive (decline). - GetJobSparkVersion used only Environments[0] for serverless jobs. A job whose environments pin differing environment_version values (or a pinned-vs-unpinned mix) has no single correct target, so refuse and ask for --serverless, mirroring the existing job-cluster divergence check. Adds a test asserting confirmUvInstall declines (no panic) with no cmdio. Co-authored-by: Isaac
Range over strings.SplitSeq instead of strings.Split: the seq variant iterates without allocating the intermediate slice, clearing the golangci-lint modernize/stringsseq finding. Co-authored-by: Isaac
anton-107
left a comment
There was a problem hiding this comment.
Reviewed the serverless-version passthrough, the runUv refactor, the confirmUvInstall consent gate (including the HasIO panic guard), and the lineWithPrefix parse — all correct, and the unit tests cover the new pieces well. Approving.
Two follow-ups, neither blocking:
1. Question on the deprecated client field vs the divergence guard.environmentVersion() reads only Spec.EnvironmentVersion. The SDK's compute.Environment also carries the deprecated Client field ("Use environment_version instead"), and interestingly the acceptance fixture in #5833 (job-ambiguous-compute) writes the environment as spec: {"client": "3"}. I confirmed the behavior with a quick test:
environmentVersion(client=2) = ""
environmentVersion(client=3) = "" // equal → divergence guard does NOT fire
single client=3 → environmentVersion="" → ResolveTarget falls back to v4
So if a serverless job pins its version via client rather than environment_version, two things happen: (a) it silently resolves to serverless-v4 regardless of the real version, and (b) a job whose environments pin differentclient values (2 vs 3) both map to "", compare equal, and slip past the new "refuse rather than guess" guard — the exact case the guard exists to catch.
Does the Jobs GET response ever return spec.client populated while environment_version is empty? If the backend normalizes the deprecated field into environment_version on read, this is a non-issue and worth a one-line comment noting client is deliberately ignored because the API always populates environment_version. If it can return client only, I'd fall back to Client (normalized) when EnvironmentVersion is empty, or flag/refuse a client-pinned environment — so the guard observes whatever field actually carries the pin. (base_environment is ignored the same way.)
2. The new serverless-from-job path has no test coverage. The headline change here — deriving serverless-vN from a job's environment_version, plus the differing-version rejection — isn't exercised at the unit level (target_test.go only stubs isServerless=false; cmd/localenv has no _test.go) or in #5833's acceptance suite (its three job scenarios are classic-single, classic-mismatch, and both-present, which short-circuits before the version code). A regression reverting serverless jobs to always-v4, or dropping the guard, would ship green.
Could we add (here or in a follow-up) a serverless --job acceptance scenario with:
- one environment carrying
environment_version(e.g."3"), asserting it resolves toenvKey serverless/serverless-v3(plus a variant with no version to exercise the v4 fallback); and - two environments with differing
environment_version, asserting the newdiffering environment_version; pass --serverless explicitlyerror.
That would also let the job-ambiguous-compute fixture switch from the deprecated client field to environment_version, so it reflects the field the code actually reads.
Minor nit: the consent prompt in confirmUvInstall hardcodes the install.sh URL, but installUv runs install.ps1 on Windows — worth selecting by runtime.GOOS (or wording it generically) since the whole point of the prompt is transparency about what will run.
…pt, tests Follow-ups from the #5936 review (all non-blocking): - environmentVersion now falls back to the deprecated Spec.Client field when Spec.EnvironmentVersion is empty. client is the predecessor of environment_version and some jobs still pin via it; reading both means the v4 fallback and the differing-version guard observe whichever field carries the pin, instead of treating a client-pinned job as unversioned (which would silently resolve to v4 and let two differing client values slip past the guard). base_environment stays ignored — it is a path/ID, not a version. The divergence error message is now field-agnostic ("differing versions"). - confirmUvInstall's prompt now names the OS-specific installer URL via a shared uvInstallerURL helper (install.ps1 on Windows, install.sh elsewhere), which installUv also uses, so the prompt is transparent about what actually runs. - Tests: environmentVersion field precedence (compute_test.go) and serverless --job version resolution + v4 fallback (target_test.go). The end-to-end differing-versions rejection through GetJobSparkVersion is best exercised as an acceptance scenario alongside the job fixtures in #5833; noted on the PR. Co-authored-by: Isaac
rugpanov
commented
Jul 16, 2026
Thanks — addressed in 3e2d456. 1. 2. Test coverage. Added unit tests here:
The end-to-end differing-versions rejection through Nit. The consent prompt now names the OS-specific installer URL ( |
eng-dev-ecosystem-bot
commented
Jul 16, 2026
Integration test reportCommit: 3e2d456
8 interesting tests: 4 RECOVERED, 4 SKIP
Top 10 slowest tests (at least 2 minutes):
|
Uh oh!
There was an error while loading. Please reload this page.
Covers the serverless-from-job path added in #5936, per the review suggestion on #5833 (the fixtures live here): - job-serverless-check: a serverless job that pins environment_version resolves to that serverless-vN (v3), not the v4 default. - job-serverless-version-mismatch: a job whose serverless environments declare differing versions is rejected with a disambiguation error. Co-authored-by: Isaac
eng-dev-ecosystem-bot
commented
Jul 16, 2026
Integration test reportCommit: 8e7ad0e
19 interesting tests: 8 FAIL, 5 RECOVERED, 3 flaky, 2 SKIP, 1 KNOWN
Top 50 slowest tests (at least 2 minutes):
|
Follow-up refinements from the #5832 review (all were left as non-blocking there and deferred with a "will address in a follow-up" reply). The
local-envcommand remains hidden until #5835, so there is no user-visible changelog entry.Changes
libs/localenv/uv.go) —EnsureAvailableno longer runs the remote uv installer (curl … | sh/irm … | iex) silently. It now requires consent: a truthyDATABRICKS_LOCALENV_AUTO_INSTALL_UVopt-in for non-interactive runs (CI/IDE), or an interactivey/Nprompt viacmdio.AskYesOrNo. A non-interactive run without the opt-in returns an actionable error instead of downloading and executing an installer. (The--debuglog of the exact installer command from [VPEX][6/8] Add local-env uv backend and hidden CLI command #5832 is kept.)cmd/localenv/compute.go) —GetJobSparkVersionnow readsEnvironments[0].Spec.EnvironmentVersion, so a serverless--jobresolves to its actualserverless-vNinstead of always defaulting to v4. Empty still falls back to v4.cmd/localenv/sync.go) — skipbundleTargetwhen an explicit--cluster/--serverless/--jobflag is set.ResolveTargetonly consults the bundle as a fallback, so the secondTryConfigureBundleload (and its re-printed load-time diagnostics) was wasted for the explicit-flag case.Validateparse (libs/localenv/uv.go) — theuv runprobe now printsPYVER:/DBCVER:sentinels and parses by prefix instead of line position, so a stray stdout line from uv doesn't shift the parse.libs/localenv/uv.go) — fold single-callernewUvManagerintoNewUvManager; collapse the triplicatedresolveIndexURL+ conditionalWithEnvinto onerunUvhelper (the conditional stays so an already-setUV_INDEX_URLisn't clobbered).Tests
Adds unit tests for the install consent gate (
TestConfirmUvInstall) and the sentinel parse (TestLineWithPrefix). Fulllibs/localenvsuite passes, including under a CI-like environment withUV_INDEX_URL/PIP_INDEX_URLset.This pull request and its description were written by Isaac.