Skip to content

localenv: write [tool.databricks.environment] version on serverless setup-local - #6256

Merged
rugpanov merged 4 commits into
mainfrom
dbconnect/environment-version
Aug 13, 2026
Merged

localenv: write [tool.databricks.environment] version on serverless setup-local#6256
rugpanov merged 4 commits into
mainfrom
dbconnect/environment-version

Conversation

@rugpanov

@rugpanov rugpanov commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Summary

When databricks environments setup-local provisions or regenerates pyproject.toml against a serverless target, it now writes a [tool.databricks.environment] section carrying the resolved serverless environment_version:

[tool.databricks.environment]
environment_version = "5"

This lets the same project run interactively, in bundles, and in serverless jobs from one source of truth. It pairs with the VS Code side (DECO-27997), which reads this section as a serverless-version source.

Behavior

  • Serverless only. The version comes from the resolved compute target (--serverless-version, a serverless --job-task, or a serverless bundle target), written as a string to match the DECO-27997 example.
  • Env-owned, formatting-preserving. The new region extends the existing formatting-preserving merge (DECO-27672): environment_version is refreshed in place on regeneration — preserving indentation, any inline comment, and other user keys in the table — inserted when the table exists without it, and the whole table appended when absent. The merge stays idempotent.
  • Greenfield. RenderFreshPyproject emits the section for serverless targets.
  • Cluster targets are a deliberate no-op — with a caveat. A cluster target does not manage the section, so it is neither written nor removed. If a project was previously set up for serverless and is then re-run against a cluster, the now-stale environment_version is left in place; the command emits a W_STALE_ENVIRONMENT_VERSION warning so the stale value (which VS Code / serverless Jobs read as a source of truth) is surfaced rather than silently misleading. We intentionally do not delete the user-visible section.
  • --constraints-only still records the version. Unlike the managed databricks-connect dependency (which the mode opts out of), the environment version records the resolved compute target, which the mode still resolves — so it is written, with a code comment explaining the distinction.

Known limitation

The section is matched by its canonical spelling ([tool.databricks.environment] + a bare environment_version key). A non-canonical equivalent a user might hand-write (dotted key under [tool.databricks], inline table, quoted segment) is not recognized, so the merge would append a second definition and produce invalid TOML. This is the same pre-existing hazard [tool.uv] carries on main; a follow-up can add a containsMultilineString-style refusal for both regions together rather than guarding only this one.

Testing

  • Unit tests in libs/localenv/merge_test.go cover insert / replace / insert-key / cluster-no-op / greenfield / pre-feature-upgrade-path, all asserting valid TOML and idempotency; warnings_test.go covers the stale-version warning (and its negative cases).
  • Extended the serverless greenfield pipeline test to assert the section end-to-end.
  • Added an acceptance test (cluster-stale-environment) showing the warning text; regenerated the affected acceptance/localenv goldens. Cluster-target tests without a pre-existing section are unchanged, confirming the no-op.

DECO-27998

@eng-dev-ecosystem-bot

eng-dev-ecosystem-bot commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Integration test report

Commit: 3f5940e

Run: 31700496845

Env 💚​RECOVERED 🙈​SKIP ✅​pass 🙈​skip Time
💚​ aws linux 4 4 286 1144 5:33
💚​ aws windows 4 4 288 1142 3:17
💚​ azure linux 4 4 285 1144 5:58
💚​ azure windows 4 4 287 1142 3:32
💚​ gcp linux 1 5 286 1144 5:13
💚​ gcp windows 1 5 288 1142 3:26
8 interesting tests: 4 RECOVERED, 4 SKIP
Test Name aws linux aws windows azure linux azure windows gcp linux gcp windows
💚​ TestAccept 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R
🙈​ TestAccept/bundle/invariant/no_drift 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S
🙈​ TestAccept/bundle/resources/vector_search_endpoints/drift/recreated_same_name 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S
🙈​ TestAccept/bundle/resources/vector_search_indexes/recreate/embedding_dimension 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S
🙈​ TestAccept/ssh/connection 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S
💚​ TestFetchRepositoryInfoAPI_FromRepo 💚​R 💚​R 💚​R 💚​R 🙈​S 🙈​S
💚​ TestFetchRepositoryInfoAPI_FromRepo/root 💚​R 💚​R 💚​R 💚​R
💚​ TestFetchRepositoryInfoAPI_FromRepo/subdir 💚​R 💚​R 💚​R 💚​R
Top 6 slowest tests (at least 2 minutes):
duration env testname
3:12 azure windows TestAccept
3:12 azure linux TestAccept
3:05 aws windows TestAccept
3:00 gcp linux TestAccept
2:58 gcp windows TestAccept
2:57 aws linux TestAccept

