Interactive profile selection on multiple host matches#4604
Open
simonfaltum wants to merge 4 commits intomainfrom
Open
Interactive profile selection on multiple host matches#4604simonfaltum wants to merge 4 commits intomainfrom
simonfaltum wants to merge 4 commits intomainfrom
Conversation
When multiple profiles in ~/.databrickscfg match the same host, instead of erroring, the CLI now: filters to workspace-compatible profiles, auto-selects if only one remains, prompts interactively if multiple remain, or returns an actionable error in non-interactive mode.
- Use cmd.CommandPath() instead of hardcoded "databricks bundle deploy" in the non-interactive fix hint, so guidance matches the actual command - Add interactive prompt test that verifies the select prompt fires when PromptSupported is true and multiple workspace profiles match - Improve RetryWorkspaceClient test to prove re-execution by changing the profile between attempts and asserting the error message changes
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Collaborator
|
Commit: 676e720
19 interesting tests: 7 KNOWN, 7 SKIP, 4 flaky, 1 RECOVERED
Top 20 slowest tests (at least 2 minutes):
|
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
requested review from
andrewnester,
anton-107,
denik,
pietern and
shreyas-goenka
as code owners
| // WorkspaceClientE() to attempt client creation again on the next call. | ||
| // This is used after resolving profile ambiguity to retry with the | ||
| // selected profile. | ||
| func (b *Bundle) RetryWorkspaceClient() { |
Contributor
There was a problem hiding this comment.
Maybe a better name is ClearWorkspaceClient?
Member
Author
There was a problem hiding this comment.
Makes sense, ClearWorkspaceClient is more accurate since it just clears the cache. Renamed.
|
|
||
| // promptForProfileByHost prompts the user to select a profile when multiple | ||
| // profiles match the same host. | ||
| func promptForProfileByHost(ctx context.Context, profiles profile.Profiles, host string) (string, error) { |
Contributor
There was a problem hiding this comment.
Can you please add a screenshot / video what the selection UI looks like to the PR description?
| logdiag.LogError(ctx, err) | ||
| return | ||
| // Check if this is a multi-profile ambiguity error. | ||
| names, isMulti := databrickscfg.AsMultipleProfiles(err) |
Contributor
There was a problem hiding this comment.
style: can you refactor this to a separate method?
Member
Author
There was a problem hiding this comment.
Agreed, extracted to a dedicated function. Much cleaner.
| case 1: | ||
| // Exactly one workspace-compatible profile — auto-select. | ||
| // This is deterministic and works in non-interactive mode. | ||
| selected = profiles[0].Name |
Contributor
There was a problem hiding this comment.
Do we hit this line of code? I would assume this error branch would never trigger if the profile was successfully determined.
Member
Author
There was a problem hiding this comment.
Yes I think we do though it might not be happening until we live in a host-agnostic world. The SDK's ResolveProfileFromHost returns all profiles matching the host including account-only profiles. After we filter to workspace-compatible ones, we may end up with just one. The test TestBundleConfigureMultiMatchAutoSelectSingleWorkspace covers this: two profiles for the same host, one workspace and one account-only, and the workspace one is auto-selected.
| cmd := emptyCommand(t) | ||
| ctx := logdiag.InitContext(cmd.Context()) | ||
| logdiag.SetCollect(ctx, true) | ||
| cmd.SetContext(ctx) |
Contributor
There was a problem hiding this comment.
Lets convert any tests that run commands non-interactively to acceptance tests? They are often a better framework when testing CLI commands end to end.
| // WorkspaceClientE() to attempt client creation again on the next call. | ||
| // This is used after resolving profile ambiguity to retry with the | ||
| // selected profile. | ||
| func (b *Bundle) RetryWorkspaceClient() { |
Contributor
There was a problem hiding this comment.
It might be better to encapsulate the prompt in the WorkspaceClientE method rather than clear the client and then set the client again. WDYT?
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
When you have two profiles in
~/.databrickscfgthat point to the same workspace (e.g.devandstagingboth withhost = https://myworkspace.cloud.databricks.com), and you run a bundle command from a bundle that setsworkspace.hostbut notprofile, the CLI crashes with a confusing error telling you to setDATABRICKS_CONFIG_PROFILEor--profile.With the host-agnostic auth work, users are encouraged to have multiple profiles pointing at the same workspace (different auth methods, different roles). This "multiple profiles matched" error will become much more common. Today it's a paper cut; soon it'll be a wall.
Changes
Before: The CLI errored immediately with
multiple profiles matched: dev, staging: please set DATABRICKS_CONFIG_PROFILE or provide --profile flag. No way to recover without restarting the command with a flag or env var.Now: When the CLI detects multiple profiles matching the same host, instead of erroring it:
--profileflag, env var)Implementation is catch-and-retry in
configureBundle: letWorkspaceClientE()run normally, catcherrMultipleProfiles, resolve the ambiguity, set the profile on the workspace config, reset thesync.Oncecache viaRetryWorkspaceClient(), and retry. On retry the SDK uses named-profile resolution (sinceProfileis now set), which is the correct code path.New helpers:
AsMultipleProfiles(err)— extracts profile names from the error chainMatchProfileNames(names...)— filters profiles by nameRetryWorkspaceClient()— resets the workspace client cache for retrypromptForProfileByHost()— interactive select UI for profile disambiguationScope is bundle-only.
ResolveProfileFromHostis only wired inWorkspace.Client(), so non-bundle commands are unaffected.Test plan
AsMultipleProfilesextracts names through the full wrapping chain; returns false for unrelated/nil errorsMatchProfileNamesmatches correct profiles, rejects others, handles empty inputRetryWorkspaceClientproves re-execution by changing config between attempts and asserting the error message changesSkipPromptrespected: error returned, no promptPromptSupported: true→ select prompt starts on stderrDATABRICKS_TOKENset →errMultipleProfilesnever firesmake checks,make lintfullpass