Skip to content

[VPEX][10] Add --cluster-name target flag#5961

Open
rugpanov wants to merge 2 commits into
dbconnect/10-flag-renamesfrom
dbconnect/11-cluster-name
Open

[VPEX][10] Add --cluster-name target flag#5961
rugpanov wants to merge 2 commits into
dbconnect/10-flag-renamesfrom
dbconnect/11-cluster-name

Conversation

@rugpanov

@rugpanov rugpanov commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

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.

@rugpanov
Add --cluster-name target flag
f5ce185 
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
This was referenced Jul 17, 2026
Open
Open
Open
Open
@rugpanov rugpanov changed the title [VPEX] Add --cluster-name target flag [VPEX][10] Add --cluster-name target flag Jul 17, 2026
@eng-dev-ecosystem-bot

eng-dev-ecosystem-bot commented Jul 17, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Integration test report

Commit: 08c5535

Run: 29583957931

Env 💚​RECOVERED 🙈​SKIP ✅​pass 🙈​skip Time
💚​ aws linux 4 4 227 1122 4:49
💚​ aws windows 4 4 229 1120 9:14
💚​ aws-ucws linux 4 4 316 1038 6:35
💚​ aws-ucws windows 4 4 318 1036 7:40
💚​ azure linux 4 4 227 1121 5:02
💚​ azure windows 4 4 229 1119 6:45
💚​ azure-ucws linux 4 4 318 1035 8:22
💚​ azure-ucws windows 4 4 320 1033 8:06
💚​ gcp linux 4 4 226 1123 4:55
💚​ gcp windows 4 4 228 1121 9:00
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
8:42 aws windows TestAccept
8:36 gcp windows TestAccept
6:30 azure-ucws windows TestAccept
6:20 aws-ucws windows TestAccept
6:18 azure windows TestAccept
3:06 azure-ucws linux TestAccept
3:00 azure linux TestAccept
2:58 gcp linux TestAccept
2:56 aws linux TestAccept
2:50 aws-ucws linux TestAccept

@rugpanov
Address review: exclude terminated clusters in name resolution, list …
08c5535 
…--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
anton-107 approved these changes Jul 17, 2026
View reviewed changes

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

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 golden acceptance/localenv/cluster-name-ambiguous/output.txt:2). ClusterDetails is 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] GetByClusterName loads all clusters and misses long-terminated ones. The SDK's Clusters.GetByClusterName calls ListAll (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 on GetClusterByName noting the >30-day caveat would help the next reader.

  • [low] Test coverage: only the ambiguous path is exercised. TestResolveClusterNameFlagError and 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-name branch this PR inserts between cluster-id and serverless. Harmless to correctness (flags are mutually exclusive) but misleads a reader. (#5964 updated the sibling TargetInfo doc for "five flag sources" but left this one.)

Reviewed with AI assistance (build + unit tests + adversarial verification against the checked-out top of stack).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anton-107 anton-107 anton-107 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@rugpanov @eng-dev-ecosystem-bot @anton-107