Comment thread libs/localenv/merge.go
// is preserved when the value is replaced).
const databricksEnvironmentTable = "[tool.databricks.environment]"

var environmentVersionRe = regexp.MustCompile(`^(\s*)environment_version\s*=`)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why aren't we using a TOML library to do this merging rather than hand-rolling regexes?

Not a blocker since it seems the rest of this file does the same. This is the first of the merging code I'm reviewing so I'm curious why this approach was taken

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The merge preserves the user's file byte-for-byte outside the small managed regions — comments, key ordering, and whitespace all survive. A TOML library round-trip (decode → re-encode) reformats the whole document, drops comments, and reorders keys, which isn't acceptable for a user-owned pyproject.toml we only partially manage. So it's deliberately line-based with targeted edits, and it refuses to merge (rather than risk corruption) on shapes it can't safely edit line-wise — e.g. multi-line strings (errMultilineString).

@anton-107

Copy link
Copy Markdown
Contributor

Reviewed and probed locally (built the branch, ran the package tests, exercised mergeDatabricksEnvironment against the TOML spellings a real pyproject.toml can use). The core is sound and the change is much smaller in blast radius than #6255 — insert / replace-in-place / insert-key / cluster-no-op all behave as documented, output is valid TOML, and idempotency holds on every shape I tried including the pre-feature upgrade path. Comment and indentation preservation via trailingComment + the captured indent group works.

Three things worth addressing.

1. A non-[table] spelling of the same table produces invalid TOML

mergeDatabricksEnvironment locates the table with tableBounds(lines, "[tool.databricks.environment]"), which only matches a literal bracketed header line. TOML has other ways to define that exact table, and for those the function decides "absent" and appends a second definition. Both of these come out of the merge rejected by the TOML parser:

[tool.databricks]
environment.environment_version = "3"      # dotted key

toml: Key 'tool.databricks.environment.environment_version' has already been defined

[tool.databricks]
environment = {environment_version = "3"}  # inline table

toml: Key 'tool.databricks.environment' has already been defined

The merged file is then unparseable, so uv sync fails at the provision phase — for a user whose input file was perfectly valid. Note the merge never parses its own output in production (only the tests call requireValidTOML), so nothing catches this before uv does.

To be fair: this hazard is pre-existing for [tool.uv] on main[tool] + uv = {package = false} already produces the same duplicate-table error today, so the pattern isn't newly introduced by this PR. But this PR adds a second instance of it, and [tool.databricks] with a dotted or inline environment key is a plausible thing for a user (or a future template) to write, more so than an inline [tool.uv]. The cheap fix that fits the existing bail-out philosophy: detect a tool.databricks table containing an environment dotted/inline key and refuse the merge with an errMultilineString-style error telling the user to edit the section manually, rather than emitting a file uv will reject. MergeManaged already has precedent for "refuse rather than risk corrupting" (containsMultilineString, errNoProjectTable).

2. A stale environment_version survives a switch to a cluster target

The no-op-for-cluster behavior is deliberate and documented ("an existing one is left untouched rather than removed"), and there's a test asserting it. But consider the actual sequence: a user runs setup-local --serverless-version 5, gets environment_version = "5" written, then re-targets the project at a cluster (--cluster-id ...). The section stays, still saying 5, now describing a target the project isn't set up for. Since the PR's whole premise is that VS Code and serverless Jobs read this section as a source of truth, a stale value is worse than an absent one — the downstream reader can't tell it's stale.

I don't think silently deleting a user-visible section is right either. But the current behavior deserves at least a warning (the file now carries a serverless version while configured for a cluster), and the trade-off should be stated in the PR description rather than only in a code comment. Right now the description calls it "a no-op", which undersells it.

3. --constraints-only writes the section, unlike every other managed region

acceptance/localenv/constraints-only/output.txt shows the new section landing in constraints-only mode. That's inconsistent with how the mode treats the other env-owned value: dbcPin is explicitly cleared so databricks-connect is "neither written nor asserted", and #6255 extends the same carve-out to its consolidation pass. EnvironmentVersion is passed through unconditionally.

If constraints-only means "manage constraints, don't touch the rest of the project's identity", the environment version arguably belongs behind the same gate. If it's intentional that constraints-only still records the target version, the reasoning should be in the code next to the other mode's comment — currently there's nothing explaining why this region opts out of the carve-out, and the golden change is easy to read as accidental.

Smaller notes

  • ServerlessEnvironmentVersion() on *ComputeInfo duplicates the strings.TrimPrefix(c.ServerlessVersion, "v") that Label() already does eight lines above (result.go:181). Worth having Label() call the new method so the "v"-stripping rule lives in one place.
  • regionDatabricksEnvironment is inserted between the databricks-connect and tool.uv regions, which reorders the changed region: output in the goldens. Harmless, but it means merge-warnings/output.txt now lists the new region before tool.uv.constraint-dependencies while serverless-check lists it after — the difference comes from which regions changed, not from ordering, so it's fine; just noting it's intentional and not a golden mistake.
  • environment_version is written as a quoted string ("5"). Worth confirming with the VS Code side (DECO-27997) that it reads a string and not an integer — the PR body shows the string form, so presumably yes, but a mismatch here is a silent integration break rather than a loud one.
  • Tests assert assert.Contains(t, regions, "tool.databricks.environment") with the literal string rather than the regionDatabricksEnvironment constant that's right there in the package. Minor, but the constant is what keeps a rename honest.

Nothing here is a blocker on the mechanics — (1) is the one I'd fix before merge, and (2)/(3) are decisions that should be explicit rather than implicit.

@anton-107 anton-107 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. The core is sound — insert / replace-in-place / insert-key / cluster-no-op all behave as documented, output is valid TOML, and idempotency holds including on the pre-feature upgrade path.

One thing I'd like fixed before merge, though it's minor and not a regression: a dotted-key or inline-table spelling of [tool.databricks.environment] makes the merge append a second definition and produces invalid TOML that uv sync then rejects. It's the same pre-existing hazard [tool.uv] already has on main, so I'm not blocking on it — but it's cheap to guard following the existing containsMultilineString/errNoProjectTable "refuse rather than corrupt" precedent.

The stale-version-after-cluster-switch behavior and the --constraints-only carve-out are decisions I'd like stated explicitly (in code comments / PR description) rather than left implicit, but they're not blockers. Details in my comment above.

@rugpanov

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough local probe. Addressed in 53c9ae6:

  • Bump gopkg.in/ini.v1 from 1.66.4 to 1.66.5 #2 (stale version): added a W_STALE_ENVIRONMENT_VERSION warning when a cluster run finds a leftover serverless environment_version, rather than leaving it silently misdescribing the target. Unit + acceptance coverage added.
  • Bump github.com/hashicorp/terraform-json from 0.13.0 to 0.14.0 #3 (--constraints-only): kept writing the section but documented why — it records the resolved compute target, which the mode still resolves, unlike the managed databricks-connect dependency it opts out of.
  • Bump github.com/databrickslabs/terraform-provider-databricks from 0.5.7 to 0.5.8 #1 (dotted-key / inline-table spelling → invalid TOML): leaving as-is for now, since it's the same pre-existing hazard [tool.uv] carries on main and guarding only this section would be inconsistent; happy to file a follow-up that adds the containsMultilineString-style refusal for both regions together.
  • Small notes: Label() now reuses ServerlessEnvironmentVersion(); tests use the regionDatabricksEnvironment constant. environment_version is written as a string, matching the DECO-27997 example.

…etup-local

When `environments setup-local` provisions or regenerates pyproject.toml
against a serverless target, write a `[tool.databricks.environment]` section
carrying `environment_version` (the resolved serverless version). This lets the
same project run interactively, in bundles, and in serverless jobs from one
source of truth.

The section is env-owned: it is refreshed in place on regeneration (preserving
any inline comment and other user keys in the table) via the existing
formatting-preserving merge, and appended when absent. Cluster targets leave the
version empty, so the section is never written and any existing one is left
untouched.

DECO-27998

Co-authored-by: Isaac
…iles

Cover the common upgrade path a code review flagged: a pyproject.toml a
pre-feature CLI wrote for a serverless target already carries the managed
[tool.uv] marker block but no [tool.databricks.environment] section. Assert the
section is added without duplicating the marker block, the result is valid TOML,
and a second merge is a no-op.

Co-authored-by: Isaac
- Warn when a cluster run finds a stale [tool.databricks.environment]
  environment_version left over from an earlier serverless run
  (W_STALE_ENVIRONMENT_VERSION); the section is not managed for cluster targets,
  so the value would otherwise silently misdescribe the target.
- Document why --constraints-only still records environment_version: it reflects
  the resolved compute target, not a managed dependency like databricks-connect.
- Deduplicate the "v"-prefix stripping so Label() reuses ServerlessEnvironmentVersion().
- Tests reference the regionDatabricksEnvironment constant, and add an acceptance
  test covering the stale-version warning on a cluster target.

Co-authored-by: Isaac
@rugpanov
rugpanov force-pushed the dbconnect/environment-version branch from 53c9ae6 to 3f5940e Compare August 13, 2026 12:31
@rugpanov
rugpanov enabled auto-merge August 13, 2026 12:36
@rugpanov
rugpanov added this pull request to the merge queue Aug 13, 2026
Merged via the queue into main with commit 76985e6 Aug 13, 2026
23 checks passed
@rugpanov
rugpanov deleted the dbconnect/environment-version branch August 13, 2026 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants