Skip to content

[VPEX][3/8] Add local-env constraint fetch with offline cache#5826

Merged
rugpanov merged 15 commits into
mainfrom
dbconnect/03-constraints
Jul 7, 2026
Merged

[VPEX][3/8] Add local-env constraint fetch with offline cache#5826
rugpanov merged 15 commits into
mainfrom
dbconnect/03-constraints

Conversation

@rugpanov

@rugpanov rugpanov commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Why

  • Once a target resolves to an env key, local-env needs the pinned Python version, databricks-connect version, and dependency constraints published for that key.
  • The fetch must degrade gracefully offline and distinguish “this environment isn't published” from “the network is down,” because those call for different user action.
  • The artifact host must be a Databricks-owned, access-controlled location and must never default to a personal repo — whoever controls the host controls what the CLI installs.

What

  • constraints.go — fetches the per-environment pyproject.toml, parses requires-python, the databricks-connect pin, and [tool.uv] constraint-dependencies, and caches it on disk.
  • Host parameterization — no host is hardcoded: RepoConstraintBaseURL reads the repo (owner/name) from the temporary DATABRICKS_LOCALENV_CONSTRAINT_REPO env var and builds a raw.githubusercontent.com/<repo>/main URL; the built-in default is empty. When unset it returns "" and FetchConstraints reports the missing source as a fetch-phase E_FETCH error (so there is no untrusted default, and the failure flows through the normal phase/JSON reporting). Once databricks/environments can publish, that becomes the hardcoded default and the env var is no longer required.
  • Failure classification: 404E_ENV_UNSUPPORTED (no cache fallback — a distinct non-transient condition); transport / non-404E_FETCH with fallback to the last-good cached copy.
  • Robustness: validate the body (parse + require requires-python) before caching so a bad 2xx can't poison the cache; atomic cache write (mkdir + temp-file + rename); dedicated http.Client with a 30s timeout; body read bounded by io.LimitReader at 1 MiB; databricks-connect matched by leading package name under PEP 503 normalization (so Databricks_Connect matches, databricks-connectors does not); cache filename = readable slug + sha256 suffix to prevent collisions.

Testing strategy

  • Unit tests with an httptest server: 200-parse, 404 → E_ENV_UNSUPPORTED, transport failure + cache fallback, missing-requires-python rejection, PEP 503 name matching, cache-dir creation, collision-free filenames, oversized-body rejection (constraints_test.go).
  • Host resolution: TestRepoConstraintBaseURL (env var → URL, unset → "", whitespace treated as unset) and TestFetchConstraintsNoSourceConfigured (empty host → E_FETCH naming the env var).
  • Gates: go build, go test, golangci-lint, deadcode, gofmt — all green.
  • Reviewed with codex to a clean pass (several fetch/cache edge-case fixes landed from review).

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. Each PR targets the previous one as its base branch, so its diff shows only that layer.

# PR What
1 #5823 foundation: result types + env-key mapping
2 #5824 compute-target resolution
3 #5826 ← you are here constraint fetch + offline cache
4 #5827 formatting-preserving pyproject.toml merge
5 #5828 six-phase pipeline + detection + package-manager interface
6 #5832 uv backend + CLI command (registered hidden)
7 #5833 acceptance tests
8 #5835 unveil (unhide + help + changelog)

This pull request and its description were written by Isaac.

@eng-dev-ecosystem-bot

eng-dev-ecosystem-bot commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator

Integration test report

Commit: 4f83973

Run: 28852927145

Env 💚​RECOVERED 🙈​SKIP ✅​pass 🙈​skip Time
💚​ aws linux 4 4 233 1055 4:55
💚​ aws windows 4 4 235 1053 6:11
💚​ aws-ucws linux 4 4 317 973 5:22
💚​ aws-ucws windows 4 4 319 971 5:46
💚​ azure linux 4 4 230 1055 5:06
💚​ azure windows 4 4 232 1053 5:57
💚​ azure-ucws linux 4 4 316 971 5:50
💚​ azure-ucws windows 4 4 318 969 6:17
💚​ gcp linux 4 4 229 1057 5:00
💚​ gcp windows 4 4 231 1055 6:13
8 interesting tests: 4 RECOVERED, 4 SKIP
Test Name aws linux aws windows aws-ucws linux aws-ucws windows azure linux azure windows azure-ucws linux azure-ucws windows gcp linux gcp windows
💚​ TestAccept 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R
🙈​ TestAccept/bundle/invariant/no_drift 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S
🙈​ TestAccept/bundle/resources/vector_search_endpoints/drift/recreated_same_name 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S
🙈​ TestAccept/bundle/resources/vector_search_indexes/recreate/embedding_dimension 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S
🙈​ TestAccept/ssh/connection 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S
💚​ TestFetchRepositoryInfoAPI_FromRepo 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R
💚​ TestFetchRepositoryInfoAPI_FromRepo/root 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R
💚​ TestFetchRepositoryInfoAPI_FromRepo/subdir 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R
Top 10 slowest tests (at least 2 minutes):
duration env testname
5:20 gcp windows TestAccept
5:17 azure-ucws windows TestAccept
5:01 aws windows TestAccept
5:00 azure windows TestAccept
4:54 aws-ucws windows TestAccept
3:11 azure-ucws linux TestAccept
3:03 azure linux TestAccept
3:02 aws linux TestAccept
3:01 gcp linux TestAccept
2:58 aws-ucws linux TestAccept

First of a stacked series adding `databricks local-env python sync`, which
provisions a local Python environment matched to a Databricks compute
target. The feature lands across small, single-concern PRs; each layer is
independently reviewable and adds no user-facing surface until the final PR
wires the command in.

This PR is the foundation the rest of the stack builds on:

- result.go: the result types and the --json / E_* error contract that
  every phase reports through (Result, PipelineError, ErrorCode, PhaseName,
  PhaseStatus, Mode, TargetInfo, ResolvedInfo, Plan, Warning), plus the
  command-path constants (local-env / python / sync) defined once.
- envkey.go: mapping a compute target to an environment key and parsing the
  Python minor from a requires-python specifier.

Nothing imports this package yet, so the CLI is unchanged. The unexported
filesystem/artifact constants and the canonical phase-order slice live with
the pipeline that consumes them (a later PR in the stack).

Co-authored-by: Isaac
@rugpanov rugpanov force-pushed the dbconnect/02-target branch from 5fdb8b6 to 2092138 Compare July 3, 2026 13:20
@rugpanov rugpanov force-pushed the dbconnect/03-constraints branch from d867d1f to 64d8996 Compare July 3, 2026 13:20
@rugpanov rugpanov temporarily deployed to test-trigger-is July 3, 2026 13:21 — with GitHub Actions Inactive
@rugpanov rugpanov temporarily deployed to test-trigger-is July 3, 2026 13:21 — with GitHub Actions Inactive
@rugpanov rugpanov changed the title Add dbconnect constraint fetch with offline cache [VPEX][3/8] Add local-env constraint fetch with offline cache Jul 3, 2026
…pecifiers

Review of the foundation layer flagged that PythonMinorFromRequires took the
first MAJOR.MINOR in the string via first-match regex. For a multi-clause
requires-python where the exclusive upper bound comes first — e.g.
"<3.13,>=3.10" — it returned 3.13, the version the "<3.13" clause forbids,
because PEP 440 clause order is arbitrary. The result feeds
PM.EnsurePython, so the tool could target a Python the constraint excludes.

Prefer a lower-bound / pinning clause (>=, >, ==, ~=, ===) and only fall
back to the first version when none is present. Adds multi-clause test
coverage; the prior tests exercised only single-bound specifiers.

Co-authored-by: Isaac
@rugpanov rugpanov force-pushed the dbconnect/02-target branch from 2092138 to 22d1137 Compare July 3, 2026 15:26
@rugpanov rugpanov force-pushed the dbconnect/03-constraints branch from 64d8996 to 3fd0bfe Compare July 3, 2026 15:26
@rugpanov rugpanov temporarily deployed to test-trigger-is July 3, 2026 15:27 — with GitHub Actions Inactive
@rugpanov rugpanov temporarily deployed to test-trigger-is July 3, 2026 15:27 — with GitHub Actions Inactive
@rugpanov rugpanov force-pushed the dbconnect/03-constraints branch from 3fd0bfe to ce01647 Compare July 3, 2026 15:31
@rugpanov rugpanov temporarily deployed to test-trigger-is July 3, 2026 15:31 — with GitHub Actions Inactive
@rugpanov rugpanov temporarily deployed to test-trigger-is July 3, 2026 15:31 — with GitHub Actions Inactive
@rugpanov rugpanov temporarily deployed to test-trigger-is July 3, 2026 15:42 — with GitHub Actions Inactive
@rugpanov rugpanov temporarily deployed to test-trigger-is July 3, 2026 15:42 — with GitHub Actions Inactive
…dden version

Round-2 review of the foundation layer noted that when a requires-python has
no lower-bound/pin clause at all (only upper-bound or exclusion, e.g.
"<3.13,!=3.12"), PythonMinorFromRequires fell back to the first number and
returned 3.13 — a version the specifier forbids. Such a spec has no floor to
install from, so it now errors rather than guessing. A bare "3.12" (no
operator) is still accepted as a valid floor.

Co-authored-by: Isaac
@rugpanov rugpanov force-pushed the dbconnect/02-target branch from 8d309ae to d9b9c50 Compare July 3, 2026 18:15
@rugpanov rugpanov force-pushed the dbconnect/03-constraints branch from 797c4ac to 03b4a2b Compare July 3, 2026 18:15
@rugpanov rugpanov temporarily deployed to test-trigger-is July 3, 2026 18:15 — with GitHub Actions Inactive
@rugpanov rugpanov temporarily deployed to test-trigger-is July 3, 2026 18:15 — with GitHub Actions Inactive
…ower bound

Round-3 review found two edge cases in the regex-based PythonMinorFromRequires:
with multiple lower bounds (">=3.8,>=3.11") it returned the first (3.8) rather
than the effective floor (3.11), and a bare floor alongside an exclusion
("!=3.11,3.12") was wrongly rejected as having no floor.

Replaced the layered regexes with a small clause parser: split on commas,
classify each clause by operator (>=,>,==,~=,=== and bare = floor; <,<=,!=
never a floor), and return the highest floor. A spec with no floor clause
("<3.13", "!=3.12") still errors. Covers multi-lower-bound, bare-floor +
exclusion, ordering, and whitespace.

Co-authored-by: Isaac
@rugpanov rugpanov force-pushed the dbconnect/02-target branch from d9b9c50 to 77ac2c8 Compare July 3, 2026 18:27
@rugpanov rugpanov force-pushed the dbconnect/03-constraints branch from 03b4a2b to 7eaf270 Compare July 3, 2026 18:27

@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.

The failure classification here is well thought through — parse-before-cache to avoid poisoning, 404 → E_ENV_UNSUPPORTED (no cache fallback) vs transport → E_FETCH (with fallback), atomic cache writes, and PEP 503 name matching. Nice. Two things to address before this lands: one hosting/ownership blocker and one on the HTTP fetch hardening. Details inline.

Comment thread libs/localenv/constraints.go Outdated
Comment on lines +95 to +96
// Constraint files are hosted at:
// https://github.com/rugpanov/databricks-environments

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.

Blocking — ownership / supply chain. These constraint artifacts decide what gets installed into a user's local Python environment (the Python version, the databricks-connect pin, and the constraint deps). Sourcing them from a personal GitHub repo means whoever controls that repo controls what the CLI installs on every user's machine — that's a supply-chain and bus-factor risk, not just a placeholder concern.

baseURL is a parameter here, so I realize the production default is wired later in the stack (#5832). But I'd like to settle it before anything ships to users: where will the production baseURL point? It needs to be a Databricks-owned, access-controlled, stable location. Please confirm the plan here (and update this comment to point at the real host) — at the latest this must be resolved before the unveil PR (#5835).

@rugpanov rugpanov Jul 6, 2026

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.

Proposing we proceed with this PR and address the host move as a follow-up. The switch to a Databricks-owned constraint host is blocked on that corporate repo (GitHub Actions are currently disabled there, so it can't build/publish the artifacts yet). In the meantime the base URL stays overridable (flag / DATABRICKS_LOCALENV_CONSTRAINT_SOURCE) and the default is a placeholder. The user-facing risk is contained regardless: the command is hidden until the unveil PR (#5835), so nothing ships to users until then — and we can gate that unveil on the owned host being ready. OK to merge this layer with the follow-up tracked?

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.

Thanks — the code-level items on this PR (#3: timeout + size cap) are resolved, and I agree the user-facing risk is contained since the command stays hidden until #5835. The one thing I'm not comfortable unilaterally signing off on is landing a default that points at a personal GitHub repo, even as an overridable placeholder.

@pietern — could you weigh in as a CLI maintainer? Summary: this layer fetches per-environment constraint artifacts (Python version, databricks-connect pin, constraint deps) that determine what gets installed into a user's local env. The production host is meant to be a Databricks-owned repo, but that repo can't publish yet (GH Actions disabled there), so the current default is a placeholder personal repo (github.com/rugpanov/databricks-environments); the base URL is overridable via --flag / DATABRICKS_LOCALENV_CONSTRAINT_SOURCE. Grigory proposes merging this unwired layer now and gating the unveil PR (#5835) on the owned host being ready.

Is that acceptable to you — merge now with the host move tracked and gated on #5835 — or would you rather hold this PR until the default points at a Databricks-owned location? Happy to approve as soon as you're OK with the plan.

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.

Implemented per @pietern's guidance (parameterize the host, use a temporary env var until we can hardcode databricks/environments):

  • The personal repo is gone from the code — the built-in default is now empty (defaultConstraintRepo = ""), and the doc comment no longer points at any personal repo.
  • The host is supplied via a temporary env var, DATABRICKS_LOCALENV_CONSTRAINT_REPO (owner/name), which RepoConstraintBaseURL turns into a raw.githubusercontent.com/<repo>/main URL. --constraint-source / DATABRICKS_LOCALENV_CONSTRAINT_SOURCE still work as a full-URL override, mainly for tests.
  • When nothing is configured, RepoConstraintBaseURL returns "" and FetchConstraints reports it as a fetch-phase error (E_FETCH, "set DATABRICKS_LOCALENV_CONSTRAINT_REPO...") — so there is no untrusted default deciding what gets installed, and the failure still flows through the normal phase/JSON reporting.
  • Once databricks/environments can publish (its GitHub Actions are enabled), defaultConstraintRepo becomes that constant and the env var is no longer required. The command stays hidden until the unveil PR ([VPEX][8/8] Unveil local-env: make the command visible, add help + changelog #5835) regardless, so this is tracked as the follow-up gating that unveil.

Added TestRepoConstraintBaseURL and TestFetchConstraintsNoSourceConfigured. (commit 4f83973)

Comment thread libs/localenv/constraints.go Outdated
Comment on lines +159 to +170
resp, err := http.DefaultClient.Do(req)
if err != nil {
return nil, fmt.Errorf("GET %s: %w", url, err)
}
defer resp.Body.Close()
if resp.StatusCode == http.StatusNotFound {
return nil, fmt.Errorf("GET %s: %w", url, errEnvKeyNotFound)
}
if resp.StatusCode < 200 || resp.StatusCode >= 300 {
return nil, fmt.Errorf("GET %s: unexpected status %s", url, resp.Status)
}
data, err := io.ReadAll(resp.Body)

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.

Two hardening asks on the fetch of untrusted network input:

  1. No timeout. http.DefaultClient has no timeout, so bounding this call depends entirely on the caller always passing a context with a deadline. It's consistent with existing repo usage (libs/versioncheck, cmd/labs/github), so a dedicated client with an explicit timeout would be cleaner — or, if you'd rather rely on the context, please guarantee in the pipeline ([VPEX][5/8] Add local-env pipeline, detection, and package-manager interface #5828) that a deadline is always set and note it here.
  2. Unbounded read. io.ReadAll(resp.Body) will read an arbitrarily large body into memory. Since this is untrusted remote content, wrap it in an io.LimitReader with a sane cap (a pyproject.toml is small) so a misbehaving/hostile host can't OOM the CLI.

Neither is a big change; #2 in particular is cheap insurance.

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.

Both done. Added a dedicated http.Client with a 30s timeout (no longer http.DefaultClient), so the fetch is bounded even without a context deadline. And wrapped the body read in io.LimitReader at 1 MiB (far above any real pyproject.toml) — an over-cap body is rejected rather than read unbounded. Added TestFetchConstraintsRejectsOversizedBody. (commit 0ef2220)

rugpanov added 9 commits July 6, 2026 13:39
…json arrays

Two items from review of the foundation layer:

- PythonMinorFromRequires bumped the minor for any strict ">" bound, but a
  patch-qualified bound like ">3.10.5" is still satisfied by 3.10.6, so the
  floor should stay 3.10 (only a bare ">3.10" excludes the whole 3.10.x
  series). clauseRe now captures the patch component and the minor is bumped
  only when it is absent. Adds >3.10.5 / >=3.10.2 test cases.

- Result.Phases and Result.Warnings are non-omitempty slices, so a bare
  Result{} would marshal them as "null" rather than "[]", an ambiguity in the
  --json contract. Added NewResult() which seeds both to empty slices, a doc
  note on the invariant, and a test asserting the JSON emits [] not null. The
  pipeline (later in the stack) constructs its Result through this.

Co-authored-by: Isaac
Second in the stacked local-env series (builds on the foundation types).

target.go resolves a compute target to a TargetInfo (and its environment
key) using ordered precedence: --cluster flag → --serverless flag → --job
flag → bundle target. Compute lookups go through the narrow ComputeClient
seam so the resolver is unit-tested against a stub with no SDK dependency.
ValidateTargetFlags guards the library path against more than one target
flag being set.

The classic-compute job branch reads the Spark version from the first
return of GetJobSparkVersion, per that method's documented contract, rather
than the recorded-version third return.

Depends on the foundation PR for NewError, the E_RESOLVE / E_NO_TARGET
codes, TargetInfo, and the EnvKeyFor* helpers. Still dormant.

Co-authored-by: Isaac
Review of the target layer noted that ResolveTarget accepted incompatible
flags on the library path: called directly with e.g.
TargetFlags{Cluster: "c", Serverless: "v4"} it silently took the first
precedence branch and ignored the rest, resolving a different target than
requested. Cobra and the cmd layer already reject this, but ResolveTarget is
exported and ValidateTargetFlags exists specifically to guard callers that
bypass Cobra, so the resolver now runs that check first and returns
E_RESOLVE on conflicting flags.

Co-authored-by: Isaac
Third in the stacked local-env series.

constraints.go fetches the per-environment pyproject.toml for a resolved
env key and parses out requires-python, the databricks-connect pin, and the
[tool.uv] constraint-dependencies. It caches each fetch on disk and
classifies failures: a 404 is E_ENV_UNSUPPORTED (a resolvable target with no
published environment, no cache fallback), while a transport or non-404 HTTP
failure is E_FETCH and falls back to the last-good cached copy.

The fetched body is validated by parseConstraints before it is written to
the cache, so a malformed 2xx response cannot poison the cache and break a
later transport-failure run that would otherwise serve the bad copy.

Depends on the foundation PR for NewError and the E_FETCH /
E_ENV_UNSUPPORTED codes. Still dormant.

Co-authored-by: Isaac
…ache, exact dep match

Review of the constraint-fetch layer surfaced several defects, all fixed here:

- parseConstraints accepted valid-but-empty TOML: a 200 body with no
  [project].requires-python returned an empty result that would be cached and
  only fail confusingly later. It now errors when requires-python is absent.

- The cache write assumed cacheDir already existed. On a fresh machine the
  first fetch succeeded but os.WriteFile failed (no such directory), so the
  cache never populated and offline runs got E_FETCH. writeCacheAtomic now
  MkdirAll's the parent.

- The cache write was non-atomic (os.WriteFile truncates in place), so a
  concurrent transport-failure reader could observe a partial file.
  writeCacheAtomic writes a temp file and renames it into place.

- databricks-connect detection used a bare string prefix, so a sibling like
  "databricks-connectors==1.0" matched first and was returned instead of the
  real pin. isDatabricksConnectDep now requires a package-name boundary.

- sanitizeEnvKey collapsed only "/", leaving a Windows "\" to be treated as a
  separator by filepath.Join; it now collapses both.

Co-authored-by: Isaac
Round-2 review noted that mapping an env key to a cache filename by replacing
"/" with "__" was not injective: distinct keys like "a/b" and "a__b" collided
on the same file, so a cached copy for one environment could be served for
another on a transport-failure fallback. cacheFileName now appends a short
sha256 of the raw env key to the readable slug, guaranteeing distinct keys get
distinct files (env keys are internally generated and never actually collide
today, but the cache should not depend on that).

Co-authored-by: Isaac
Round-3 review noted isDatabricksConnectDep was case-sensitive, but Python
package names are case-insensitive (PEP 503): a valid entry like
"Databricks-Connect==16.4.0" went undetected, leaving the pin empty. The match
now lowercases the entry before comparing; the caller still stores the original
casing.

Co-authored-by: Isaac
Round-4 review found isDatabricksConnectDep matched only case, missing PEP 503
name equivalence: "databricks_connect" and "databricks.connect" (and other
whitespace) were not recognized. The check now extracts the leading package
name up to the first PEP 508 delimiter and compares it under PEP 503
normalization (lowercase, collapse runs of -, _, . to a single -), so all
spellings match while a distinct package like databricks-connectors does not.

Co-authored-by: Isaac
Hardening the fetch of untrusted remote constraint artifacts, per review:

- Use a dedicated http.Client with a 30s timeout instead of http.DefaultClient
  (which has none), so the request is bounded even if the caller's context
  carries no deadline.
- Cap the response body with io.LimitReader (1 MiB, far above any real
  pyproject.toml) so a misbehaving or hostile host can't read an unbounded body
  into memory; an over-cap body is rejected.

Adds a test for the oversized-body rejection.

Note: the separate (blocking) review point about the production artifact host
being a personal GitHub repo is tracked on the PR and gated on the unveil PR;
it is a hosting/ownership decision, not a code change here.

Co-authored-by: Isaac
@rugpanov rugpanov force-pushed the dbconnect/02-target branch from 436dabf to 42a7988 Compare July 6, 2026 11:47
@rugpanov rugpanov force-pushed the dbconnect/03-constraints branch from 79890ca to 0ef2220 Compare July 6, 2026 11:47
@rugpanov rugpanov temporarily deployed to test-trigger-is July 6, 2026 11:47 — with GitHub Actions Inactive
@rugpanov rugpanov temporarily deployed to test-trigger-is July 6, 2026 11:47 — with GitHub Actions Inactive
@rugpanov

rugpanov commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

@anton-107 HTTP hardening done in 0ef2220bc (30s client timeout + 1 MiB io.LimitReader cap, with a test). The blocking artifact-host point is tracked and gated on the unveil PR #5835 — replied inline. Ready for another look on the code.

@anton-107 anton-107 requested a review from pietern July 6, 2026 15:04
…al-repo default

Per review, the constraint artifacts must not be sourced from a hardcoded
personal GitHub repo. This parameterizes the host: RepoConstraintBaseURL reads
the hosting repo ("owner/name") from the DATABRICKS_LOCALENV_CONSTRAINT_REPO
environment variable and builds a raw.githubusercontent.com main-branch URL.

The built-in default is intentionally empty and resolution errors when no repo
is configured, so no untrusted default controls what the CLI installs. This is
temporary: once the Databricks-owned databricks/environments repo can publish
the artifacts (its GitHub Actions are currently disabled), defaultConstraintRepo
becomes that constant and the env var is no longer required. Tracked as a
follow-up; the command stays hidden until the unveil PR regardless.

Co-authored-by: Isaac
@rugpanov rugpanov temporarily deployed to test-trigger-is July 7, 2026 08:35 — with GitHub Actions Inactive
@rugpanov rugpanov temporarily deployed to test-trigger-is July 7, 2026 08:35 — with GitHub Actions Inactive
Base automatically changed from dbconnect/02-target to main July 7, 2026 12:24

@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 blocking hosting concern is resolved in 4f83973fd, and per @pietern's guidance.

#1 (personal-repo hosting) — fixed:

  • The personal repo is gone from the code. defaultConstraintRepo is empty (""), and the doc comment no longer names any personal repo.
  • The host now comes from a temporary env var DATABRICKS_LOCALENV_CONSTRAINT_REPO (owner/name), which RepoConstraintBaseURL resolves to a raw.githubusercontent.com/<repo>/main URL; --constraint-source remains a full-URL override for tests.
  • When nothing is configured, RepoConstraintBaseURL returns "" and FetchConstraints reports it as E_FETCH with an actionable message naming the env var — so there is no untrusted default deciding what gets installed, and the failure flows through the normal phase/JSON reporting instead of aborting early. Good call routing it through the fetch phase rather than a hard abort.
  • Follow-up correctly tracked: once databricks/environments can publish, defaultConstraintRepo becomes that constant; the command stays hidden until #5835 regardless, so nothing ships to users before the owned host is ready.

#3 (fetch hardening) was already resolved (dedicated 30s-timeout client + 1 MiB io.LimitReader cap + over-cap rejection test).

Tests added for both the URL resolution and the no-source-configured path. LGTM.

One minor, non-blocking note for whoever wires the eventual default: /main is a mutable branch ref, so the fetched constraints can change under a pinned CLI version. Fine for now given the cache + hidden-command posture, but worth considering a tag/immutable ref before GA. Not blocking this PR.

@rugpanov rugpanov added this pull request to the merge queue Jul 7, 2026
Merged via the queue into main with commit f3f18e1 Jul 7, 2026
26 checks passed
@rugpanov rugpanov deleted the dbconnect/03-constraints branch July 7, 2026 13:12
@eng-dev-ecosystem-bot

Copy link
Copy Markdown
Collaborator

Integration test report

Commit: f3f18e1

Run: 28868922425

Env ❌​FAIL 🟨​KNOWN 🔄​flaky 💚​RECOVERED 🙈​SKIP ✅​pass 🙈​skip Time
🔄​ aws linux 3 3 4 527 993 38:59
🔄​ aws windows 4 4 4 482 1006 51:25
❌​ aws-ucws linux 2 1 8 5 2 978 808 156:40
❌​ aws-ucws windows 2 1 6 5 2 922 826 189:40
🔄​ azure linux 2 2 4 523 994 46:36
🔄​ azure windows 5 1 4 478 1007 52:58
❌​ azure-ucws linux 4 1 12 4 2 879 839 123:23
🔄​ azure-ucws windows 14 6 2 822 857 127:17
💚​ gcp linux 4 4 514 999 49:31
🔄​ gcp windows 3 3 4 469 1012 51:42
41 interesting tests: 28 flaky, 8 FAIL, 2 RECOVERED, 2 SKIP, 1 KNOWN
Test Name aws linux aws windows aws-ucws linux aws-ucws windows azure linux azure windows azure-ucws linux azure-ucws windows gcp linux gcp windows
🟨​ TestAccept 🔄​f 💚​R 🟨​K 🟨​K 💚​R 🔄​f 🟨​K 💚​R 💚​R 🔄​f
❌​ TestAccept/bundle/deploy/spark-jar-task ✅​p ✅​p ❌​F ✅​p ✅​p ✅​p ✅​p ✅​p ✅​p ✅​p
❌​ TestAccept/bundle/deploy/spark-jar-task/DATABRICKS_BUNDLE_ENGINE=direct ✅​p ✅​p ❌​F ✅​p ✅​p ✅​p ✅​p ✅​p ✅​p ✅​p
🔄​ TestAccept/bundle/generate/pipeline_and_deploy 🔄​f 🔄​f 🔄​f 🔄​f ✅​p ✅​p 🔄​f 🔄​f ✅​p 🔄​f
🔄​ TestAccept/bundle/generate/pipeline_and_deploy/DATABRICKS_BUNDLE_ENGINE=direct 🔄​f 🔄​f ✅​p ✅​p ✅​p ✅​p 🔄​f 🔄​f ✅​p 🔄​f
🔄​ TestAccept/bundle/generate/pipeline_and_deploy/DATABRICKS_BUNDLE_ENGINE=terraform ✅​p ✅​p 🔄​f 🔄​f ✅​p ✅​p ✅​p ✅​p ✅​p ✅​p
🔄​ TestAccept/bundle/generate/python_job_and_deploy ✅​p 🔄​f 🔄​f ✅​p ✅​p 🔄​f ✅​p 🔄​f ✅​p ✅​p
🔄​ TestAccept/bundle/generate/python_job_and_deploy/DATABRICKS_BUNDLE_ENGINE=direct ✅​p ✅​p 🔄​f ✅​p ✅​p ✅​p ✅​p ✅​p ✅​p ✅​p
🔄​ TestAccept/bundle/generate/python_job_and_deploy/DATABRICKS_BUNDLE_ENGINE=terraform ✅​p 🔄​f ✅​p ✅​p ✅​p 🔄​f ✅​p 🔄​f ✅​p ✅​p
❌​ TestAccept/bundle/invariant/continue_293 🙈​s 🙈​s ✅​p ✅​p 🙈​s 🙈​s ❌​F 🔄​f 🙈​s 🙈​s
🔄​ TestAccept/bundle/invariant/continue_293/DATABRICKS_BUNDLE_ENGINE=direct/INPUT_CONFIG=database_catalog.yml.tmpl ✅​p ✅​p 🔄​f ✅​p
❌​ TestAccept/bundle/invariant/continue_293/DATABRICKS_BUNDLE_ENGINE=direct/INPUT_CONFIG=synced_database_table.yml.tmpl ✅​p ✅​p ❌​F 🔄​f
❌​ TestAccept/bundle/invariant/migrate 🙈​s 🙈​s ✅​p ✅​p 🙈​s 🙈​s ❌​F ✅​p 🙈​s 🙈​s
❌​ TestAccept/bundle/invariant/migrate/DATABRICKS_BUNDLE_ENGINE=direct/INPUT_CONFIG=synced_database_table.yml.tmpl ✅​p ✅​p ❌​F ✅​p
🔄​ TestAccept/bundle/invariant/no_drift 🙈​S 🙈​S 💚​R 💚​R 🙈​S 🙈​S 🔄​f 💚​R 🙈​S 🙈​S
🔄​ TestAccept/bundle/invariant/no_drift/DATABRICKS_BUNDLE_ENGINE=direct/INPUT_CONFIG=synced_database_table.yml.tmpl/READPLAN= ✅​p ✅​p 🔄​f 🔄​f
🔄​ TestAccept/bundle/invariant/no_drift/DATABRICKS_BUNDLE_ENGINE=direct/INPUT_CONFIG=synced_database_table.yml.tmpl/READPLAN=1 ✅​p ✅​p 🔄​f 🔄​f
🔄​ TestAccept/bundle/resources/apps/inline_config ✅​p ✅​p ✅​p ✅​p ✅​p ✅​p ✅​p 🔄​f ✅​p ✅​p
🔄​ TestAccept/bundle/resources/apps/inline_config/DATABRICKS_BUNDLE_ENGINE=terraform ✅​p ✅​p ✅​p ✅​p ✅​p ✅​p ✅​p 🔄​f ✅​p ✅​p
❌​ TestAccept/bundle/resources/clusters/lifecycle-started ✅​p ✅​p ✅​p ❌​F ✅​p ✅​p ✅​p ✅​p ✅​p ✅​p
❌​ TestAccept/bundle/resources/clusters/lifecycle-started/DATABRICKS_BUNDLE_ENGINE=direct ✅​p ✅​p ✅​p ❌​F ✅​p ✅​p ✅​p ✅​p ✅​p ✅​p
🔄​ TestAccept/bundle/resources/dashboards/change-name ✅​p ✅​p ✅​p ✅​p ✅​p ✅​p 🔄​f 🔄​f ✅​p ✅​p
🔄​ TestAccept/bundle/resources/dashboards/change-name/DATABRICKS_BUNDLE_ENGINE=direct ✅​p ✅​p ✅​p ✅​p ✅​p ✅​p 🔄​f 🔄​f ✅​p ✅​p
🔄​ TestAccept/bundle/resources/grants/schemas/change_privilege 🙈​s 🙈​s 🔄​f ✅​p 🙈​s 🙈​s ✅​p ✅​p 🙈​s 🙈​s
🔄​ TestAccept/bundle/resources/grants/schemas/change_privilege/DATABRICKS_BUNDLE_ENGINE=direct 🔄​f ✅​p ✅​p ✅​p
🔄​ TestAccept/bundle/resources/postgres_roles/recreate 🙈​s 🙈​s ✅​p 🔄​f 🙈​s 🙈​s 🙈​s 🙈​s 🙈​s 🙈​s
🔄​ TestAccept/bundle/resources/postgres_roles/recreate/DATABRICKS_BUNDLE_ENGINE=direct ✅​p 🔄​f
🔄​ TestAccept/bundle/resources/postgres_roles/update 🙈​s 🙈​s ✅​p 🔄​f 🙈​s 🙈​s 🙈​s 🙈​s 🙈​s 🙈​s
🔄​ TestAccept/bundle/resources/postgres_roles/update/DATABRICKS_BUNDLE_ENGINE=direct ✅​p 🔄​f
🔄​ TestAccept/bundle/resources/postgres_synced_tables/recreate 🙈​s 🙈​s 🔄​f ✅​p 🙈​s 🙈​s 🙈​s 🙈​s 🙈​s 🙈​s
🔄​ TestAccept/bundle/resources/postgres_synced_tables/recreate/DATABRICKS_BUNDLE_ENGINE=direct 🔄​f ✅​p
🔄​ TestAccept/bundle/resources/quality_monitors/change_assets_dir 🙈​s 🙈​s ✅​p ✅​p 🙈​s 🙈​s 🔄​f ✅​p 🙈​s 🙈​s
🔄​ TestAccept/bundle/resources/quality_monitors/change_assets_dir/DATABRICKS_BUNDLE_ENGINE=terraform ✅​p ✅​p 🔄​f ✅​p
🔄​ TestAccept/bundle/resources/synced_database_tables/basic 🙈​s 🙈​s ✅​p ✅​p 🙈​s 🙈​s 🔄​f 🔄​f 🙈​s 🙈​s
🔄​ TestAccept/bundle/resources/synced_database_tables/basic/DATABRICKS_BUNDLE_ENGINE=terraform ✅​p ✅​p 🔄​f 🔄​f
💚​ TestAccept/bundle/resources/vector_search_endpoints/drift/recreated_same_name 🙈​S 🙈​S 💚​R 💚​R 🙈​S 🙈​S 💚​R 💚​R 🙈​S 🙈​S
🙈​ TestAccept/bundle/resources/vector_search_indexes/recreate/embedding_dimension 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S
🙈​ TestAccept/ssh/connection 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S 🙈​S
💚​ TestFetchRepositoryInfoAPI_FromRepo 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R 💚​R
🔄​ TestFetchRepositoryInfoAPI_FromRepo/root 💚​R 💚​R 💚​R 💚​R 🔄​f 🔄​f 💚​R 💚​R 💚​R 💚​R
🔄​ TestFetchRepositoryInfoAPI_FromRepo/subdir 💚​R 💚​R 💚​R 💚​R 🔄​f 🔄​f 💚​R 💚​R 💚​R 💚​R
Top 50 slowest tests (at least 2 minutes):
duration env testname
13:12 gcp linux TestAccept/bundle/resources/apps/lifecycle-started/DATABRICKS_BUNDLE_ENGINE=direct
13:07 gcp windows TestAccept/bundle/resources/clusters/lifecycle-started/DATABRICKS_BUNDLE_ENGINE=direct
12:32 aws windows TestAccept/bundle/resources/clusters/lifecycle-started/DATABRICKS_BUNDLE_ENGINE=direct
12:04 gcp linux TestAccept/bundle/resources/clusters/lifecycle-started/DATABRICKS_BUNDLE_ENGINE=direct
12:00 gcp windows TestAccept/bundle/resources/apps/lifecycle-started/DATABRICKS_BUNDLE_ENGINE=direct
11:01 aws-ucws linux TestAccept/bundle/config-remote-sync/multiple_resources/DATABRICKS_BUNDLE_ENGINE=terraform
10:30 aws windows TestAccept/bundle/resources/clusters/lifecycle-started-toggle/DATABRICKS_BUNDLE_ENGINE=direct
9:56 aws-ucws windows TestAccept/bundle/resources/apps/lifecycle-started/DATABRICKS_BUNDLE_ENGINE=direct
9:36 aws-ucws linux TestAccept/bundle/resources/clusters/lifecycle-started-toggle/DATABRICKS_BUNDLE_ENGINE=direct
8:23 aws-ucws linux TestAccept/bundle/resources/apps/lifecycle-started/DATABRICKS_BUNDLE_ENGINE=direct
8:15 aws windows TestAccept/bundle/resources/apps/lifecycle-started/DATABRICKS_BUNDLE_ENGINE=direct
8:07 gcp linux TestAccept/bundle/resources/clusters/deploy/local_ssd_count/DATABRICKS_BUNDLE_ENGINE=direct
7:53 azure linux TestAccept/bundle/resources/clusters/lifecycle-started-toggle/DATABRICKS_BUNDLE_ENGINE=direct
7:51 aws linux TestAccept/bundle/resources/apps/lifecycle-started/DATABRICKS_BUNDLE_ENGINE=direct
7:39 azure-ucws windows TestAccept/bundle/resources/apps/lifecycle-started/DATABRICKS_BUNDLE_ENGINE=direct
7:35 azure linux TestAccept/bundle/resources/apps/lifecycle-started/DATABRICKS_BUNDLE_ENGINE=direct
7:29 azure-ucws linux TestAccept/bundle/resources/apps/lifecycle-started/DATABRICKS_BUNDLE_ENGINE=direct
7:24 gcp linux TestAccept/bundle/resources/clusters/deploy/update-after-create/DATABRICKS_BUNDLE_ENGINE=terraform
7:20 azure windows TestAccept/bundle/resources/apps/lifecycle-started/DATABRICKS_BUNDLE_ENGINE=direct
7:18 gcp windows TestAccept/bundle/resources/clusters/deploy/local_ssd_count/DATABRICKS_BUNDLE_ENGINE=direct
7:10 aws-ucws linux TestAccept/bundle/resources/clusters/resize-terminated-fallback/DATABRICKS_BUNDLE_ENGINE=direct
6:45 aws windows TestAccept/bundle/resources/clusters/deploy/update-after-create/DATABRICKS_BUNDLE_ENGINE=terraform
6:43 azure-ucws windows TestAccept/bundle/resources/clusters/lifecycle-started/DATABRICKS_BUNDLE_ENGINE=direct
6:40 aws windows TestAccept/bundle/resources/clusters/resize-terminated-fallback/DATABRICKS_BUNDLE_ENGINE=direct
6:20 azure-ucws linux TestAccept/bundle/resources/clusters/lifecycle-started/DATABRICKS_BUNDLE_ENGINE=direct
6:13 azure-ucws windows TestAccept/bundle/resources/clusters/deploy/update-after-create/DATABRICKS_BUNDLE_ENGINE=direct
6:11 azure windows TestAccept/bundle/resources/clusters/deploy/update-after-create/DATABRICKS_BUNDLE_ENGINE=direct
5:59 aws linux TestAccept/bundle/resources/clusters/lifecycle-started/DATABRICKS_BUNDLE_ENGINE=direct
5:47 aws-ucws linux TestAccept/bundle/resources/clusters/lifecycle-started/DATABRICKS_BUNDLE_ENGINE=direct
5:40 azure-ucws linux TestAccept/bundle/config-remote-sync/multiple_resources/DATABRICKS_BUNDLE_ENGINE=terraform
5:34 gcp linux TestAccept/bundle/resources/clusters/resize-terminated-fallback/DATABRICKS_BUNDLE_ENGINE=direct
5:32 aws windows TestAccept/bundle/deploy/spark-jar-task/DATABRICKS_BUNDLE_ENGINE=terraform
5:28 azure-ucws linux TestAccept/bundle/resources/clusters/lifecycle-started-toggle/DATABRICKS_BUNDLE_ENGINE=direct
5:20 aws-ucws linux TestAccept/bundle/resources/apps/lifecycle-started-omitted/DATABRICKS_BUNDLE_ENGINE=direct
5:13 gcp windows TestAccept/bundle/resources/clusters/lifecycle-started-toggle/DATABRICKS_BUNDLE_ENGINE=direct
5:11 azure windows TestAccept/bundle/resources/clusters/lifecycle-started/DATABRICKS_BUNDLE_ENGINE=direct
5:06 azure linux TestAccept/bundle/resources/clusters/lifecycle-started/DATABRICKS_BUNDLE_ENGINE=direct
5:03 aws-ucws windows TestAccept/bundle/deploy/spark-jar-task/DATABRICKS_BUNDLE_ENGINE=terraform
5:02 gcp linux TestAccept/bundle/resources/apps/lifecycle-started-toggle/DATABRICKS_BUNDLE_ENGINE=direct
4:57 aws-ucws windows TestAccept/bundle/deploy/spark-jar-task/DATABRICKS_BUNDLE_ENGINE=direct
4:56 aws-ucws linux TestAccept/bundle/deploy/spark-jar-task/DATABRICKS_BUNDLE_ENGINE=terraform
4:55 gcp linux TestAccept/bundle/resources/apps/lifecycle-started-omitted/DATABRICKS_BUNDLE_ENGINE=direct
4:52 aws linux TestAccept/bundle/deploy/spark-jar-task/DATABRICKS_BUNDLE_ENGINE=terraform
4:49 gcp windows TestAccept/bundle/resources/apps/inline_config/DATABRICKS_BUNDLE_ENGINE=terraform
4:46 gcp linux TestAccept/bundle/resources/apps/inline_config/DATABRICKS_BUNDLE_ENGINE=direct
4:41 azure windows TestAccept/bundle/deploy/spark-jar-task/DATABRICKS_BUNDLE_ENGINE=terraform
4:39 gcp linux TestAccept/bundle/resources/clusters/lifecycle-started-toggle/DATABRICKS_BUNDLE_ENGINE=direct
4:37 gcp windows TestAccept/bundle/resources/apps/lifecycle-started-toggle/DATABRICKS_BUNDLE_ENGINE=direct
4:37 azure-ucws windows TestAccept/bundle/resources/clusters/lifecycle-started-toggle/DATABRICKS_BUNDLE_ENGINE=direct
4:22 azure-ucws linux TestAccept/bundle/resources/apps/inline_config/DATABRICKS_BUNDLE_ENGINE=terraform

ZaSkittles pushed a commit to ZaSkittles/cli that referenced this pull request Jul 7, 2026
databricks#5823)

## Why

- The `local-env` feature needs a shared vocabulary before any behavior
can be built: the result shape, the error taxonomy, and how a compute
target maps to an environment key.
- Landing these contract types first lets every later layer (resolve /
fetch / merge / pipeline / command) depend on stable, reviewed
definitions.
- Kept deliberately minimal and dependency-free so it reviews on its own
and stays `unused`/`deadcode`-clean with no consumers yet.

## What

- **`result.go`** — the `--json` / `E_*` output contract: `Result`,
`PipelineError`, `ErrorCode`, `PhaseName`, `PhaseStatus`, `Mode`,
`TargetInfo`, `ResolvedInfo`, `Plan`, `Warning`; plus the command-path
constants (`local-env` / `python` / `sync`) defined in one place.
- **`envkey.go`** — `EnvKeyForServerless` / `EnvKeyForSparkVersion` /
`NormalizeServerless`, and `PythonMinorFromRequires` (clause-aware:
returns the effective highest lower bound of a `requires-python`).
- No wiring into `cmd/`, so the CLI is unchanged. Filesystem/artifact
constants and the phase-order slice deliberately live with their
consumer (PR 5).

## Testing strategy

- Unit tests for the error/type contract (`result_test.go`) and env-key
mapping incl. multi-clause / strict-`>` / no-floor `requires-python`
cases (`envkey_test.go`).
- Gates: `go build`, `go test`, `golangci-lint`, `deadcode`, `gofmt` —
all green.
- Reviewed with codex across several rounds to convergence (all findings
fixed or explicitly rejected as speculative).

---

## 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.** Each PR targets the previous one as its base
branch, so its diff shows only that layer.

| # | PR | What |
|---|----|------|
| 1 | **databricks#5823 ← you are here** | foundation: result types + env-key
mapping |
| 2 | databricks#5824 | compute-target resolution |
| 3 | databricks#5826 | constraint fetch + offline cache |
| 4 | databricks#5827 | formatting-preserving pyproject.toml merge |
| 5 | databricks#5828 | six-phase pipeline + detection + package-manager interface
|
| 6 | databricks#5832 | uv backend + CLI command (registered hidden) |
| 7 | databricks#5833 | acceptance tests |
| 8 | databricks#5835 | unveil (unhide + help + changelog) |

This pull request and its description were written by Isaac.
ZaSkittles pushed a commit to ZaSkittles/cli that referenced this pull request Jul 7, 2026
## Why

- `local-env` must turn the user's compute selection into a single
environment key before it can fetch anything, and the selection can come
from several places with a defined precedence.
- Isolating resolution behind a narrow seam keeps it testable without a
live workspace and keeps SDK details out of the engine.

## What

- **`target.go`** — `ResolveTarget` with ordered precedence `--cluster`
→ `--serverless` → `--job` → bundle target, producing a `TargetInfo` +
env key.
- Compute lookups go through the narrow `ComputeClient` interface
(stubbable in tests).
- `ValidateTargetFlags` rejects more than one target flag;
`ResolveTarget` runs it up front so a non-Cobra caller can't silently
resolve the wrong target.
- Classic-compute jobs read the Spark version from the documented first
return of `GetJobSparkVersion` (not the recorded-version third return).

## Testing strategy

- Unit tests against a stub `ComputeClient` covering each precedence
branch, the mutually-exclusive-flags error, and the job classic-compute
contract (`target_test.go`).
- Gates: `go build`, `go test`, `golangci-lint`, `deadcode`, `gofmt` —
all green.
- Reviewed with codex to a clean pass.

---

## 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.** Each PR targets the previous one as its base
branch, so its diff shows only that layer.

| # | PR | What |
|---|----|------|
| 1 | databricks#5823 | foundation: result types + env-key mapping |
| 2 | **databricks#5824 ← you are here** | compute-target resolution |
| 3 | databricks#5826 | constraint fetch + offline cache |
| 4 | databricks#5827 | formatting-preserving pyproject.toml merge |
| 5 | databricks#5828 | six-phase pipeline + detection + package-manager interface
|
| 6 | databricks#5832 | uv backend + CLI command (registered hidden) |
| 7 | databricks#5833 | acceptance tests |
| 8 | databricks#5835 | unveil (unhide + help + changelog) |

This pull request and its description were written by Isaac.
ZaSkittles pushed a commit to ZaSkittles/cli that referenced this pull request Jul 7, 2026
…atabricks#5827)

## Why

- `local-env` must apply the resolved Python version and constraints to
the user's `pyproject.toml` without disturbing their own content —
comments, ordering, formatting, and unrelated config must survive
untouched.
- Re-running must be safe and idempotent, and a greenfield project needs
a sensible file created from scratch.
- This is the most intricate logic in the feature, so it lands in its
own PR for focused review.

## What

- **`merge.go`** — a formatting-preserving merge that rewrites only the
env-owned regions (`requires-python`, the `databricks-connect` entry in
`[dependency-groups].dev`, and a marker-bracketed managed `[tool.uv]`
block) and preserves every other byte incl. CRLF; idempotent.
`RenderFreshPyproject` builds a complete managed file for a greenfield
project.
- Scoping/robustness: managed `constraint-dependencies` nests
header-less inside an existing user `[tool.uv]` (never a duplicate
header); single- vs multi-line array detection tracks real bracket depth
outside strings/comments; the `databricks-connect` rewrite is confined
to `dev` and leaves trailing comments alone; `requires-python`'s inline
comment is preserved; table-header parsing tolerates inline comments and
recognizes `[[array.of.tables]]`.

## Testing strategy

- Unit tests that parse the merged output as TOML (not just substring
checks), covering idempotency, CRLF preservation, user-key preservation,
the duplicate-`[tool.uv]` case, bracket-in-element arrays,
sibling-group/comment non-clobbering, and `[[tool.uv.index]]` children
(`merge_test.go`).
- Gates: `go build`, `go test`, `golangci-lint`, `deadcode`, `gofmt` —
all green.
- Reviewed with codex to a clean pass (multiple TOML-corruption edge
cases were caught and fixed).

---

## 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.** Each PR targets the previous one as its base
branch, so its diff shows only that layer.

| # | PR | What |
|---|----|------|
| 1 | databricks#5823 | foundation: result types + env-key mapping |
| 2 | databricks#5824 | compute-target resolution |
| 3 | databricks#5826 | constraint fetch + offline cache |
| 4 | **databricks#5827 ← you are here** | formatting-preserving pyproject.toml
merge |
| 5 | databricks#5828 | six-phase pipeline + detection + package-manager interface
|
| 6 | databricks#5832 | uv backend + CLI command (registered hidden) |
| 7 | databricks#5833 | acceptance tests |
| 8 | databricks#5835 | unveil (unhide + help + changelog) |

This pull request and its description were written by Isaac.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants