From c89b4e8d72f721393ed4b20c50b248a7fb061751 Mon Sep 17 00:00:00 2001 From: Kai Kummerer Date: Thu, 11 Apr 2024 20:51:03 +0200 Subject: [PATCH 1/4] Change config handling: only create config file & directory if needed --- internal/cmd/config/list/list.go | 7 +---- internal/cmd/config/set/set.go | 4 +-- internal/cmd/config/unset/unset.go | 4 +-- internal/pkg/auth/storage.go | 2 +- internal/pkg/config/config.go | 39 +++++++++++++----------- internal/pkg/projectname/project_name.go | 2 +- 6 files changed, 28 insertions(+), 30 deletions(-) diff --git a/internal/cmd/config/list/list.go b/internal/cmd/config/list/list.go index b6bdf48e2..0f6974703 100644 --- a/internal/cmd/config/list/list.go +++ b/internal/cmd/config/list/list.go @@ -37,11 +37,6 @@ func NewCmd(p *print.Printer) *cobra.Command { "$ stackit config list"), ), RunE: func(cmd *cobra.Command, args []string) error { - err := viper.ReadInConfig() - if err != nil { - return fmt.Errorf("read config file: %w", err) - } - configData := viper.AllSettings() // Sort the config options by key @@ -84,7 +79,7 @@ func NewCmd(p *print.Printer) *cobra.Command { table.AddRow(key, valueString) table.AddSeparator() } - err = table.Display(p) + err := table.Display(p) if err != nil { return fmt.Errorf("render table: %w", err) } diff --git a/internal/cmd/config/set/set.go b/internal/cmd/config/set/set.go index 36e9c06d4..fd85eebe7 100644 --- a/internal/cmd/config/set/set.go +++ b/internal/cmd/config/set/set.go @@ -80,9 +80,9 @@ func NewCmd(p *print.Printer) *cobra.Command { viper.Set(config.ProjectNameKey, "") } - err = viper.WriteConfig() + err = config.Write() if err != nil { - return fmt.Errorf("write new config to file: %w", err) + return fmt.Errorf("writing config file: %w", err) } return nil }, diff --git a/internal/cmd/config/unset/unset.go b/internal/cmd/config/unset/unset.go index ff4e4d665..dbf0495f5 100644 --- a/internal/cmd/config/unset/unset.go +++ b/internal/cmd/config/unset/unset.go @@ -144,9 +144,9 @@ func NewCmd() *cobra.Command { viper.Set(config.SKECustomEndpointKey, "") } - err := viper.WriteConfig() + err := config.Write() if err != nil { - return fmt.Errorf("write updated config to file: %w", err) + return fmt.Errorf("writing config file: %w", err) } return nil }, diff --git a/internal/pkg/auth/storage.go b/internal/pkg/auth/storage.go index 3a2031fff..c3755e6e9 100644 --- a/internal/pkg/auth/storage.go +++ b/internal/pkg/auth/storage.go @@ -136,7 +136,7 @@ func GetAuthField(key authFieldKey) (string, error) { var errFallback error value, errFallback = getAuthFieldFromEncodedTextFile(key) if errFallback != nil { - return "", fmt.Errorf("write to keyring failed (%w), tried write to encoded text file: %w", err, errFallback) + return "", fmt.Errorf("read from keyring failed (%w), read from encoded file also failed: %w", err, errFallback) } } return value, nil diff --git a/internal/pkg/config/config.go b/internal/pkg/config/config.go index 08eac0b98..fefaca21c 100644 --- a/internal/pkg/config/config.go +++ b/internal/pkg/config/config.go @@ -1,6 +1,7 @@ package config import ( + "errors" "fmt" "os" "path/filepath" @@ -70,6 +71,8 @@ var ConfigKeys = []string{ SKECustomEndpointKey, } +var FolderPath string + func InitConfig() { home, err := os.UserHomeDir() cobra.CheckErr(err) @@ -80,23 +83,28 @@ func InitConfig() { viper.SetConfigType(configFileExtension) viper.AddConfigPath(configFolderPath) - err = createFolderIfNotExists(configFolderPath) - cobra.CheckErr(err) - err = createFileIfNotExists(configFilePath) - cobra.CheckErr(err) + // Write config dir path to global variable + FolderPath = configFolderPath err = viper.ReadInConfig() - cobra.CheckErr(err) + if !errors.As(err, &viper.ConfigFileNotFoundError{}) { + cobra.CheckErr(err) + } + + // This hack is required to allow creating the config file with `viper.WriteConfig` + // see https://github.com/spf13/viper/issues/851#issuecomment-789393451 + viper.SetConfigFile(configFilePath) + setConfigDefaults() viper.AutomaticEnv() viper.SetEnvPrefix("stackit") } -func createFolderIfNotExists(folderPath string) error { - _, err := os.Stat(folderPath) +func CreateFolderIfNotExists() error { + _, err := os.Stat(FolderPath) if os.IsNotExist(err) { - err := os.MkdirAll(folderPath, os.ModePerm) + err := os.MkdirAll(FolderPath, os.ModePerm) if err != nil { return err } @@ -106,17 +114,12 @@ func createFolderIfNotExists(folderPath string) error { return nil } -func createFileIfNotExists(filePath string) error { - _, err := os.Stat(filePath) - if os.IsNotExist(err) { - err := viper.SafeWriteConfigAs(filePath) - if err != nil { - return err - } - } else if err != nil { - return err +// Write saves the config file (wrapping `viper.WriteConfig`) and ensures that its directory exists +func Write() error { + if err := CreateFolderIfNotExists(); err != nil { + return fmt.Errorf("create config directory: %w", err) } - return nil + return viper.WriteConfig() } // All config keys should be set to a default value so that they can be set as an environment variable diff --git a/internal/pkg/projectname/project_name.go b/internal/pkg/projectname/project_name.go index d2a54ff0a..9f80da8ab 100644 --- a/internal/pkg/projectname/project_name.go +++ b/internal/pkg/projectname/project_name.go @@ -43,7 +43,7 @@ func GetProjectName(ctx context.Context, cmd *cobra.Command, p *print.Printer) ( // (So next time we can just pull it from there) if !isProjectIdSetInFlags(cmd) { viper.Set(config.ProjectNameKey, projectName) - err = viper.WriteConfig() + err = config.Write() if err != nil { return "", fmt.Errorf("write new config to file: %w", err) } From 10ece6f9ce68d27abc03bcfb248d141d058f7a65 Mon Sep 17 00:00:00 2001 From: Kai Kummerer Date: Mon, 15 Apr 2024 16:03:18 +0200 Subject: [PATCH 2/4] Changes from code review --- internal/cmd/config/set/set.go | 2 +- internal/cmd/config/unset/unset.go | 2 +- internal/pkg/auth/storage.go | 2 +- internal/pkg/config/config.go | 22 ++++++++++------------ 4 files changed, 13 insertions(+), 15 deletions(-) diff --git a/internal/cmd/config/set/set.go b/internal/cmd/config/set/set.go index fd85eebe7..459f3a3b4 100644 --- a/internal/cmd/config/set/set.go +++ b/internal/cmd/config/set/set.go @@ -82,7 +82,7 @@ func NewCmd(p *print.Printer) *cobra.Command { err = config.Write() if err != nil { - return fmt.Errorf("writing config file: %w", err) + return fmt.Errorf("write config to file: %w", err) } return nil }, diff --git a/internal/cmd/config/unset/unset.go b/internal/cmd/config/unset/unset.go index dbf0495f5..7f6bc8449 100644 --- a/internal/cmd/config/unset/unset.go +++ b/internal/cmd/config/unset/unset.go @@ -146,7 +146,7 @@ func NewCmd() *cobra.Command { err := config.Write() if err != nil { - return fmt.Errorf("writing config file: %w", err) + return fmt.Errorf("write config to file: %w", err) } return nil }, diff --git a/internal/pkg/auth/storage.go b/internal/pkg/auth/storage.go index c3755e6e9..1bcb6e784 100644 --- a/internal/pkg/auth/storage.go +++ b/internal/pkg/auth/storage.go @@ -136,7 +136,7 @@ func GetAuthField(key authFieldKey) (string, error) { var errFallback error value, errFallback = getAuthFieldFromEncodedTextFile(key) if errFallback != nil { - return "", fmt.Errorf("read from keyring failed (%w), read from encoded file also failed: %w", err, errFallback) + return "", fmt.Errorf("read from keyring: %w, read from encoded file as fallback: %w", err, errFallback) } } return value, nil diff --git a/internal/pkg/config/config.go b/internal/pkg/config/config.go index fefaca21c..59eebbd7e 100644 --- a/internal/pkg/config/config.go +++ b/internal/pkg/config/config.go @@ -71,7 +71,7 @@ var ConfigKeys = []string{ SKECustomEndpointKey, } -var FolderPath string +var folderPath string func InitConfig() { home, err := os.UserHomeDir() @@ -80,31 +80,29 @@ func InitConfig() { configFilePath := filepath.Join(configFolderPath, fmt.Sprintf("%s.%s", configFileName, configFileExtension)) viper.SetConfigName(configFileName) - viper.SetConfigType(configFileExtension) - viper.AddConfigPath(configFolderPath) // Write config dir path to global variable - FolderPath = configFolderPath + folderPath = configFolderPath + + // This hack is required to allow creating the config file with `viper.WriteConfig` + // see https://github.com/spf13/viper/issues/851#issuecomment-789393451 + viper.SetConfigFile(configFilePath) err = viper.ReadInConfig() if !errors.As(err, &viper.ConfigFileNotFoundError{}) { cobra.CheckErr(err) } - // This hack is required to allow creating the config file with `viper.WriteConfig` - // see https://github.com/spf13/viper/issues/851#issuecomment-789393451 - viper.SetConfigFile(configFilePath) - setConfigDefaults() viper.AutomaticEnv() viper.SetEnvPrefix("stackit") } -func CreateFolderIfNotExists() error { - _, err := os.Stat(FolderPath) +func createFolderIfNotExists() error { + _, err := os.Stat(folderPath) if os.IsNotExist(err) { - err := os.MkdirAll(FolderPath, os.ModePerm) + err := os.MkdirAll(folderPath, os.ModePerm) if err != nil { return err } @@ -116,7 +114,7 @@ func CreateFolderIfNotExists() error { // Write saves the config file (wrapping `viper.WriteConfig`) and ensures that its directory exists func Write() error { - if err := CreateFolderIfNotExists(); err != nil { + if err := createFolderIfNotExists(); err != nil { return fmt.Errorf("create config directory: %w", err) } return viper.WriteConfig() From db8f4ad91b3011d8b401e13b2f8058baac60f28a Mon Sep 17 00:00:00 2001 From: Kai Kummerer Date: Tue, 16 Apr 2024 10:07:45 +0200 Subject: [PATCH 3/4] Fix config read --- internal/pkg/config/config.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/internal/pkg/config/config.go b/internal/pkg/config/config.go index 59eebbd7e..190f64edb 100644 --- a/internal/pkg/config/config.go +++ b/internal/pkg/config/config.go @@ -1,7 +1,6 @@ package config import ( - "errors" "fmt" "os" "path/filepath" @@ -79,8 +78,6 @@ func InitConfig() { configFolderPath := filepath.Join(home, configFolder) configFilePath := filepath.Join(configFolderPath, fmt.Sprintf("%s.%s", configFileName, configFileExtension)) - viper.SetConfigName(configFileName) - // Write config dir path to global variable folderPath = configFolderPath @@ -88,10 +85,13 @@ func InitConfig() { // see https://github.com/spf13/viper/issues/851#issuecomment-789393451 viper.SetConfigFile(configFilePath) - err = viper.ReadInConfig() - if !errors.As(err, &viper.ConfigFileNotFoundError{}) { - cobra.CheckErr(err) + f, err := os.Open(configFilePath) + if !os.IsNotExist(err) { + if err := viper.ReadConfig(f); err != nil { + cobra.CheckErr(err) + } } + defer f.Close() //nolint:errcheck // file close errors are hard to handle setConfigDefaults() From 384b9f7cc16903775170a3eaf0772f54a03dc92a Mon Sep 17 00:00:00 2001 From: Kai Kummerer Date: Tue, 16 Apr 2024 10:57:21 +0200 Subject: [PATCH 4/4] correctly handle f.Close error --- internal/pkg/config/config.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/internal/pkg/config/config.go b/internal/pkg/config/config.go index 190f64edb..4077c1d3a 100644 --- a/internal/pkg/config/config.go +++ b/internal/pkg/config/config.go @@ -91,7 +91,13 @@ func InitConfig() { cobra.CheckErr(err) } } - defer f.Close() //nolint:errcheck // file close errors are hard to handle + defer func() { + if f != nil { + if err := f.Close(); err != nil { + cobra.CheckErr(err) + } + } + }() setConfigDefaults()