[VPEX][10] Add --cluster-name target flag - #5961
Merged
Merged
Conversation
This was referenced Jul 17, 2026
rugpanov
changed the title
--cluster-name target flag--cluster-name target flag
Collaborator
Integration test reportCommit: 8e83519
8 interesting tests: 4 RECOVERED, 4 SKIP
Top 10 slowest tests (at least 2 minutes):
|
rugpanov
added a commit
that referenced
this pull request
Jul 17, 2026
…--cluster-name in no-target guidance Review of #5961 (isaac major + codex P3): - GetClusterByName no longer uses the SDK's GetByClusterName, which lists clusters in every state and errors on any name collision — a terminated cluster sharing a name with a live one would spuriously block resolution. List only non-terminated clusters (ListClustersFilterBy) and match by name; a name still ambiguous among active clusters, or matching none, is a genuine E_RESOLVE. - noTargetMessage now includes --cluster-name so the no-target guidance lists every valid target flag. - Acceptance: cluster-name-ambiguous now models two RUNNING clusters (genuine ambiguity) with the clearer error; no-target and json-error goldens updated. Co-authored-by: Isaac
anton-107
approved these changes
Jul 17, 2026
anton-107
left a comment
anton-107
left a comment
Contributor
There was a problem hiding this comment.
Reviewed as part of the full stack (#5960–#5965). The --cluster-name path resolves correctly (name→ID→env key, source=cluster), and it joins both the Cobra exclusion group and the library-path ValidateTargetFlags/bundle-fallback guards. Approving.
Findings to consider (none blocking):
-
[low] Error leaks an internal Go type name to end users. The ambiguous/unknown-name errors surface the raw SDK message verbatim, e.g.
there are 2 instances of ClusterDetails named 'dup'(see goldenacceptance/localenv/cluster-name-ambiguous/output.txt:2).ClusterDetailsis an SDK struct name; a user-facing CLI shouldn't expose it. Consider rewrapping, e.g. "2 clusters are named 'dup'; pass --cluster-id to disambiguate". -
[low]
GetByClusterNameloads all clusters and misses long-terminated ones. The SDK'sClusters.GetByClusterNamecallsListAll(paginates every cluster into memory), and the backing List API omits clusters terminated >30 days — so a valid-but-long-terminated cluster name yields "does not exist". This is inherent to name resolution (there's no get-by-name endpoint; names aren't unique), so impact is low, but a one-line comment onGetClusterByNamenoting the >30-day caveat would help the next reader. -
[low] Test coverage: only the ambiguous path is exercised.
TestResolveClusterNameFlagErrorand the acceptance dir cover the ambiguous error; the unknown-name branch (ClusterDetails named 'X' does not exist) has no unit or acceptance case, though the description says both failure modes are covered. Worth one more case since the two share a single wrapping branch. -
[nit] Stale precedence comment.
ResolveTarget's doc comment (libs/localenv/target.go:64) still lists precedence as--cluster-id → --serverless-version → --job-id → bundle, omitting the--cluster-namebranch this PR inserts between cluster-id and serverless. Harmless to correctness (flags are mutually exclusive) but misleads a reader. (#5964 updated the siblingTargetInfodoc for "five flag sources" but left this one.)
Reviewed with AI assistance (build + unit tests + adversarial verification against the checked-out top of stack).
rugpanov
force-pushed
the
dbconnect/10-flag-renames
branch
2 times, most recently
from
July 20, 2026 08:56
a84ab00 to
97a0ec4
Compare
rugpanov
added a commit
that referenced
this pull request
Jul 20, 2026
…--cluster-name in no-target guidance Review of #5961 (isaac major + codex P3): - GetClusterByName no longer uses the SDK's GetByClusterName, which lists clusters in every state and errors on any name collision — a terminated cluster sharing a name with a live one would spuriously block resolution. List only non-terminated clusters (ListClustersFilterBy) and match by name; a name still ambiguous among active clusters, or matching none, is a genuine E_RESOLVE. - noTargetMessage now includes --cluster-name so the no-target guidance lists every valid target flag. - Acceptance: cluster-name-ambiguous now models two RUNNING clusters (genuine ambiguity) with the clearer error; no-target and json-error goldens updated. Follow-up review (anton-107, #5961): - Note on GetClusterByName that clusters/list omits clusters terminated >30 days ago, so such a name resolves as "no active cluster named". - Fix the ResolveTarget precedence doc comment to include the --cluster-name branch this PR inserts. - Add cluster-name-unknown acceptance case covering the unknown-name branch (0 matches), the counterpart to cluster-name-ambiguous. Co-authored-by: Isaac
rugpanov
force-pushed
the
dbconnect/11-cluster-name
branch
from
July 20, 2026 09:17
08c5535 to
e5e16ab
Compare
Contributor
Author
|
Thanks for the detailed pass. Addressed the follow-up findings:
Also cascade-rebased onto the updated PR 9 branch. |
Per the [P0] CLI Changes spec, add --cluster-name as a compute target: it resolves the cluster name to an ID via the Clusters API and then behaves identically to --cluster-id (source=cluster, env key from the resolved cluster's spark_version). - ComputeClient gains GetClusterByName; sdkCompute implements it via the SDK's Clusters.GetByClusterName, which errors on an unknown or ambiguous name (two clusters sharing it) — surfaced as an actionable E_RESOLVE. - --cluster-name joins the mutually-exclusive target group and the bundle-fallback guard. - Unit tests for success, ambiguity, and cluster-id/-name exclusivity; acceptance tests cluster-name-check (happy path) and cluster-name-ambiguous (E_RESOLVE), plus the help golden. Co-authored-by: Isaac
…--cluster-name in no-target guidance Review of #5961 (isaac major + codex P3): - GetClusterByName no longer uses the SDK's GetByClusterName, which lists clusters in every state and errors on any name collision — a terminated cluster sharing a name with a live one would spuriously block resolution. List only non-terminated clusters (ListClustersFilterBy) and match by name; a name still ambiguous among active clusters, or matching none, is a genuine E_RESOLVE. - noTargetMessage now includes --cluster-name so the no-target guidance lists every valid target flag. - Acceptance: cluster-name-ambiguous now models two RUNNING clusters (genuine ambiguity) with the clearer error; no-target and json-error goldens updated. Follow-up review (anton-107, #5961): - Note on GetClusterByName that clusters/list omits clusters terminated >30 days ago, so such a name resolves as "no active cluster named". - Fix the ResolveTarget precedence doc comment to include the --cluster-name branch this PR inserts. - Add cluster-name-unknown acceptance case covering the unknown-name branch (0 matches), the counterpart to cluster-name-ambiguous. Co-authored-by: Isaac
rugpanov
force-pushed
the
dbconnect/11-cluster-name
branch
from
July 20, 2026 09:38
e5e16ab to
8e83519
Compare
pietern
pushed a commit
that referenced
this pull request
Jul 20, 2026
Stacked on #5960 (flag renames), which is stacked on #5959 (command rename). ## What Adds `--cluster-name` as a compute target for `environments setup-local`, per the `[P0] CLI Changes` spec. It resolves the cluster **name** to an **ID** via the Clusters API, then resolves identically to `--cluster-id` (`source=cluster`, env key from the resolved cluster's `spark_version`). ## Changes - `ComputeClient` gains `GetClusterByName`; `sdkCompute` implements it via the SDK's `Clusters.GetByClusterName`, which errors on an **unknown** or **ambiguous** name (two clusters sharing it) — both surfaced as an actionable `E_RESOLVE`. - `--cluster-name` joins the mutually-exclusive target group (`--cluster-id`/`--cluster-name`/`--serverless-version`/`--job-id`) and the bundle-fallback guard. - New `ResolveTarget` precedence branch after `--cluster-id`. ## Testing - Unit tests: success (name→ID→env key), ambiguity → `E_RESOLVE`, and `--cluster-id`/`--cluster-name` mutual exclusivity. - Acceptance: `cluster-name-check` (happy path, stubbed `clusters/list`) and `cluster-name-ambiguous` (`E_RESOLVE`); help golden updated. - `go build ./...`, lint (0 issues), deadcode clean, unit + acceptance green. This pull request and its description were written by Isaac.
Collaborator
Integration test reportCommit: 56845c0
25 interesting tests: 11 flaky, 6 FAIL, 5 RECOVERED, 2 SKIP, 1 KNOWN
Top 50 slowest tests (at least 2 minutes):
|
yansonggao-db
pushed a commit
to yansonggao-db/cli
that referenced
this pull request
Jul 21, 2026
…icks#5965) Stacked on databricks#5964 → databricks#5963 → databricks#5962 → databricks#5961 → databricks#5960 → databricks#5959. ## What `--serverless-version` is now documented to take a **bare number** (e.g. `5`) rather than `v5`. - Input `5` is the documented form; `v5`/`V5` are still accepted (tolerant). - Both map to the same env key `serverless/serverless-vN`, so the **environments-repo layout is unchanged** (still `serverless/serverless-v5/…`). ## Changes - Flag help: `e.g. v4` → `e.g. 5`; the `E_ENV_UNSUPPORTED` hint suggests `--serverless-version 5`. - `defaultServerlessVersion` stored in bare form (`"5"`); still resolves to `serverless-v5` via `NormalizeServerless`. - Acceptance scripts pass the bare form; env-key goldens unchanged (`serverless-vN`). - Unit tests cover bare + v-prefixed input and the default constant. ## Testing `go build ./...`, lint (0 issues), deadcode clean, unit + acceptance green. This pull request and its description were written by Isaac.
tanishgupta-db
pushed a commit
to tanishgupta-db/cli
that referenced
this pull request
Jul 21, 2026
Stacked on databricks#5961 (`--cluster-name`) → databricks#5960 → databricks#5959. ## What Per the `[P0] CLI Changes` spec, the serverless stand-in used when the source does not pin a version is now **`serverless-v5`** (was `v4`). Applies only to the fallback cases: - a serverless `--job-id` whose environment records no version, and - a bundle that records `serverless` without a version. Explicitly passing `--serverless-version <vN>` is unaffected. ## Changes - Introduce `defaultServerlessVersion = "v5"` in `envkey.go`, used at both fallback sites in `target.go` so the default lives in one place. - Update the two unit tests that assert the default. No acceptance goldens change: all serverless acceptance tests pass `--serverless-version v4` explicitly, so none exercise the default path. ## Note VS Code resolves the real serverless version itself and passes `--serverless-version` explicitly (spec §63), so this fallback is only hit when the version is genuinely unknown — but v5 is now the correct stand-in per spec. This pull request and its description were written by Isaac.
tanishgupta-db
pushed a commit
to tanishgupta-db/cli
that referenced
this pull request
Jul 21, 2026
Stacked on databricks#5962 (serverless-v5) → databricks#5961 → databricks#5960 → databricks#5959. ## What Per the `[P0] CLI Changes` spec, a bad-flags usage error is now surfaced as **`E_USAGE`** at the **preflight** phase, through the phase/JSON contract — instead of a bare Cobra mutual-exclusion error printed before `RunE` (which produces no command JSON object). ## Why The VS Code extension branches on the JSON `error.code`. Previously, passing two target flags triggered Cobra's `MarkFlagsMutuallyExclusive` *before* `RunE`, so `--output json` emitted nothing structured. Now the conflict is caught inside the pipeline and reported like any other phase failure. ## Changes - Add the `E_USAGE` error code. - Validate target flags at the top of the pipeline's preflight → `E_USAGE`, `diskMutated=false`. - Drop `cmd.MarkFlagsMutuallyExclusive` and the redundant early-return in `runPipeline` so the conflict flows into the pipeline. - `flag-conflict` golden updated (now shows the phase table + preflight error); new `flag-conflict-json` asserts `error{code:"E_USAGE", failurePhase:"preflight"}`; pipeline unit test for the path. ## Testing `go build ./...`, lint (0 issues), deadcode clean, unit + acceptance green. This pull request and its description were written by Isaac.
tanishgupta-db
pushed a commit
to tanishgupta-db/cli
that referenced
this pull request
Jul 21, 2026
…ricks#5964) Stacked on databricks#5963 (E_USAGE) → databricks#5962 → databricks#5961 → databricks#5960 → databricks#5959. ## What Reconciles the error codes with the `[P0] CLI Changes` spec table (spec item databricks#10). **No behavior change** — the code already emits the correct set. The spec's table was stale: - omitted `E_NOT_WRITABLE` (preflight) and `E_PYTHON_INSTALL` (provision), and - wrongly listed `E_UV_MISSING` as "reserved / not emitted" when the CLI does emit it at preflight. ## Changes - Annotate each `ErrorCode` constant with the phase that emits it, and document the two spec codes the CLI deliberately never emits: `E_PYTHON_POLICY` (no signal source yet) and `E_AUTH` (handled earlier by `MustWorkspaceClient`). This makes the constant block the authoritative reference the spec mirrors. - Fix a `TargetInfo` doc comment that still said "four precedence sources" — with `--cluster-name` there are now five flag sources (the JSON `source` value is still one of cluster/serverless/job/bundle). ## Note The spec doc's error-code table itself (in the VPEX ERD, a Google Doc) has been updated separately to match — that's outside this repo. ## Testing `go build ./...`, lint (0 issues), deadcode clean, unit + acceptance green (comment-only code change). This pull request and its description were written by Isaac.
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.
Stacked on #5960 (flag renames), which is stacked on #5959 (command rename).
What
Adds
--cluster-nameas a compute target forenvironments setup-local, per the[P0] CLI Changesspec. It resolves the cluster name to an ID via the Clusters API, then resolves identically to--cluster-id(source=cluster, env key from the resolved cluster'sspark_version).Changes
ComputeClientgainsGetClusterByName;sdkComputeimplements it viaClusters.ListAllwith aFilterBy.ClusterStatesfilter (PENDING/RUNNING/RESTARTING/RESIZING) and matches on name. This intentionally avoids the SDK'sClusters.GetByClusterName, which lists clusters in every state and errors on any name collision: a terminated cluster sharing a name with a live one would otherwise block resolution for a name the user reasonably considers unique. An unknown name (no active match) or an ambiguous name (more than one active cluster shares it) is surfaced as an actionableE_RESOLVE.--cluster-namejoins the mutually-exclusive target group (--cluster-id/--cluster-name/--serverless-version/--job-id) and the bundle-fallback guard.ResolveTargetprecedence branch after--cluster-id.Testing
E_RESOLVE, and--cluster-id/--cluster-namemutual exclusivity.cluster-name-check(happy path, stubbedclusters/list), pluscluster-name-ambiguousandcluster-name-unknown(bothE_RESOLVE); help golden updated.go build ./..., lint (0 issues), deadcode clean, unit + acceptance green.This pull request and its description were written by Isaac.