localenv: write [tool.databricks.environment] version on serverless setup-local - #6256
Conversation
Integration test reportCommit: 3f5940e
8 interesting tests: 4 RECOVERED, 4 SKIP
Top 6 slowest tests (at least 2 minutes):
|
| // is preserved when the value is replaced). | ||
| const databricksEnvironmentTable = "[tool.databricks.environment]" | ||
|
|
||
| var environmentVersionRe = regexp.MustCompile(`^(\s*)environment_version\s*=`) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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).
|
Reviewed and probed locally (built the branch, ran the package tests, exercised Three things worth addressing. 1. A non-
|
anton-107
left a comment
There was a problem hiding this comment.
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.
|
Thanks for the thorough local probe. Addressed in 53c9ae6:
|
…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
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
53c9ae6 to
3f5940e
Compare
Summary
When
databricks environments setup-localprovisions or regeneratespyproject.tomlagainst a serverless target, it now writes a[tool.databricks.environment]section carrying the resolved serverlessenvironment_version: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-version, a serverless--job-task, or a serverless bundle target), written as a string to match the DECO-27997 example.environment_versionis 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.RenderFreshPyprojectemits the section for serverless targets.environment_versionis left in place; the command emits aW_STALE_ENVIRONMENT_VERSIONwarning 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-onlystill records the version. Unlike the manageddatabricks-connectdependency (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 bareenvironment_versionkey). 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 onmain; a follow-up can add acontainsMultilineString-style refusal for both regions together rather than guarding only this one.Testing
libs/localenv/merge_test.gocover insert / replace / insert-key / cluster-no-op / greenfield / pre-feature-upgrade-path, all asserting valid TOML and idempotency;warnings_test.gocovers the stale-version warning (and its negative cases).cluster-stale-environment) showing the warning text; regenerated the affectedacceptance/localenvgoldens. Cluster-target tests without a pre-existing section are unchanged, confirming the no-op.DECO-27998