From 5b819cd982182542476fe249a4c0002e96bd6dd3 Mon Sep 17 00:00:00 2001 From: Miles Yucht Date: Mon, 14 Aug 2023 14:45:08 +0200 Subject: [PATCH] Always resolve .databrickscfg file (#659) ## Changes #629 introduced a change to autopopulate the host from .databrickscfg if the user is logging back into a host they were previously using. This did not respect the DATABRICKS_CONFIG_FILE env variable, causing the flow to stop working for users with no .databrickscfg file in their home directory. This PR refactors all config file loading to go through one interface, `databrickscfg.GetDatabricksCfg()`, and an auxiliary `databrickscfg.GetDatabricksCfgPath()` to get the configured file path. Closes #655. ## Tests ``` $ databricks auth login --profile abc Error: open /Users/miles/.databrickscfg: no such file or directory $ ./cli auth login --profile abc Error: cannot load Databricks config file: open /Users/miles/.databrickscfg: no such file or directory $ DATABRICKS_CONFIG_FILE=~/.databrickscfg.bak ./cli auth login --profile abc Databricks Host: https://asdf ``` --- cmd/auth/env.go | 5 +++-- cmd/auth/login.go | 2 +- cmd/auth/profiles.go | 24 +++++--------------- cmd/root/auth.go | 19 +++++++++------- libs/databrickscfg/profiles.go | 34 ++++++++++++++++++++++++----- libs/databrickscfg/profiles_test.go | 9 +++++--- 6 files changed, 55 insertions(+), 38 deletions(-) diff --git a/cmd/auth/env.go b/cmd/auth/env.go index 7bf3fd91..241d5f88 100644 --- a/cmd/auth/env.go +++ b/cmd/auth/env.go @@ -9,6 +9,7 @@ import ( "net/url" "strings" + "github.com/databricks/cli/libs/databrickscfg" "github.com/databricks/databricks-sdk-go/config" "github.com/spf13/cobra" "gopkg.in/ini.v1" @@ -28,7 +29,7 @@ func canonicalHost(host string) (string, error) { var ErrNoMatchingProfiles = errors.New("no matching profiles found") -func resolveSection(cfg *config.Config, iniFile *ini.File) (*ini.Section, error) { +func resolveSection(cfg *config.Config, iniFile *config.File) (*ini.Section, error) { var candidates []*ini.Section configuredHost, err := canonicalHost(cfg.Host) if err != nil { @@ -68,7 +69,7 @@ func resolveSection(cfg *config.Config, iniFile *ini.File) (*ini.Section, error) } func loadFromDatabricksCfg(cfg *config.Config) error { - iniFile, err := getDatabricksCfg() + iniFile, err := databrickscfg.Get() if errors.Is(err, fs.ErrNotExist) { // it's fine not to have ~/.databrickscfg return nil diff --git a/cmd/auth/login.go b/cmd/auth/login.go index e248118a..cf1d5c30 100644 --- a/cmd/auth/login.go +++ b/cmd/auth/login.go @@ -61,7 +61,7 @@ func newLoginCommand(persistentAuth *auth.PersistentAuth) *cobra.Command { } // 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(databrickscfg.DefaultPath, func(p databrickscfg.Profile) bool { + _, profiles, err := databrickscfg.LoadProfiles(func(p databrickscfg.Profile) bool { return p.Name == profileName }) if err != nil { diff --git a/cmd/auth/profiles.go b/cmd/auth/profiles.go index 2b08164f..97d8eeab 100644 --- a/cmd/auth/profiles.go +++ b/cmd/auth/profiles.go @@ -5,32 +5,16 @@ import ( "fmt" "net/http" "os" - "path/filepath" - "strings" "sync" "github.com/databricks/cli/libs/cmdio" + "github.com/databricks/cli/libs/databrickscfg" "github.com/databricks/databricks-sdk-go" "github.com/databricks/databricks-sdk-go/config" "github.com/spf13/cobra" "gopkg.in/ini.v1" ) -func getDatabricksCfg() (*ini.File, error) { - configFile := os.Getenv("DATABRICKS_CONFIG_FILE") - if configFile == "" { - configFile = "~/.databrickscfg" - } - if strings.HasPrefix(configFile, "~") { - homedir, err := os.UserHomeDir() - if err != nil { - return nil, fmt.Errorf("cannot find homedir: %w", err) - } - configFile = filepath.Join(homedir, configFile[1:]) - } - return ini.Load(configFile) -} - type profileMetadata struct { Name string `json:"name"` Host string `json:"host,omitempty"` @@ -111,10 +95,12 @@ func newProfilesCommand() *cobra.Command { cmd.RunE = func(cmd *cobra.Command, args []string) error { var profiles []*profileMetadata - iniFile, err := getDatabricksCfg() + iniFile, err := databrickscfg.Get() if os.IsNotExist(err) { // return empty list for non-configured machines - iniFile = ini.Empty() + iniFile = &config.File{ + File: &ini.File{}, + } } else if err != nil { return fmt.Errorf("cannot parse config file: %w", err) } diff --git a/cmd/root/auth.go b/cmd/root/auth.go index c13f7463..2f32d260 100644 --- a/cmd/root/auth.go +++ b/cmd/root/auth.go @@ -40,10 +40,7 @@ func MustAccountClient(cmd *cobra.Command, args []string) error { // 1. only admins will have account configured // 2. 99% of admins will have access to just one account // hence, we don't need to create a special "DEFAULT_ACCOUNT" profile yet - _, profiles, err := databrickscfg.LoadProfiles( - databrickscfg.DefaultPath, - databrickscfg.MatchAccountProfiles, - ) + _, profiles, err := databrickscfg.LoadProfiles(databrickscfg.MatchAccountProfiles) if err != nil { return err } @@ -124,8 +121,11 @@ func transformLoadError(path string, err error) error { } func askForWorkspaceProfile() (string, error) { - path := databrickscfg.DefaultPath - file, profiles, err := databrickscfg.LoadProfiles(path, databrickscfg.MatchWorkspaceProfiles) + path, err := databrickscfg.GetPath() + if err != nil { + return "", fmt.Errorf("cannot determine Databricks config file path: %w", err) + } + file, profiles, err := databrickscfg.LoadProfiles(databrickscfg.MatchWorkspaceProfiles) if err != nil { return "", transformLoadError(path, err) } @@ -156,8 +156,11 @@ func askForWorkspaceProfile() (string, error) { } func askForAccountProfile() (string, error) { - path := databrickscfg.DefaultPath - file, profiles, err := databrickscfg.LoadProfiles(path, databrickscfg.MatchAccountProfiles) + path, err := databrickscfg.GetPath() + if err != nil { + return "", fmt.Errorf("cannot determine Databricks config file path: %w", err) + } + file, profiles, err := databrickscfg.LoadProfiles(databrickscfg.MatchAccountProfiles) if err != nil { return "", transformLoadError(path, err) } diff --git a/libs/databrickscfg/profiles.go b/libs/databrickscfg/profiles.go index 7892bddd..864000d0 100644 --- a/libs/databrickscfg/profiles.go +++ b/libs/databrickscfg/profiles.go @@ -1,7 +1,9 @@ package databrickscfg import ( + "fmt" "os" + "path/filepath" "strings" "github.com/databricks/databricks-sdk-go/config" @@ -64,12 +66,34 @@ func MatchAllProfiles(p Profile) bool { return true } -const DefaultPath = "~/.databrickscfg" +// Get the path to the .databrickscfg file, falling back to the default in the current user's home directory. +func GetPath() (string, error) { + configFile := os.Getenv("DATABRICKS_CONFIG_FILE") + if configFile == "" { + configFile = "~/.databrickscfg" + } + if strings.HasPrefix(configFile, "~") { + homedir, err := os.UserHomeDir() + if err != nil { + return "", fmt.Errorf("cannot find homedir: %w", err) + } + configFile = filepath.Join(homedir, configFile[1:]) + } + return configFile, nil +} -func LoadProfiles(path string, fn ProfileMatchFunction) (file string, profiles Profiles, err error) { - f, err := config.LoadFile(path) +func Get() (*config.File, error) { + configFile, err := GetPath() if err != nil { - return + return nil, fmt.Errorf("cannot determine Databricks config file path: %w", err) + } + return config.LoadFile(configFile) +} + +func LoadProfiles(fn ProfileMatchFunction) (file string, profiles Profiles, err error) { + f, err := Get() + if err != nil { + return "", nil, fmt.Errorf("cannot load Databricks config file: %w", err) } homedir, err := os.UserHomeDir() @@ -106,7 +130,7 @@ func LoadProfiles(path string, fn ProfileMatchFunction) (file string, profiles P } func ProfileCompletion(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { - _, profiles, err := LoadProfiles(DefaultPath, MatchAllProfiles) + _, profiles, err := LoadProfiles(MatchAllProfiles) if err != nil { return nil, cobra.ShellCompDirectiveError } diff --git a/libs/databrickscfg/profiles_test.go b/libs/databrickscfg/profiles_test.go index 582c6658..b1acdce9 100644 --- a/libs/databrickscfg/profiles_test.go +++ b/libs/databrickscfg/profiles_test.go @@ -32,19 +32,22 @@ func TestLoadProfilesReturnsHomedirAsTilde(t *testing.T) { } else { t.Setenv("HOME", "./testdata") } - file, _, err := LoadProfiles("./testdata/databrickscfg", func(p Profile) bool { return true }) + t.Setenv("DATABRICKS_CONFIG_FILE", "./testdata/databrickscfg") + file, _, err := LoadProfiles(func(p Profile) bool { return true }) require.NoError(t, err) assert.Equal(t, "~/databrickscfg", file) } func TestLoadProfilesMatchWorkspace(t *testing.T) { - _, profiles, err := LoadProfiles("./testdata/databrickscfg", MatchWorkspaceProfiles) + t.Setenv("DATABRICKS_CONFIG_FILE", "./testdata/databrickscfg") + _, profiles, err := LoadProfiles(MatchWorkspaceProfiles) require.NoError(t, err) assert.Equal(t, []string{"DEFAULT", "query", "foo1", "foo2"}, profiles.Names()) } func TestLoadProfilesMatchAccount(t *testing.T) { - _, profiles, err := LoadProfiles("./testdata/databrickscfg", MatchAccountProfiles) + t.Setenv("DATABRICKS_CONFIG_FILE", "./testdata/databrickscfg") + _, profiles, err := LoadProfiles(MatchAccountProfiles) require.NoError(t, err) assert.Equal(t, []string{"acc"}, profiles.Names()) }