environments: don't report a fabricated databricks-connect version in setup-local --dry-run - #6218
Conversation
… setup-local --dry-run (DECO-27996) versionFromPin extracts the version substring from a dependency pin, so a major-only pin like databricks-connect~=17.0 (serverless pins by major since environments#15) yielded "17.0" — a compatible-release floor, not a version anything installs. On a real run validate overwrites dbconnectVersion with the installed version, but under --dry-run validate is stubbed, so the fabricated value leaked into the --output json contract the VS Code extension consumes. Gate the reported version behind dbcVersionFromPin, which emits only a concrete major.minor.patch version and returns "" (omitted) otherwise. versionFromPin is left untouched so dbcMajorFromPin still extracts the major from a ~=17.0 pin and the real-run major-version assertion in validate keeps working. Co-authored-by: Isaac
f97f835 to
14699f1
Compare
|
|
||
| // isFullVersion reports whether v has at least three numeric dot-separated | ||
| // components, e.g. "17.3.0" but not "17.0". | ||
| func isFullVersion(v string) bool { |
There was a problem hiding this comment.
isFullVersion isn't a true PEP 440 concrete-version check — it splits on . and only requires the first three components be all-digit. That's inconsistent at the edges:
- Non-dotted prerelease suffixes are dropped:
17.3.0rc1→parts[2]="0rc1"(not all digits) → reported as"". - Dotted suffixes are kept:
17.3.0.dev1/.post1→17/3/0all digits → kept. - Malformed input like
17.3.0.invalidis accepted.
Verdict: unlikely to bite in practice — the environments repo pins either major-only (~=17.0) or concrete GA (~=17.3.0), never prereleases. Worth at most a one-line doc-comment note that only digit-terminated major.minor.patch components count as "full". Not a blocker.
Integration test reportCommit: aa34129
8 interesting tests: 4 RECOVERED, 4 SKIP
Top 12 slowest tests (at least 2 minutes):
|
anton-107
left a comment
anton-107
left a comment
There was a problem hiding this comment.
Approving — I checked out the branch and verified the claims rather than reading the diff alone. The bug is real, live in production right now, the fix is correct, and the JSON contract stays safe.
What I verified
The bug is live for every serverless target, not just v4. Current pins in databricks/environments:
| env | pin | dry-run reported before this fix |
|---|---|---|
| serverless-v3 | ~=16.0 |
16.0 |
| serverless-v4 | ~=17.0 |
17.0 |
| serverless-v5 | ~=18.0 |
18.0 |
| serverless-v5-ml | ~=18.0 |
18.0 |
environments#15 merged 2026-08-11 07:23Z, ~50 min before this PR. The description cites only ~=17.0, which undersells the blast radius a little — every serverless env is affected.
The new test is genuinely red without the fix. I reverted just the call site (dbcVersionFromPin → versionFromPin) and got Should be empty, but was 17.0. Not a tautological test.
Real runs are untouched. dbcMajorFromPin("databricks-connect~=17.0") → "17", so validate's major assertion still works. The stated reason for not hardening versionFromPin directly holds up.
Omitting the field is contract-safe. dbconnectVersion is already omitempty, result.go:196 documents omission for constraints-only mode, and cmd/environments/output.go:97 guards on != "". Absence is not a new state for the VS Code consumer.
170 unit tests pass, gofmt and go vet clean, CI green 16/16.
Two non-blocking findings
1. The fix narrows the bug class but doesn't close it. ~=17.2.0 is also a range in PEP 440 — it means >=17.2.0, ==17.2.*, so it installs the newest 17.2.x, not necessarily 17.2.0. isFullVersion accepts it, so dry-run still reports 17.2.0, which can be just as much "a value nothing installs" as 17.0 was. Only ==17.2.0 is truly concrete. This PR fixes the acute case (a major.minor floor that is not a real release) and leaves the mild one (right major.minor, possibly wrong patch) — a strict improvement, so not a blocker. But it means the cleaner invariant is arguably "dry-run never reports an installed version, because nothing was installed", since dry-run fundamentally cannot know the patch without resolving. Worth deciding deliberately rather than inheriting by accident.
2. No acceptance coverage for the shape that is now live. Every fixture under acceptance/localenv/ pins a full version (~=17.2.0 or ~=15.4.0). The major-only pin — now the only form serverless uses in production — is covered by unit tests but never at the acceptance layer, which is the layer that snapshots the actual --output json contract the extension consumes. acceptance/localenv/serverless-json/ is therefore no longer representative of live serverless. Switching that fixture to ~=17.0 (or adding a case beside it) would pin the real contract and would have caught this originally.
Happy for both to be follow-ups — neither blocks the fix, which is a clear improvement over reporting a fabricated floor.
|
|
||
| // isFullVersion reports whether v has at least three numeric dot-separated | ||
| // components, e.g. "17.3.0" but not "17.0". | ||
| func isFullVersion(v string) bool { |
There was a problem hiding this comment.
Confirmed all three of your edge cases empirically, and found a few more — for the record:
~=17.3.0rc1 -> "" (dropped)
~=17.3.0b2 -> "" (dropped)
~=17.3.0.dev1 -> "17.3.0.dev1" (kept)
~=17.3.0.post1 -> "17.3.0.post1" (kept)
~=17.3.0.invalid -> "17.3.0.invalid" (kept)
~=17.3.0. -> "17.3.0." (kept)
~=17.3.0.1.2.3 -> "17.3.0.1.2.3" (kept)
~=17.3.00 -> "17.3.00" (kept)
Agreed this is not a blocker given real pins are either major-only or GA. Your suggested one-line doc note hasn't landed yet though — worth adding here so the next reader doesn't mistake this for a PEP 440 validator, e.g. "only digit-terminated major.minor.patch components count as full; suffixed forms like .dev1 or .post1 pass through unchecked."
Separately, and more substantive than the suffix edges: isFullVersion treats 17.2.0 as concrete, but ~=17.2.0 is still a PEP 440 range (>=17.2.0, ==17.2.*) and installs the newest 17.2.x. So a "full" pin can report a patch nothing installs — the same class of bug this PR fixes, just milder. See the main review comment; fine as a follow-up.
There was a problem hiding this comment.
Added the doc note in afdf505 — isFullVersion now states it's not a PEP 440 validator: only the leading major.minor.patch must be digit-terminated, so suffixed forms like .dev1/.post1 pass through unchecked (fine here since real pins are major-only or concrete GA).
On the more substantive ~=17.2.0-is-also-a-range point: agreed, and it's a genuine contract decision (only ==X.Y.Z truly determines the installed version). I've parked it on DECO-27996 with two candidate behaviours — (A) omit all pin-derived versions in dry-run, (B) report only exact == pins — for the team to verify whether an exact pin can ever reach this path and settle the desired --dry-run contract before implementing. Left out of this PR deliberately.
| Mode: ModeDefault, Check: true, ProjectDir: dir, | ||
| ConstraintBaseURL: srv.URL, CacheDir: t.TempDir(), | ||
| Flags: ComputeFlags{Serverless: "v4"}, | ||
| Compute: stubCompute{}, PM: fakePM{py: "3.12", dbc: "17.2.0"}, |
There was a problem hiding this comment.
Nit: dbc: "17.2.0" is dead input here — Check: true means the pipeline stops before validate, so fakePM.Validate is never called. It reads as though the value matters to the assertion. Dropping it (or leaving just fakePM{py: "3.12"}) would make it clearer that the reported version comes purely from the pin on the dry-run path.
There was a problem hiding this comment.
Done in afdf505 — dropped it to fakePM{py: "3.12"} with a comment that Check mode stops before validate, so the reported version comes purely from the pin.
| {"databricks-connect~=17.0", ""}, | ||
| {"databricks-connect~=17", ""}, | ||
| {"", ""}, | ||
| } |
There was a problem hiding this comment.
Since serverless is now 100% major-only across v3/v4/v5/v5-ml (~=16.0/~=17.0/~=18.0), consider adding the other live shapes to this table so the fix is pinned against what production actually emits:
{"databricks-connect~=16.0", ""},
{"databricks-connect~=18.0", ""},
{"databricks-connect==17.3.0", "17.3.0"},The == case is worth having explicitly — it's the only genuinely concrete pin form, and it documents that the helper accepts it.
There was a problem hiding this comment.
Added in afdf505 — the table now covers ==17.3.0 (the only genuinely concrete form), ~=16.0, and ~=18.0. Also added a serverless-json-major-pin acceptance fixture that pins the --output json contract for the now-live major-only shape and confirms dbconnectVersion is omitted; serverless-json still covers the full-pin path.
…CO-27996) Follow-ups from review of the dbcVersionFromPin fix: - Note in isFullVersion that it is not a PEP 440 validator: only the leading major.minor.patch must be digit-terminated, so suffixed forms pass through unchecked. Fine here because real pins are major-only or a concrete GA. - Drop the dead dbc value from the dry-run test: Check mode stops before validate, so fakePM.Validate never runs and the value never mattered. - Cover the live serverless pin shapes in TestDBCVersionFromPin: the exact ==17.3.0 form (the only genuinely concrete pin) plus ~=16.0 and ~=18.0, which serverless now uses across every env (environments#15). - Add the serverless-json-major-pin acceptance fixture, which snapshots the --output json contract for the now-live major-only pin and confirms dbconnectVersion is omitted. serverless-json still covers the full-pin path. Co-authored-by: Isaac
|
Thanks for the thorough review, @anton-107 — especially checking out the branch and verifying the blast radius across all serverless envs. Addressed the actionable feedback in afdf505:
Verification: gofmt / Finding #1 ( |
rugpanov
changed the title
3 participants
Summary
Found in code review of the unveil PR (#5835). P1, not P0: it only affects
--dry-run; real runs are correct.versionFromPininlibs/localenv/pipeline.goderives the reporteddbconnectVersionby taking everything from the first digit of the published pin string. A pin string is not a version:~=17.3.0happens to return17.3.0(correct).~=17.0returns"17.0"— amajor.minorfloor that nothing installs. Serverless has pinned by major (~=17.0, not~=17.3.0) since Pin serverless databricks-connect by major (~=17.0, not ~=17.3.0) environments#15 (merged 2026-08-11), so this is live now for serverless targets.On a real run
validateoverwritesdbconnectVersionwith the actually-installed version, so the reported value is correct. Under--dry-run,validateis stubbed, so the fabricated value is what gets reported — dry-run and real runs disagree, and the--output jsoncontract the VS Code extension consumes carries the wrong value in dry-run. No wrong install happens; the provisioned venv is still correct.Fix
Gate the reported version behind a new
dbcVersionFromPin, which emits a version only when the pin carries a fullmajor.minor.patch(e.g.~=17.3.0→17.3.0) and returns""(omitted from JSON) for a range-only pin such as~=17.0.The key constraint:
versionFromPinhas two callers. Hardening it directly would breakdbcMajorFromPin's major extraction for~=17.0and regress real runs (validate would fail to determine the major) — worse than the dry-run bug. So the gate is applied only to the reporting path;versionFromPinis left untouched.Tests
The ticket noted nothing tested this cross-repo coupling. Added:
TestPipelineDryRunOmitsFabricatedDBConnectVersion— end-to-end dry-run with a~=17.0pin (written red-first; reported17.0before the fix).TestDBCVersionFromPin— table-driven, including the~=17.0bare-major case.TestDBCMajorFromPinHandlesMajorOnlyPin— guards that the real-run major path still works for~=17.0.go test ./libs/localenv(170 pass) and related acceptance tests (9 pass) are green; the existing JSON goldens use a full17.2.0pin, so their output is unchanged.go vet,golangci-lint, andgofmtare clean.No changelog fragment:
setup-localis still hidden pending the unveil (#5835), matching the prior decision to drop a premature fragment for it.This pull request and its description were written by Isaac.