Skip to content

Use profile information when getting a token using the CLI #855

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
hectorcast-db merged 3 commits into from
Oct 11, 2023
Merged

Use profile information when getting a token using the CLI #855

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bb6eadc
Use profile information when getting a token using the CLI
hectorcast-db Oct 10, 2023
3a38e53
Extract method to set host
hectorcast-db Oct 10, 2023
d432627
Throw an error if both profile and host are provided
hectorcast-db Oct 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 19 additions & 11 deletions cmd/auth/login.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,20 +60,10 @@ func newLoginCommand(persistentAuth *auth.PersistentAuth) *cobra.Command {
profileName = profile
}

// If the chosen profile has a hostname and the user hasn't specified a host, infer the host from the profile.
_, profiles, err := databrickscfg.LoadProfiles(func(p databrickscfg.Profile) bool {
return p.Name == profileName
})
err := setHost(ctx, profileName, persistentAuth, args)
if err != nil {
return err
}
if persistentAuth.Host == "" {
if len(profiles) > 0 && profiles[0].Host != "" {
persistentAuth.Host = profiles[0].Host
} else {
configureHost(ctx, persistentAuth, args, 0)
}
}
defer persistentAuth.Close()

// We need the config without the profile before it's used to initialise new workspace client below.
Expand Down Expand Up @@ -135,3 +125,21 @@ func newLoginCommand(persistentAuth *auth.PersistentAuth) *cobra.Command {

return cmd
}

func setHost(ctx context.Context, profileName string, persistentAuth *auth.PersistentAuth, args []string) error {
// If the chosen profile has a hostname and the user hasn't specified a host, infer the host from the profile.
_, profiles, err := databrickscfg.LoadProfiles(func(p databrickscfg.Profile) bool {
return p.Name == profileName
})
if err != nil {
return err
}
if persistentAuth.Host == "" {
if len(profiles) > 0 && profiles[0].Host != "" {
persistentAuth.Host = profiles[0].Host
} else {
configureHost(ctx, persistentAuth, args, 0)
}
}
return nil
}
17 changes: 15 additions & 2 deletions cmd/auth/token.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package auth
import (
"context"
"encoding/json"
"errors"
"time"

"github.com/databricks/cli/libs/auth"
Expand All @@ -21,8 +22,20 @@ func newTokenCommand(persistentAuth *auth.PersistentAuth) *cobra.Command {

cmd.RunE = func(cmd *cobra.Command, args []string) error {
ctx := cmd.Context()
if persistentAuth.Host == "" {
configureHost(ctx, persistentAuth, args, 0)

var profileName string
Copy link
Copy Markdown
Contributor

@andrewnester andrewnester Oct 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would happen if we run the command with host positional argument and profile flag? Like

databricks auth token https://foo.com --profile bar

Copy link
Copy Markdown
Contributor Author

@hectorcast-db hectorcast-db Oct 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be ignored. The profile flag is used only to get the host from the config. I can't see any valid use-case of passing both as parameters.

Copy link
Copy Markdown
Contributor

@andrewnester andrewnester Oct 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The confusing point here is when there's a mismatch between profile host and host in argument. We either should not allow to provide both at the same time or error out if there's host mismatch

Copy link
Copy Markdown
Contributor Author

@hectorcast-db hectorcast-db Oct 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we throw an error if the url is provided on top of the profile?

Copy link
Copy Markdown
Contributor

@andrewnester andrewnester Oct 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say we throw an error when host from profile and host from args do not match. @pietern wdyt?

Copy link
Copy Markdown
Contributor

@pietern pietern Oct 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the value of doing that over simply erroring because both a profile name and a host are specified.

Are there valid cases where you specify both?

If not we should make these mutually exclusive.

Copy link
Copy Markdown
Contributor Author

@hectorcast-db hectorcast-db Oct 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only use the profile to select the host. So currently there is no use case for passing both. I added an error if they are both specified.

profileFlag := cmd.Flag("profile")
if profileFlag != nil {
profileName = profileFlag.Value.String()
// If a profile is provided we read the host from the .databrickscfg file
if profileName != "" && len(args) > 0 {
return errors.New("providing both a profile and a host parameters is not supported")
}
}

err := setHost(ctx, profileName, persistentAuth, args)
if err != nil {
return err
}
defer persistentAuth.Close()

Expand Down