Add text output mode for auth token and auth env#4725
Add text output mode for auth token and auth env#4725simonfaltum wants to merge 11 commits intomainfrom
Conversation
Both commands now respect --output text when explicitly set: - auth token --output text: outputs just the access token string - auth env --output text: outputs KEY=VALUE lines JSON remains the default for backward compatibility. Co-authored-by: Isaac
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
|
Commit: 1a3c536
20 interesting tests: 9 SKIP, 7 KNOWN, 4 flaky
Top 25 slowest tests (at least 2 minutes):
|
Co-authored-by: Isaac
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
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
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
left a comment
shreyas-goenka
left a comment
There was a problem hiding this comment.
Note: This review was posted by Claude (AI assistant). Shreyas will do a separate, more thorough review pass.
Priority: MEDIUM — Env var inconsistency needs a decision
HIGH: DATABRICKS_OUTPUT_FORMAT env var silently ignored
Both auth token and auth env commands use cmd.Flag("output").Changed to detect text mode, which means the DATABRICKS_OUTPUT_FORMAT=text env var is ignored. This is inconsistent with the rest of the CLI where the env var is a first-class way to set output format. The guard exists because these commands have an inverted default (JSON by default, unlike other commands), so without it the default behavior would break. A decision is needed on whether the env var should be honored here.
MEDIUM: Shell quoting not fully .env-file compatible
The quoteEnvValue function uses POSIX shell single-quote escaping ('\''), but the PR description claims ".env loader" compatibility. Most .env parsers (Docker, python-dotenv) use different quoting rules. Only eval usage is truly compatible.
LOW: Doc comment typo
quoteEnvValue doc references '\" but code does '\''.
What looks good
- Backward compatibility preserved (JSON remains default)
collectEnvVarsextraction reduces duplication- Clean text output formatting
The comment referenced the '\" escape sequence, but the code actually uses the POSIX '\'' sequence (end-quote, backslash-escaped literal single quote, re-open quote).
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
…al characters Values containing \n or \r were emitted unquoted, producing raw multi-line shell output instead of a single safe KEY=VALUE pair. Add both characters to shellQuotedSpecialChars so they trigger single-quoting.
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Use root.MustAnyClient and auth.Env instead of custom profile/host
resolution logic. This makes auth env return the environment variables
for the exact identity the CLI is authenticated as, including bundle
context and all standard auth resolution paths.
Breaking changes:
- Removed command-specific --host and --profile flags (use the
inherited flags from the parent/root commands)
- JSON output is a flat map instead of wrapped in {"env": ...}
- Only the primary env var per attribute is emitted (via auth.Env)
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
To be honest, even that is arguable. For example, env variables in OIDC settings are not something that users truly control — some of these are not even part of the profile. It is one of these commands that have badly aged; it might be a good candidate for deprecation or at least refactor. |
cmd/auth/token.go
Outdated
| _, _ = fmt.Fprintln(cmd.OutOrStdout(), t.AccessToken) | ||
| return nil |
There was a problem hiding this comment.
What's the rationale behind swallowing the error, here and in other places?
There was a problem hiding this comment.
Sure, I'll return the write errors instead. The pattern was carried over from the existing code but it's better to surface them so the CLI exits non-zero if stdout is broken.
cmd/auth/token.go
Outdated
| } | ||
|
|
||
| func writeTokenOutput(cmd *cobra.Command, t *oauth2.Token) error { | ||
| // Output plain token when the user explicitly passes --output text. |
There was a problem hiding this comment.
This comment does not bring much compared to reading the code. Rather, I think we should briefly explain why we discard implicit — backward compatibily?
There was a problem hiding this comment.
Fair point. I'll replace this with a comment explaining the actual reason: auth token defaults to JSON (unlike most CLI commands), so we only switch to text when the user explicitly passes --output text to avoid breaking scripts that parse the JSON output.
Address review feedback: return write errors from fmt.Fprintln,
fmt.Fprintf, and Write calls instead of discarding them with _, _.
Also improve the comment in writeTokenOutput to explain why we
check cmd.Flag("output").Changed rather than just restating what
the code does.
Co-authored-by: Isaac
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
There was a problem hiding this comment.
LGTM for the change related to auth token.
Though, I'd like to better understand the future of the auth env command and how we expect it to work with authentication types that rely on variables that are not owned by Databricks. This matters as ensuring proper implementation of this command has implication on the implementation of the auth library and the "profile type".
It might be worth to split this PR.
4 participants
Why
auth envhad its own custom auth resolution logic (local--host/--profileflags, manual profile-to-host matching via ini file scanning). It didn't go through the standard CLI auth chain, so it couldn't resolve auth from bundle context, default profiles, or environment variables. It was effectively a standalone tool that only worked with explicit profile or host arguments.The command should return the environment variables needed to authenticate as the exact same identity the CLI is currently authenticated as. This is what
bundle runalready does implicitly (viaauth.Envon the resolved config), but there was no explicit command for it.Additionally, both
auth tokenandauth envalways output JSON, ignoring the--outputflag.Changes
auth env: Refactored to use the CLI's standard auth resolution (root.MustAnyClient+cmdctx.ConfigUsed), same pattern asauth describe. Usesauth.Env(cfg)fromlibs/auth/env.goto generate env vars. This means it now works with bundle context, env var auth, default profiles, and all other standard auth paths.Before:
databricks auth env --host https://my-workspace.cloud.databricks.comwith custom profile matching.Now:
databricks auth envreturns the env vars for whatever identity the CLI resolved through its normal auth chain (profile flag, bundle config, env vars, default profile, etc.).Breaking changes:
--hostand--profileflags (the inherited flags from the parentauthand root commands cover this){"env": {...}}auth.Env, consistent withbundle run)Removed ~120 lines of dead code:
canonicalHost,resolveSection,loadFromDatabricksCfg,collectEnvVars,ErrNoMatchingProfiles.auth token: Respects--output textwhen explicitly set, outputting just the access token string suitable for piping.Both commands: Respect
--output textfor KEY=VALUE lines (auth env) or plain token string (auth token). JSON remains the default for backward compatibility.Test plan
--outputkeeps JSON)auth envtext modemake checkspasses