Pass --profile to CLI token source and add --host fallback for older CLIs#1297
Conversation
…CLIs When cfg.profile is set, DatabricksCliTokenSource now uses `databricks auth token --profile <name>` as the primary command instead of `--host`. A --host fallback command is also built so that if the CLI returns "unknown flag: --profile" (older CLI version), the SDK retries with --host and emits a warning to upgrade. Ported from databricks-sdk-go#1497. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ell scripts Shell scripts (.sh) don't work on Windows. Replace with mocker.patch on _run_subprocess, consistent with how other tests in this file are written. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
51a8971 to
56c57f1
Compare
simonfaltum
left a comment
There was a problem hiding this comment.
LGTM, mostly minor nits. Consider the comment with stdout or stderr
| @@ -684,6 +688,18 @@ def refresh(self) -> oauth.Token: | |||
| message = stdout or stderr | |||
There was a problem hiding this comment.
I know you didn't touch this but I think this or means if the CLI dumps usage text to stdout and the real error ("unknown flag: --profile") to stderr, message will be the usage text and the fallback in refresh() won't trigger. might want stdout + "\n" + stderr or at least check both. the fallback tests only mock stderr so this doesn't get caught today.
| try: | ||
| return self._exec_cli_command(self._cmd) | ||
| except IOError as e: | ||
| if self._host_cmd is not None and "unknown flag: --profile" in str(e): |
There was a problem hiding this comment.
nit: maybe worth a quick comment here noting which CLI version introduced --profile support (or a link to the CLI PR), so a future reader knows when this fallback can eventually be removed.
| cli_path = self.__class__._find_executable(cli_path) | ||
|
|
||
| host_cmd = None | ||
| if cfg.profile: |
There was a problem hiding this comment.
praise: this guards against cfg.host being None! me gusta
| access_token_field: str, | ||
| expiry_field: str, | ||
| disable_async: bool = True, | ||
| host_cmd: Optional[List[str]] = None, |
There was a problem hiding this comment.
nit: host_cmd makes total sense in DatabricksCliTokenSource context but at the CliTokenSource level it's a bit opaque - fallback_cmd would be more self-documenting. totally fine to keep it if the Go SDK uses the same name for parity though.
- Rename host_cmd -> fallback_cmd for clarity at CliTokenSource level - Fix message construction to include both stdout and stderr so "unknown flag: --profile" in stderr is never masked by usage text in stdout - Add comment linking to Go PR for when the fallback can be removed - Add test covering the stdout+stderr case Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
If integration tests don't run automatically, an authorized user can run them manually by following the instructions below: Trigger: Inputs:
Checks will be approved automatically on success. |
## Why Today \`databricks auth token --profile <name>\` fails to find a valid token even when one exists — because the token was stored under the host URL key (via \`--host\`), not the profile name key. The SDK's \`DatabricksCliCredentialsProvider\` always passed \`--host\` regardless of whether a profile was configured, so the CLI never had a chance to do profile-based lookup. This is a port of [databricks-sdk-py#1297](databricks/databricks-sdk-py#1297) / [databricks-sdk-go#1497](databricks/databricks-sdk-go#1497). ## Changes ### \`CliTokenSource\` - Added \`fallbackCmd\` optional field: a fallback command for CLIs too old to support \`--profile\`. - Added inner \`CliCommandException extends IOException\`: its \`getMessage()\` returns the clean stderr-based message (what users see), while \`getFullOutput()\` exposes the combined stdout+stderr for flag detection — avoiding verbose combined output in user-facing errors. - Extracted \`execCliCommand()\` helper to run a single CLI invocation. - Updated \`getToken()\` to try the primary command first; if it fails with \`"unknown flag: --profile"\` (checked in full output across both streams), retries with \`fallbackCmd\` and emits a warning to upgrade the CLI. ### \`DatabricksCliCredentialsProvider\` - Renamed \`buildCliCommand\` → \`buildHostArgs\` (reflects its role as the \`--host\` legacy path). - When \`cfg.profile\` is set: uses \`--profile <name>\` as the primary command. If \`cfg.host\` is also present, builds a \`--host\` fallback via \`buildHostArgs()\` for compatibility with older CLIs. - When \`cfg.profile\` is empty: unchanged — existing \`--host\` path is used. ## Test plan - [x] \`--profile\` fails with \`"unknown flag: --profile"\` in stderr → retries with \`--host\`, succeeds - [x] \`--profile\` fails with \`"unknown flag: --profile"\` in stdout → fallback still triggered - [x] \`--profile\` fails with real auth error → no retry, error propagated - [x] \`fallbackCmd\` is \`null\` and \`--profile\` fails → original error raised - [x] All existing \`buildHostArgs\` tests (workspace, account, unified host variants) pass 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Why
Today
databricks auth token --profile <name>fails to find a valid token even when one exists — because the token was stored under the host URL key (via--host), not the profile name key. The SDK'sDatabricksCliTokenSourcealways passed--hostregardless of whether a profile was configured, so the CLI never had a chance to do profile-based lookup.This is a port of databricks-sdk-go#1497.
Changes
CliTokenSourcehost_cmdoptional parameter: a fallback--hostcommand for CLIs too old to support--profile._exec_cli_command()helper to run a single CLI invocation.refresh()to try the primary command first; if it fails with"unknown flag: --profile", retry withhost_cmdand emit a warning to upgrade the CLI.DatabricksCliTokenSource_build_host_args()static method from the existing inline--hostargument construction.cfg.profileis set: uses--profile <name>as the primary command. Ifcfg.hostis also present, builds a--hostfallback via_build_host_args()for compatibility with older CLIs.cfg.profileis empty: unchanged — existing--hostpath is used.Test plan
--profileset with host → primary cmd uses--profile, fallback--hostcmd is built--profileset without host → primary cmd uses--profile, no fallback--profilenot set → existing--hostpath unchangedrefresh():--profilefails with "unknown flag: --profile" → retries with--host, succeedsrefresh():--profilefails with real auth error → no retry, error propagatedrefresh():--profilefails, nohost_cmdset → original error raised🤖 Generated with Claude Code