Move usage of config struct in auth login #4310
Open
Conversation
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
tejaskochar-db
requested review from
andrewnester,
anton-107,
denik,
pietern and
shreyas-goenka
as code owners
tejaskochar-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
tejaskochar-db
requested review from
renaudhartert-db
and removed request for
andrewnester,
anton-107,
denik,
pietern and
shreyas-goenka
Collaborator
|
Commit: 9a12e75
26 interesting tests: 8 flaky, 7 KNOWN, 5 SKIP, 4 RECOVERED, 2 BUG
Top 50 slowest tests (at least 2 minutes):
|
rauchy
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
tejaskochar-db
force-pushed
the
refactor-login
branch
from
10af05e to
76a6317
Compare
tejaskochar-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
tejaskochar-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
tejaskochar-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Improve comment Co-authored-by: Renaud Hartert <renaud.hartert@databricks.com>
tejaskochar-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
tejaskochar-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
…nly it works this time
tejaskochar-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
tejaskochar-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
pietern
reviewed
Jan 21, 2026
cmd/auth/login.go
Outdated
| ConfigFile: cfg.ConfigFile, | ||
| ServerlessComputeID: cfg.ServerlessComputeID, | ||
| Host: authArguments.Host, | ||
| AuthType: "databricks-cli", |
Contributor
There was a problem hiding this comment.
Can this refer to a constant?
tejaskochar-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
pietern
approved these changes
Jan 21, 2026
renaudhartert-db
approved these changes
Jan 21, 2026
Contributor
renaudhartert-db
left a comment
renaudhartert-db
left a comment
There was a problem hiding this comment.
LGTM modulo remaining comments.
| // 1. Configuring cluster and serverless; | ||
| // 2. Saving the profile. | ||
|
|
||
| var clusterID, serverlessComputeID string |
Contributor
There was a problem hiding this comment.
Do we actually need these variables compared to setting the value in cfg as it was done before? I'm not convinced have these here is a readability improvement.
Author
There was a problem hiding this comment.
Do you mean moving cfg out of the switch block and accumulating values in it as before?
We have to either put them in cfg or in these variables. I wanted to move the cfg struct within the switch case to avoid others also getting confused about how the token command depends on the Go SDK, but I don't think thats a problem now since we've removed the databricks-cli auth type and added the comments you suggested. I don't have a strong opinion here but would lean towards keeping the config struct for configuring clusters separate from collecting fields to save to profile.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Changes
Refactors the
auth logincommand to make it clear that the config struct is only needed for making a request to configure clusters when a--configure-clustersflag is passed.The request itself is changed from using
databricks-cliauth type which creates a child CLI process to using the newly minted token directly in a custom auth type.Why
Reduce confusion about what part of the SDK is used by the CLI in the auth login command. This is being done as a part of an effort to make the
databricks-cliauth type consistent across the SDKs by having the Go SDK also rely on the CLI'sauth tokencommand like the Java and Python SDKs.Tests
--configure-clusterflag worksNO_CHANGELOG=true