[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):
|
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.