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: 092818a
33 interesting tests: 22 KNOWN, 9 flaky, 1 SKIP, 1 RECOVERED
Top 50 slowest tests (at least 2 minutes):
|
rauchy
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
cmd/auth/login.go
Outdated
| Host: authArguments.Host, | ||
| AccountID: authArguments.AccountID, | ||
| AuthType: "databricks-cli", | ||
| ConfigFile: os.Getenv("DATABRICKS_CONFIG_FILE"), |
Contributor
There was a problem hiding this comment.
I think the behaviour is slightly different now: before it overrode ConfigFile only when DATABRICKS_CONFIG_FILE is not empty, and now - all the time
Contributor
There was a problem hiding this comment.
My understanding from the comment at line 165 is that we should not even attempt to read the config file at all.
Author
There was a problem hiding this comment.
I think the behaviour is slightly different now: before it overrode ConfigFile only when DATABRICKS_CONFIG_FILE is not empty, and now - all the time
I had thought the if statement was redundant because cfg.ConfigFile remains "" if the environment variable is not set, but I think it is indeed irrelevant given this:
My understanding from the comment at line 165 is that we should not even attempt to read the config file at all.
Since we aren't providing a profile name, nothing can be loaded from the config file.
I don't see the purpose of adding the ConfigFile path to the config struct, even in the earlier code so I'll remove the line. Please correct me if I'm missing something.
Author
There was a problem hiding this comment.
I've added an acceptance test for to ensure the behaviour doesn't change from removing the line (named custom-config-file).
| } | ||
| } | ||
|
|
||
| if profileName != "" { |
Contributor
There was a problem hiding this comment.
Isn't that condition always true?
Author
There was a problem hiding this comment.
It surprisingly is not.
func newLoginCommand(authArguments *auth.AuthArguments) *cobra.Command {
// ...
profileName := cmd.Flag("profile").Value.String()
// ...
// If the user has not specified a profile name, prompt for one.
if profileName == "" {
var err error
profileName = getProfileName(authArguments)
if profileName == "" {
profileName = "DEFAULT"
}
profileName, err = promptForProfile(ctx, profileName) // <--- this call
if err != nil {
return err
}
}
// ...
}The call to promptForProfile can return the empty string if interactive prompts are not supported.
func promptForProfile(ctx context.Context, defaultValue string) (string, error) {
if !cmdio.IsPromptSupported(ctx) {
return "", nil
}
// ....
}Which means that the CLI's auth login command actually can be used without saving a profile.
Contributor
There was a problem hiding this comment.
Which means that the CLI's auth login command actually can be used without saving a profile.
Drive by comment: Worth confirming this. My recollection is that if you do not specify a profile, we automatically save to the default profile.
cmd/auth/login.go
Outdated
| Host: authArguments.Host, | ||
| AccountID: authArguments.AccountID, | ||
| AuthType: "databricks-cli", | ||
| ConfigFile: os.Getenv("DATABRICKS_CONFIG_FILE"), |
Contributor
There was a problem hiding this comment.
My understanding from the comment at line 165 is that we should not even attempt to read the config file at all.
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
Comment on lines
+161
to
+162
| // An OAuth token has been successfully obtained at this point. | ||
| // We now attempt to save the current configuration as a profile in the config file. |
Contributor
There was a problem hiding this comment.
Nit
Suggested change
| // An OAuth token has been successfully obtained at this point. | |
| // We now attempt to save the current configuration as a profile in the config file. | |
| // At this point, an OAuth token has been successfully minted and stored | |
| // in the CLI cache. The rest of the command focuses on: | |
| // 1. Configuring cluster and serverless; | |
| // 2. Saving the profile. |
| w, err := databricks.NewWorkspaceClient(&databricks.Config{ | ||
| Host: authArguments.Host, | ||
| AccountID: authArguments.AccountID, | ||
| AuthType: "databricks-cli", |
Contributor
There was a problem hiding this comment.
With the Go SDK using the CLI in databricks-cli, this will effectively make the CLI spawn a child CLI process. It would be great if we could directly re-use the token that just got minted instead.
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.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
NO_CHANGELOG=true