Add text output mode for auth token and auth env#4725
Open
simonfaltum wants to merge 8 commits intomainfrom
Open
Add text output mode for auth token and auth env#4725simonfaltum wants to merge 8 commits intomainfrom
simonfaltum wants to merge 8 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
Collaborator
|
Commit: 23d833e
16 interesting tests: 7 SKIP, 6 RECOVERED, 3 flaky
Top 21 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
Contributor
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
|
|
||
| // collectEnvVars returns the environment variables for the given config | ||
| // as a map from env var name to value. | ||
| func collectEnvVars(cfg *config.Config) map[string]string { |
Contributor
There was a problem hiding this comment.
We have the auth.Env and auth.ProcessEnv functions. Worth looking into using those.
This command in general is pretty old and is also useless. It only returns the env corresponding to a profile. We should be able to make it more general i.e. return the env vars I need to set, to authenticate the same as the current CLI context.
I think a breaking change is pretty justified here (after double checking once with the usage numbers)
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
databricks auth tokenanddatabricks auth envalways output JSON, ignoring the--outputflag. Users who want to pipe the token string or source env vars have to parse JSON first.Changes
Before: Both commands always output JSON regardless of the
--outputflag.Now: Both commands respect
--output textwhen explicitly set:auth token --output text: outputs just the access token string, suitable for pipingauth env --output text: outputs KEY=VALUE lines, compatible with .env loaders and evalJSON remains the default (backward compatible). Text mode activates only when the user explicitly passes
--output text.Test plan
make checkspasses