From 8d7e5630e1dbb42d0ddf5f4a3d93b4a194157570 Mon Sep 17 00:00:00 2001 From: Miles Yucht Date: Mon, 14 Aug 2023 14:13:02 +0200 Subject: [PATCH 1/2] Always resolve .databrickscfg file --- 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 7bf3fd91f6..f1894968d4 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.GetDatabricksCfg() 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 e248118aed..cf1d5c301b 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 2b08164f6b..863cbc0ae9 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.GetDatabricksCfg() 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 c13f746373..8bedda0500 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.GetDatabricksCfgPath() + 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.GetDatabricksCfgPath() + 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 7892bddd18..05c3fe880f 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 GetDatabricksCfgPath() (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 GetDatabricksCfg() (*config.File, error) { + configFile, err := GetDatabricksCfgPath() 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 := GetDatabricksCfg() + 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 582c6658e7..b1acdce92c 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()) } From 7b73e6a39b18e94be38600b0590fee2d089d1ae7 Mon Sep 17 00:00:00 2001 From: Miles Yucht Date: Mon, 14 Aug 2023 14:37:37 +0200 Subject: [PATCH 2/2] address comments --- cmd/auth/env.go | 2 +- cmd/auth/profiles.go | 2 +- cmd/root/auth.go | 4 ++-- libs/databrickscfg/profiles.go | 8 ++++---- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/cmd/auth/env.go b/cmd/auth/env.go index f1894968d4..241d5f880f 100644 --- a/cmd/auth/env.go +++ b/cmd/auth/env.go @@ -69,7 +69,7 @@ func resolveSection(cfg *config.Config, iniFile *config.File) (*ini.Section, err } func loadFromDatabricksCfg(cfg *config.Config) error { - iniFile, err := databrickscfg.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/profiles.go b/cmd/auth/profiles.go index 863cbc0ae9..97d8eeabcc 100644 --- a/cmd/auth/profiles.go +++ b/cmd/auth/profiles.go @@ -95,7 +95,7 @@ func newProfilesCommand() *cobra.Command { cmd.RunE = func(cmd *cobra.Command, args []string) error { var profiles []*profileMetadata - iniFile, err := databrickscfg.GetDatabricksCfg() + iniFile, err := databrickscfg.Get() if os.IsNotExist(err) { // return empty list for non-configured machines iniFile = &config.File{ diff --git a/cmd/root/auth.go b/cmd/root/auth.go index 8bedda0500..2f32d260e0 100644 --- a/cmd/root/auth.go +++ b/cmd/root/auth.go @@ -121,7 +121,7 @@ func transformLoadError(path string, err error) error { } func askForWorkspaceProfile() (string, error) { - path, err := databrickscfg.GetDatabricksCfgPath() + path, err := databrickscfg.GetPath() if err != nil { return "", fmt.Errorf("cannot determine Databricks config file path: %w", err) } @@ -156,7 +156,7 @@ func askForWorkspaceProfile() (string, error) { } func askForAccountProfile() (string, error) { - path, err := databrickscfg.GetDatabricksCfgPath() + path, err := databrickscfg.GetPath() if err != nil { return "", fmt.Errorf("cannot determine Databricks config file path: %w", err) } diff --git a/libs/databrickscfg/profiles.go b/libs/databrickscfg/profiles.go index 05c3fe880f..864000d034 100644 --- a/libs/databrickscfg/profiles.go +++ b/libs/databrickscfg/profiles.go @@ -67,7 +67,7 @@ func MatchAllProfiles(p Profile) bool { } // Get the path to the .databrickscfg file, falling back to the default in the current user's home directory. -func GetDatabricksCfgPath() (string, error) { +func GetPath() (string, error) { configFile := os.Getenv("DATABRICKS_CONFIG_FILE") if configFile == "" { configFile = "~/.databrickscfg" @@ -82,8 +82,8 @@ func GetDatabricksCfgPath() (string, error) { return configFile, nil } -func GetDatabricksCfg() (*config.File, error) { - configFile, err := GetDatabricksCfgPath() +func Get() (*config.File, error) { + configFile, err := GetPath() if err != nil { return nil, fmt.Errorf("cannot determine Databricks config file path: %w", err) } @@ -91,7 +91,7 @@ func GetDatabricksCfg() (*config.File, error) { } func LoadProfiles(fn ProfileMatchFunction) (file string, profiles Profiles, err error) { - f, err := GetDatabricksCfg() + f, err := Get() if err != nil { return "", nil, fmt.Errorf("cannot load Databricks config file: %w", err) }