Fix CLI token source --profile fallback with version detection - #1605
Conversation
4cb4c26 to
82d9599
Compare
Range-diff: stack/force-refresh-flag (4cb4c26 -> 82d9599)
Reproduce locally: |
82d9599 to
68d45f4
Compare
Range-diff: stack/force-refresh-flag (82d9599 -> 68d45f4)
Reproduce locally: |
68d45f4 to
4a5079f
Compare
Range-diff: stack/force-refresh-flag (68d45f4 -> 4a5079f)
Reproduce locally: |
4a5079f to
6f4fead
Compare
Range-diff: stack/force-refresh-flag (4a5079f -> 6f4fead)
Reproduce locally: |
6f4fead to
2218270
Compare
Range-diff: stack/force-refresh-flag (6f4fead -> 2218270)
Reproduce locally: |
2218270 to
76e74ca
Compare
Range-diff: stack/force-refresh-flag (2218270 -> 76e74ca)
Reproduce locally: |
- Replace custom cliVersion struct with golang.org/x/mod/semver for robust version parsing and comparison. - Use displayVersion helper instead of a String() method on a struct, so the empty (unknown) case is handled explicitly. - Clarify --profile global-flag comment to explain why version detection (not runtime probing) is needed. - Tighten the --profile fallback warning and use len(cmd) == 0 instead of comparing against nil. - Fix the misleading exec.ExitError comment so it describes why we prefer stderr over the wrapped error. - Consolidate TestNewCliTokenSource subtests into a table-driven form consistent with the rest of the file. Signed-off-by: Mihai Mitrea <mihai.mitrea@databricks.com>
Resolves conflict in NEXT_CHANGELOG.md: v0.127.0 and v0.128.0 have already shipped, so their entries have moved to CHANGELOG.md. Only the Layer 1 bug-fix entry remains under v0.129.0 > Bug Fixes. Signed-off-by: Mihai Mitrea <mihai.mitrea@databricks.com>
Dev builds of the Databricks CLI (plain `go build` without the goreleaser ldflags) report their version as `v0.0.0-dev[+<commit>]`. That sentinel compares less than every real release in semver, so capability checks like `--profile` (>= v0.207.1) fail closed and the SDK silently drops back to the conservative flag set. This is the safe default — we'd rather under-claim than over-claim capabilities — but leaves CLI contributors wondering why their locally built binary "doesn't support" features that clearly exist in main. Detect the `v0.0.0-dev` prefix in resolveCliCommand and emit an info-level log pointing to the exact ldflags workaround that matches the CLI's own goreleaser config. Behavior is unchanged; only observability improves. Covered by TestNewCliTokenSource_DevBuildInfoLog and two new cases in TestParseCliVersion. Signed-off-by: Mihai Mitrea <mihai.mitrea@databricks.com>
Reorganize buildCliCommand so the happy path (use `--profile` on a supported CLI) sits at the lowest indentation and every branch is terminal — each `if` ends in a `return` instead of mutating a shared `cmd` variable. Extract the --host fallback into buildHostCommand, so the reader of buildCliCommand doesn't need to understand host construction to follow the decision tree. buildHostCommand returns nil when cfg.Host is unset, which naturally propagates the "neither profile nor host usable" case back up to resolveCliCommand. Per review feedback on #1605 (r3106582935). No behavior change; all existing TestBuildCliCommand cases pass unchanged, plus a new TestBuildHostCommand for the extracted helper. Signed-off-by: Mihai Mitrea <mihai.mitrea@databricks.com>
The global-flag comment explains why buildCliCommand must use version detection to decide whether to emit --profile. Placing it at the top of the function reads as if it applies to the empty-profile guard just below. Moving it to sit directly above the semver.Compare check (with surrounding blank lines for breathing room) makes it clear the comment describes that specific decision. Pure whitespace/comment move. No behavior change, no test changes. Signed-off-by: Mihai Mitrea <mihai.mitrea@databricks.com>
| // Such builds always compare less than any real release, which would disable | ||
| // every feature gate; we surface an informational hint instead. | ||
| // See https://github.com/databricks/cli/blob/main/internal/build/info.go. |
There was a problem hiding this comment.
Let's move this part of the comment next to the logic that processes the value.
| // buildCliCommand constructs the CLI command for fetching an auth token. | ||
| // The CLI version determines which flags are used. Returns nil when neither | ||
| // profile nor host is usable. | ||
| func buildCliCommand(ctx context.Context, cliPath string, cfg *Config, ver string) []string { |
There was a problem hiding this comment.
Let's pipe the error so that we can show a precise message (is it the host? is it the profile?). This is also more extendable.
| func buildCliCommand(ctx context.Context, cliPath string, cfg *Config, ver string) []string { | |
| func buildCliCommand(ctx context.Context, cliPath string, cfg *Config, ver string) ([]string, error) { |
renaudhartert-db
left a comment
There was a problem hiding this comment.
LGTM modulo resolution of open comments.
Three fixes per Renaud's review on #1605: 1. Move the dev-build behavior description from the devBuildVersionPrefix const doc to resolveCliCommand where the handling actually happens. The const doc now only describes what the sentinel is; the consequence of encountering it lives next to the logger.Infof call. 2. Change buildCliCommand to return ([]string, error) so callers get a precise reason when no command can be built: - "neither profile nor host is configured" when both are empty. - "Databricks CLI X does not support --profile (requires >= Y) and no host fallback is configured" when profile is set on an old CLI and there's nothing to fall back to. Also skip the misleading "Falling back to --host" warning when there is no host to fall back to. buildHostCommand now assumes its caller has verified cfg.Host is non-empty. 3. Guard against empty exitErr.Stderr in execCliCommand so we don't render "cannot get access token: : exit status N" with a double colon when the CLI dies without writing to stderr (e.g. killed by a signal). When stderr is non-empty we still include it in the message and wrap exitErr with %w to preserve it for errors.As. Tests updated: TestBuildCliCommand now covers both new error paths, TestBuildHostCommand drops its "no host — nil" case (the caller now guards), and TestCliTokenSource_Token covers the empty-stderr case. Signed-off-by: Mihai Mitrea <mihai.mitrea@databricks.com>
Three fixes per Renaud's review on #1605: 1. Move the dev-build behavior description from the devBuildVersionPrefix const doc to resolveCliCommand where the handling actually happens. The const doc now only describes what the sentinel is; the consequence of encountering it lives next to the logger.Infof call. 2. Change buildCliCommand to return ([]string, error) so callers get a precise reason when no command can be built: - "neither profile nor host is configured" when both are empty. - "Databricks CLI X does not support --profile (requires >= Y) and no host fallback is configured" when profile is set on an old CLI and there's nothing to fall back to. Also skip the misleading "Falling back to --host" warning when there is no host to fall back to. buildHostCommand now assumes its caller has verified cfg.Host is non-empty. 3. Guard against empty exitErr.Stderr in execCliCommand so we don't render "cannot get access token: : exit status N" with a double colon when the CLI dies without writing to stderr (e.g. killed by a signal). When stderr is non-empty we still include it in the message and wrap exitErr with %w to preserve it for errors.As. Tests updated: TestBuildCliCommand now covers both new error paths, TestBuildHostCommand drops its "no host — nil" case (the caller now guards), and TestCliTokenSource_Token covers the empty-stderr case. Signed-off-by: Mihai Mitrea <mihai.mitrea@databricks.com>
a90b93b to
a76e875
Compare
Three fixes per Renaud's review on #1605: 1. Move the dev-build behavior description from the devBuildVersionPrefix const doc to resolveCliCommand where the handling actually happens. The const doc now only describes what the sentinel is; the consequence of encountering it lives next to the logger.Infof call. 2. Change buildCliCommand to return ([]string, error) so callers get a precise reason when no command can be built: - "neither profile nor host is configured" when both are empty. - "Databricks CLI X does not support --profile (requires >= Y) and no host fallback is configured" when profile is set on an old CLI and there's nothing to fall back to. Also skip the misleading "Falling back to --host" warning when there is no host to fall back to. buildHostCommand now assumes its caller has verified cfg.Host is non-empty. 3. Guard against empty exitErr.Stderr in execCliCommand so we don't render "cannot get access token: : exit status N" with a double colon when the CLI dies without writing to stderr (e.g. killed by a signal). When stderr is non-empty we still include it in the message and wrap exitErr with %w to preserve it for errors.As. Tests updated: TestBuildCliCommand now covers both new error paths, TestBuildHostCommand drops its "no host — nil" case (the caller now guards), and TestCliTokenSource_Token covers the empty-stderr case. Signed-off-by: Mihai Mitrea <mihai.mitrea@databricks.com>
a76e875 to
1a52600
Compare
|
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. |
Summary
Fix the broken
--profilefallback inCliTokenSourceby replacing error-based detection with version-based CLI detection at init time.Why
The
--profileflag ondatabricks auth tokenis a global Cobra flag (defined as a persistent flag on the root command). Old CLIs (< v0.207.1) silently accept it — they don't report"unknown flag: --profile"but instead fail later with"cannot fetch credentials". This means the existingisUnknownFlagErrorcheck (config/cli_token_source.go:120) never matches, and the--hostfallback is dead code.This was verified by testing against CLI v0.207.0 vs v0.207.1:
databricks auth token --profile workspace→Error: init: cannot fetch credentials(not "unknown flag")databricks auth token --profile workspace→ returns a valid tokenApproaches considered
Three approaches were evaluated for detecting whether the installed CLI supports
--profile:Error-based detection (try-and-retry) — the current approach on
main. Rundatabricks auth token --profile <name>and check whether the error contains"unknown flag: --profile". This is broken: because--profileis a global Cobra flag, old CLIs accept it silently and fail with a different error ("cannot fetch credentials"), so the fallback to--hostnever triggers.--helpflag parsing (databricks auth token --help+ substring matching) was rejected because the--helpoutput format is not a stable API. More importantly,--profilewould appear in--helpoutput even on old CLIs that don't actually implement profile-based token lookup — it shows up because it's a global persistent flag, not because theauth tokensubcommand uses it. This approach has the same fundamental flaw as error-based detection.Version detection (
databricks version+ semver comparison) — the approach taken here. Rundatabricks versionat init time, parse the semver (e.g.,"Databricks CLI v0.207.1"), and compare against known minimum versions for each flag. This is reliable because the version string is a stable output format, and the mapping between flags and CLI versions is well-defined (databricks/cli#855 for--profilein v0.207.1). If version detection fails, the SDK falls back to the most conservative command (--hostonly).References
--profilesupport added in CLI v0.207.1: databricks/cli#855 (Oct 2023)What changed
Interface changes
None.
CliTokenSourceis not part of the public API surface.NewCliTokenSourcenow takescontext.Contextas its first parameter, needed forexec.CommandContextwhen runningdatabricks version. This is consistent with everyCredentialsStrategy.Configuremethod in the codebase, and the single caller (auth_u2m.go) already hasctxin scope.Behavioral changes
cfg.Profileis set but the CLI is too old (< v0.207.1), the SDK now correctly falls back to--host. Previously this fallback was dead code.--profileflag is not supported and the SDK falls back to--host.Internal changes
cliVersiontype: semver parsing withAtLeast()comparison andString()formatting.getCliVersion(ctx, cliPath): runsdatabricks versionand parses the output.parseCliVersion: parses"Databricks CLI v0.207.1"→cliVersion{0, 207, 1}.resolveCliCommand: bridges version detection and command building. Falls back to zero version on detection failure.buildCliCommand: pure function — takes a version, returns a single resolved command. No exec calls, easy to test.CliTokenSourcesimplified to a singlecmd []stringfield. NohostCmd, no runtime fallback.Token()is now one line:return c.execCliCommand(ctx, c.cmd).isUnknownFlagError,buildCliCommands(plural),buildHostCommand,hostCmdfield.How is this tested?
Manual tests on versions 0.207.0 and 0.207.1
Unit tests in
config/cli_token_source_test.go:TestParseCliVersion— standard versions, patch versions, malformed output, empty string, missing prefix.TestCliVersion_AtLeast— equal, higher/lower patch/minor/major, zero vs zero, zero vs nonzero.TestBuildCliCommand— table-driven: version x config → expected command. Covers: host-only, account host, profile+new CLI (uses --profile), profile+old CLI (falls back to --host), profile-only+old CLI (nil), zero version (detection failed, falls back to --host), neither profile nor host (nil).TestNewCliTokenSource— success with host, success with profile, CLI not found, neither profile nor host.TestCliTokenSource_Token— success, CLI error, invalid JSON.