Add orthogonal setup-local flags: --no-constraints and --no-dbconnect - #6464
Open
rugpanov wants to merge 6 commits into
Open
Add orthogonal setup-local flags: --no-constraints and --no-dbconnect#6464rugpanov wants to merge 6 commits into
setup-local flags: --no-constraints and --no-dbconnect#6464rugpanov wants to merge 6 commits into
Conversation
rugpanov
force-pushed
the
setup-local/orthogonal-flags
branch
from
September 1, 2026 15:20
70c6da8 to
d1aad7e
Compare
rugpanov
marked this pull request as ready for review
September 1, 2026 15:45
This was referenced Sep 1, 2026
rclarey
approved these changes
Sep 3, 2026
rclarey
left a comment
rclarey
left a comment
Contributor
There was a problem hiding this comment.
Mostly LGTM, but some of the flag combinations have non-obvious consequences to me
--constraints-only+--no-constraintsis nonsense, but I guess we don't want to remove--constraints-onlyfor backwards compatibility?- is
--no-constraints+--no-dbconnect+--no-provisionmeaningful? It seems this does some checks / fetches the constraints artifact but does nothing with it? Is there a case where someone would want all 3 together?
| } | ||
|
|
||
| // Normalize a missing [tool.uv].constraint-dependencies to a non-nil empty | ||
| // slice. A nil ConstraintDeps is reserved as the --no-constraints "leave the |
Contributor
There was a problem hiding this comment.
Why not thread the --no-constraints flag through so we don't need a special signal value? (same is true for other signal values used elsewhere)
*Why* `databricks environments setup-local` had a single `--constraints-only` mode that bundled "skip databricks-connect" with the rest of the setup. Callers need to control the setup axes independently — skip the remote pins, or skip the databricks-connect dependency — so the flags that control them should be orthogonal. *What* Adds two orthogonal, composable negative flags: - `--no-constraints` skips writing the remote Python-version and dependency pins (requires-python and the [tool.uv] constraint block); any existing values are left untouched, and provisioning still installs the resolved Python (the flag governs only what is written). - `--no-dbconnect` skips the databricks-connect dependency. Equivalent to the existing `--constraints-only`, which stays as-is for now. The `--no-constraints` "unmanaged" signal is a nil ConstraintDeps / empty requires-python; parseConstraints normalizes a missing [tool.uv].constraint-dependencies to a non-nil empty slice so that nil uniquely means the flag, not merely an artifact that omits the section. Default runs (no new flags) are byte-for-byte unchanged. *Verification* - Unit tests (libs/localenv): --no-constraints leaves existing pins untouched and omits them greenfield, and parseConstraints normalizes missing constraint-dependencies. - Acceptance goldens: no-constraints and no-dbconnect, plus the refreshed help output. - gofmt, go vet, and full go build ./... clean. Co-authored-by: Isaac <no-reply@databricks.com>
rugpanov
force-pushed
the
setup-local/orthogonal-flags
branch
from
September 3, 2026 15:24
7d3ec5f to
f9ffddd
Compare
rugpanov
changed the title
setup-local flags (--no-constraints, --no-dbconnect, --no-provision) and a skipped phase statussetup-local flags: --no-constraints and --no-dbconnect
*Why* Review feedback: the --no-constraints "leave the constraint block unmanaged" intent was encoded by clearing effective.RequiresPython to "" and effective.ConstraintDeps to nil, then having the merge, fresh render, and warning detector infer "skip" from those shapes. That overloaded the values (nil ConstraintDeps meant the flag, an empty slice meant "write an empty block"), and only a parseConstraints normalization step kept the two apart. Overloading a data value to carry a control signal is fragile — anyone constructing a Constraints by hand, or refactoring the parse path, silently re-collides the two and stops managing constraints with no test to catch it. *What* Thread an explicit skipConstraints bool from the pipeline into MergeManaged, RenderFreshPyproject, and detectMergeWarnings; gate the requires-python and [tool.uv] regions (and their two warnings) on it. The pipeline no longer clears effective's values, so those functions always see the real artifact values and the intent is unambiguous. Consequences of removing the sentinel: - parseConstraints no longer normalizes a missing constraint-dependencies to a non-nil empty slice; nil and empty now flow through identically. - mergeToolUv drops its nil-guard so nil is treated like empty (both render an empty managed block); mergeRequiresPython drops its empty-value guard. Behaviour is unchanged: default and --no-constraints output are byte-identical (every acceptance golden passes untouched). This is an internal cleanup that deletes the sentinel and its normalization. *Verification* - go build ./... and go vet ./libs/localenv/... ./cmd/environments/... clean. - go test ./libs/localenv/... ./cmd/environments/... pass. - go test ./acceptance -run TestAccept/localenv passes with goldens unchanged. - New unit tests: MergeManaged/RenderFreshPyproject skip the regions on skipConstraints=true even when the Constraints carry values; a nil ConstraintDeps writes an empty managed block when not skipping; detectMergeWarnings suppresses the constraint warnings under skip. Verified each fails without the gate before it was added. Co-authored-by: Isaac <no-reply@databricks.com>
*Why* The rebase onto latest main picked up a stricter `validate_nextchanges.py`, which now requires each fragment to be a single `* `-bullet line ending in a period with a trailing PR link. The existing prose fragment failed that check, breaking the `lint` and `preview` CI jobs. *What* Rewrite `.nextchanges/cli/setup-local-orthogonal-flags.md` as one bullet line with the `([#6464](...))` trailing PR link. No content change to the described behaviour. *Verification* - PR_NUMBER=6464 python3 tools/validate_nextchanges.py exits 0. Co-authored-by: Isaac <no-reply@databricks.com>
*Why* Review feedback: `--no-constraints` was threaded into the pipeline but never surfaced in `SetupLocalEvent`. Because the flag leaves `Mode` at `default` (Mode only records the databricks-connect axis), a `--no-constraints` run was indistinguishable from a plain run in telemetry, so its adoption could not be measured. *What* Add a `skip_constraints` bool to `SetupLocalEvent` (not omitempty, so a genuine false is distinguishable from an older CLI that did not report it, matching the existing `success`/`greenfield` fields). Carry it on `Result` as an internal-only `json:"-"` field — like `PythonInterpreter` — so the telemetry event stays purely Result-derived without expanding the `--output json` contract. The pipeline sets it from `p.SkipConstraints`. Note: `SetupLocalEvent` mirrors the server-side lumberjack proto; the matching `skip_constraints` field must be added there for the value to be captured. Until then the server ignores the unknown field (no harm), same as any CLI-first telemetry addition. *Verification* - go build ./..., go vet, and gofmt clean. - go test ./cmd/environments/... ./libs/localenv/... pass; new unit test asserts a --no-constraints run records skip_constraints=true and a plain run leaves it false (verified it fails without the mapping). - go test ./acceptance -run TestAccept/localenv passes with goldens unchanged (the field is json:"-", so the JSON contract is untouched). Co-authored-by: Isaac <no-reply@databricks.com>
Collaborator
Integration test reportCommit: 03d88cf
Top 6 slowest tests (at least 2 minutes):
|
rclarey
approved these changes
Sep 4, 2026
…ect explicit
*Why*
The previous change threaded --no-constraints as a positional bool through
MergeManaged/RenderFreshPyproject/detectMergeWarnings. A positional bool per axis
does not scale: every new axis re-churns all ~75 call sites and yields
boolean-blind calls like MergeManaged(x, c, true, false). The remaining axis —
databricks-connect — was still driven by the empty-dbcPin signal value (exactly
the "other signal values elsewhere" the reviewer flagged), so it had the same
overloading problem --no-constraints just shed.
*What*
Introduce MergeOptions{SkipConstraints, SkipDBConnect bool}, passed to
MergeManaged, RenderFreshPyproject, and detectMergeWarnings. The zero value manages
every axis, so a future axis is a new field with no call-site churn, and each call
reads self-documentingly.
Thread SkipDBConnect explicitly everywhere the empty-dbcPin signal used to reach:
the merge gates the databricks-connect region on it, the fresh render emits an
empty dev group under it, the warning detector suppresses the databricks-connect /
standalone-pyspark warnings, planDBConnect returns the zero plan, and validate
skips the databricks-connect assertion and version reporting. The pipeline no
longer clears the pin — c carries the real artifact value throughout, and intent
is a flag, not an empty string. An empty databricks-connect value now means only
"the artifact has no pin" (a data no-op), never "skip".
Behaviour is unchanged: default, --no-constraints, and --no-dbconnect /
--constraints-only output are byte-identical (every acceptance golden passes
untouched).
*Verification*
- go build ./..., go vet, and gofmt clean.
- go test ./libs/localenv/... ./cmd/environments/... pass; new unit tests cover
SkipDBConnect in the merge, fresh render, and warning detector (verified each
fails without its gate before restoring).
- go test ./acceptance -run TestAccept/localenv passes with goldens unchanged.
Co-authored-by: Isaac <no-reply@databricks.com>
…h consistently *Why* Review follow-ups on the MergeOptions change: - MergeManaged rejected a pyproject.toml without a [project] table even under --no-constraints, though [project] is required only to hold requires-python — which that mode does not write. A valid uv file with only dependency groups / tool config failed with E_MERGE for a reason that no longer applied. Confirmed uv itself syncs a [project]-less file, so relaxing the guard is safe end to end. - planDBConnect and validate still took a bare skip bool, reintroducing the boolean-blind call sites MergeOptions was meant to remove. - Several comments restated flag semantics already documented on MergeOptions. *What* - Gate the [project] guard on !opts.SkipConstraints, so a [project]-less file is merged for its other axes under --no-constraints instead of rejected. - Pass MergeOptions through planDBConnect and validate instead of a bare bool. - Trim comments that duplicated the MergeOptions field docs. Behaviour is unchanged for every existing case (all have [project]); the only new behaviour is that --no-constraints now succeeds on a [project]-less file. *Verification* - go build ./..., go vet, gofmt clean. - go test ./libs/localenv/... ./cmd/environments/... pass; new unit test covers a [project]-less file merging under SkipConstraints (verified it fails with the guard ungated before restoring). - go test ./acceptance -run TestAccept/localenv passes with goldens unchanged. Co-authored-by: Isaac <no-reply@databricks.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
databricks environments setup-localhad a single--constraints-onlymode thatbundled "skip databricks-connect" together with the rest of the setup. Callers
need to control the setup axes independently — skip the remote pins, or skip the
databricks-connect dependency — so the flags that control them should be
orthogonal.
What
Two orthogonal, composable negative flags:
--no-constraints— skip writing the remote Python-version and dependencypins (
requires-pythonand the[tool.uv]constraint block). Existing valuesare left untouched, and (when provisioning) the resolved Python is still
installed — the flag only governs what is written.
--no-dbconnect— skip the databricks-connect dependency. Equivalent tothe existing
--constraints-only, which is left in place for now (either flagtriggers the same behaviour).
Both axes are threaded explicitly through the merge as a
MergeOptionsstruct (
SkipConstraints/SkipDBConnect) rather than inferred from the shape ofa value: the merge, fresh-project render, warning detector, and validate step all
gate on the flags, so they always see the real artifact values. This removes the
former signal values — a
nilConstraintDepsand an empty databricks-connect pinare now plain data (treated identically to their non-empty forms), not a stand-in
for "leave this unmanaged". The zero-value
MergeOptionsmanages every axis, so afuture axis is a new struct field rather than another positional parameter.
The setup-local telemetry event records a
skip_constraintsfield so a--no-constraintsrun is distinguishable from a plain one (modeonly recordsthe databricks-connect axis). The matching field must be added to the server-side
lumberjack proto for the value to be captured.
Note on
--no-constraints+ provisioning--no-constraintsgoverns only what is written. A provisioning run still installsand validates the resolved Python, so if a user's kept
requires-pythonisdisjoint from the target, uv surfaces it as a normal
E_PROVISIONrather than thecommand guessing an alternative.
Backward compatibility
Default runs (no new flags) produce byte-for-byte identical output and behaviour
— every existing acceptance golden is unchanged except the refreshed
--helplisting.
Testing
libs/localenv): each axis leaves existing values untouched and omitsthem greenfield; the merge, fresh render, and warning detector skip the
constraint / databricks-connect regions on the corresponding
MergeOptionsflageven when the
Constraintscarry values; anilConstraintDepswrites anempty managed block when constraints are not skipped.
cmd/environments): the telemetry event recordsskip_constraints.acceptance/localenv):no-constraintsandno-dbconnectgoldens, plus the refreshed
helpoutput.gofmt,go vet, and fullgo build ./...clean; rebased on latestmain.This pull request and its description were written by Isaac.