Skip to content

ssh setup: use the shared cluster picker so duplicate cluster names work - #6298

Open
TangoEnSkai wants to merge 1 commit into
databricks:mainfrom
TangoEnSkai:fix/ssh-setup-duplicate-cluster-names
Open

ssh setup: use the shared cluster picker so duplicate cluster names work#6298
TangoEnSkai wants to merge 1 commit into
databricks:mainfrom
TangoEnSkai:fix/ssh-setup-duplicate-cluster-names

Conversation

@TangoEnSkai

Copy link
Copy Markdown

Changes

The ssh setup interactive cluster picker called
ClusterDetailsClusterNameToClusterIdMap, which builds a map[string]string
keyed by cluster display name. Databricks allows several clusters to share a
name, so on those workspaces listing failed outright:

Error: failed to load names for Clusters drop-down. Please manually specify
cluster argument. Original error: duplicate .ClusterName: <name>

This switches the prompt to cfgpickers.AskForCluster, the picker
auth login --configure-cluster already uses. It fits without modification:

  • lists via ListAll with the same ClusterSourceApi/ClusterSourceUi
    filter this prompt was already using,
  • keys the picker by list position rather than by display name, so duplicate
    names are not a problem,
  • already renders the cluster ID next to the name, so duplicates are
    distinguishable in the list.

Passing no filters keeps the current selection semantics — every API/UI
cluster stays eligible.

Two behaviour changes worth calling out explicitly:

  • When the workspace has exactly one cluster, AskForCluster returns it
    without prompting. Previously ssh setup always prompted.
  • The list now also shows state, access mode and runtime, and is searchable.
    Rendering those costs one CurrentUser.Me and one SparkVersions call.

The prompt label changes from "The cluster to connect to" to
AskForCluster's "Choose compatible cluster".

Why

Users on workspaces with duplicate cluster names cannot use ssh setup
interactively at all — the command fails before showing anything, and the
only way forward is to find the cluster ID by hand and pass --cluster.

Reusing the existing picker rather than adding a second one keeps the two
interactive cluster selections in the CLI behaving the same way.

Part of #974. That issue was originally reported against
auth login --configure-cluster, which was fixed by moving to
AskForCluster; ssh setup was left on the name-keyed SDK helper. Note it
does not close the issue completely: the generated commands in
cmd/workspace/clusters still call the same helper in 13 places, but those
are produced by the SDK generator and cannot be fixed in this repository.

Tests

go test ./experimental/ssh/... ./libs/databrickscfg/... — all green.

Two tests added to experimental/ssh/internal/setup/setup_test.go, driving
defaultClusterSelectionPrompt against qa.HTTPFixtures:

  • duplicate names: two clusters sharing a display name. Listing must
    succeed and reach the picker, which cannot run without a TTY, so the test
    asserts the failure comes from the picker and not from loading. Against
    main this fails with duplicate .ClusterName: shared name.
  • single cluster: asserts the ID is returned directly.

The interactive picker called ClusterDetailsClusterNameToClusterIdMap, which
builds a map keyed by cluster display name. Databricks allows several clusters
to share a name, so listing failed outright with "duplicate .ClusterName".

Use cfgpickers.AskForCluster instead. It lists with the same API/UI
cluster-source filter, keys the picker by list position rather than by name,
and already renders the cluster ID next to the name so duplicates can be told
apart.

Two side effects worth noting: the picker now auto-selects when the workspace
has exactly one cluster, and it shows state, access mode and runtime, which
costs a CurrentUser.Me and a SparkVersions call.

Part of databricks#974. The generated commands in cmd/workspace/clusters still use the
name-keyed helper; those come from the SDK generator and are not fixable here.
@github-actions

Copy link
Copy Markdown
Contributor

An authorized user can trigger integration tests manually by following the instructions below:

Trigger:
go/deco-tests-run/cli

Inputs:

  • PR number: 6298
  • Commit SHA: 393944c5ff0e897422428a16f388a08ee809272e

Checks will be approved automatically on success.

@github-actions

Copy link
Copy Markdown
Contributor

Waiting for approval

Based on git history, these people are best suited to review:

  • @janniklasrose -- recent work in .nextchanges/cli/
  • @pietern -- recent work in experimental/ssh/internal/setup/, .nextchanges/cli/
  • @lennartkats-db -- recent work in .nextchanges/cli/

Eligible reviewers: @andrewnester, @anton-107, @denik, @rclarey, @renaudhartert-db, @rugpanov, @shreyas-goenka, @simonfaltum

Suggestions based on git history. See OWNERS for ownership rules.

@TangoEnSkai

Copy link
Copy Markdown
Author

@pietern you have the most recent work in experimental/ssh/internal/setup/, so tagging you rather than the reviewers the bot picked up from the changelog fragment.

The fix itself is small — it reuses cfgpickers.AskForCluster instead of adding a second picker — but it does change two user-visible things I would rather have you decide on than assume:

  1. With exactly one cluster in the workspace, AskForCluster returns it without prompting. ssh setup used to prompt unconditionally.
  2. The prompt label becomes "Choose compatible cluster", and rendering the extra columns costs a CurrentUser.Me and a SparkVersions call.

If either is unwanted, the alternative is to build the picker list locally in setup.go, which is what #5777 did before it was closed. I went with reuse because the two interactive cluster pickers in the CLI then behave the same way, but I am happy to switch.

Worth noting this does not close #974cmd/workspace/clusters still calls the name-keyed helper in 13 places, but that file is generated, so it would need a change on the SDK generator side.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant