From 5a4c90fc926015eaefc4333d469cdf4a34a6a4c4 Mon Sep 17 00:00:00 2001 From: Erik Ostien Date: Tue, 12 Nov 2024 17:06:41 -0700 Subject: [PATCH] Remove ActiveProfile struct, and add ability to get a profile's viper from the MainConfig struct. Refeactor to fix failing tests. --- cmd/config/list_profiles.go | 5 +- cmd/config/set_test.go | 2 +- cmd/config/unset_test.go | 2 +- .../commands/config/list_profiles_internal.go | 14 +- .../config/list_profiles_internal_test.go | 4 +- internal/commands/config/set_internal.go | 5 +- internal/commands/config/unset_internal.go | 5 +- internal/commands/request/request_internal.go | 17 +- internal/profiles/validate.go | 21 ++- internal/profiles/viper.go | 176 +++++++++++------- 10 files changed, 155 insertions(+), 96 deletions(-) diff --git a/cmd/config/list_profiles.go b/cmd/config/list_profiles.go index 7a6dad7..fd48ce4 100644 --- a/cmd/config/list_profiles.go +++ b/cmd/config/list_profiles.go @@ -30,7 +30,10 @@ func configListProfilesRunE(cmd *cobra.Command, args []string) error { l := logger.Get() l.Debug().Msgf("Config list-profiles Subcommand Called.") - config_internal.RunInternalConfigListProfiles() + err := config_internal.RunInternalConfigListProfiles() + if err != nil { + return err + } return nil } diff --git a/cmd/config/set_test.go b/cmd/config/set_test.go index 18c9e8b..60e1472 100644 --- a/cmd/config/set_test.go +++ b/cmd/config/set_test.go @@ -60,7 +60,7 @@ func TestConfigSetCmd_CheckViperConfig(t *testing.T) { testutils.CheckExpectedError(t, err, nil) mainViper := profiles.GetMainConfig().ViperInstance() - profileViperKey := profiles.GetMainConfig().ActiveProfile().Name() + "." + viperKey + profileViperKey := "default." + viperKey viperNewValue := mainViper.GetString(profileViperKey) if viperNewValue != viperNewUUID { diff --git a/cmd/config/unset_test.go b/cmd/config/unset_test.go index 24d13eb..cdbe716 100644 --- a/cmd/config/unset_test.go +++ b/cmd/config/unset_test.go @@ -46,7 +46,7 @@ func TestConfigUnsetCmd_CheckViperConfig(t *testing.T) { testutils.CheckExpectedError(t, err, nil) mainViper := profiles.GetMainConfig().ViperInstance() - profileViperKey := profiles.GetMainConfig().ActiveProfile().Name() + "." + viperKey + profileViperKey := "default." + viperKey viperNewValue := mainViper.GetString(profileViperKey) if viperOldValue == viperNewValue { t.Errorf("Expected viper configuration value to be updated. Old: %s, New: %s", viperOldValue, viperNewValue) diff --git a/internal/commands/config/list_profiles_internal.go b/internal/commands/config/list_profiles_internal.go index 3a768ad..164f828 100644 --- a/internal/commands/config/list_profiles_internal.go +++ b/internal/commands/config/list_profiles_internal.go @@ -1,17 +1,23 @@ package config_internal import ( + "strings" + "github.com/fatih/color" + "github.com/pingidentity/pingcli/internal/configuration/options" "github.com/pingidentity/pingcli/internal/logger" "github.com/pingidentity/pingcli/internal/output" "github.com/pingidentity/pingcli/internal/profiles" ) -func RunInternalConfigListProfiles() { +func RunInternalConfigListProfiles() (err error) { l := logger.Get() profileNames := profiles.GetMainConfig().ProfileNames() - activeProfile := profiles.GetMainConfig().ActiveProfile().Name() + activeProfileName, err := profiles.GetOptionValue(options.RootActiveProfileOption) + if err != nil { + return err + } listStr := "Profiles:\n" @@ -20,7 +26,7 @@ func RunInternalConfigListProfiles() { activeFmt := color.New(color.Bold, color.FgGreen).SprintFunc() for _, profileName := range profileNames { - if profileName == activeProfile { + if strings.EqualFold(profileName, activeProfileName) { listStr += "- " + profileName + activeFmt(" (active)") + " \n" } else { listStr += "- " + profileName + "\n" @@ -38,4 +44,6 @@ func RunInternalConfigListProfiles() { } output.Message(listStr, nil) + + return nil } diff --git a/internal/commands/config/list_profiles_internal_test.go b/internal/commands/config/list_profiles_internal_test.go index d7b67ca..6f214b1 100644 --- a/internal/commands/config/list_profiles_internal_test.go +++ b/internal/commands/config/list_profiles_internal_test.go @@ -3,6 +3,7 @@ package config_internal import ( "testing" + "github.com/pingidentity/pingcli/internal/testing/testutils" "github.com/pingidentity/pingcli/internal/testing/testutils_viper" ) @@ -10,5 +11,6 @@ import ( func Test_RunInternalConfigListProfiles(t *testing.T) { testutils_viper.InitVipers(t) - RunInternalConfigListProfiles() + err := RunInternalConfigListProfiles() + testutils.CheckExpectedError(t, err, nil) } diff --git a/internal/commands/config/set_internal.go b/internal/commands/config/set_internal.go index cbf5214..4c6e4a7 100644 --- a/internal/commands/config/set_internal.go +++ b/internal/commands/config/set_internal.go @@ -27,12 +27,11 @@ func RunInternalConfigSet(kvPair string) (err error) { return fmt.Errorf("failed to set configuration: value for key '%s' is empty. Use 'pingcli config unset %s' to unset the key", vKey, vKey) } - if err = profiles.GetMainConfig().ValidateExistingProfileName(pName); err != nil { + subViper, err := profiles.GetMainConfig().GetProfileViper(pName) + if err != nil { return fmt.Errorf("failed to set configuration: %v", err) } - subViper := profiles.GetMainConfig().ViperInstance().Sub(pName) - opt, err := configuration.OptionFromViperKey(vKey) if err != nil { return fmt.Errorf("failed to set configuration: %v", err) diff --git a/internal/commands/config/unset_internal.go b/internal/commands/config/unset_internal.go index 5242acb..1fe4ee7 100644 --- a/internal/commands/config/unset_internal.go +++ b/internal/commands/config/unset_internal.go @@ -19,12 +19,11 @@ func RunInternalConfigUnset(viperKey string) (err error) { return fmt.Errorf("failed to unset configuration: %v", err) } - if err = profiles.GetMainConfig().ValidateExistingProfileName(pName); err != nil { + subViper, err := profiles.GetMainConfig().GetProfileViper(pName) + if err != nil { return fmt.Errorf("failed to unset configuration: %v", err) } - subViper := profiles.GetMainConfig().ViperInstance().Sub(pName) - opt, err := configuration.OptionFromViperKey(viperKey) if err != nil { return fmt.Errorf("failed to unset configuration: %v", err) diff --git a/internal/commands/request/request_internal.go b/internal/commands/request/request_internal.go index 97c3718..b39662c 100644 --- a/internal/commands/request/request_internal.go +++ b/internal/commands/request/request_internal.go @@ -252,13 +252,20 @@ func pingoneAuth() (accessToken string, err error) { } if pName == "" { - pName = profiles.GetMainConfig().ActiveProfile().Name() + pName, err = profiles.GetOptionValue(options.RootActiveProfileOption) + if err != nil { + return "", err + } + } + + subViper, err := profiles.GetMainConfig().GetProfileViper(pName) + if err != nil { + return "", err } - profileViper := profiles.GetMainConfig().ViperInstance().Sub(pName) - profileViper.Set(options.RequestAccessTokenOption.ViperKey, pingoneAuthResponse.AccessToken) - profileViper.Set(options.RequestAccessTokenExpiryOption.ViperKey, tokenExpiry) - err = profiles.GetMainConfig().SaveProfile(pName, profileViper) + subViper.Set(options.RequestAccessTokenOption.ViperKey, pingoneAuthResponse.AccessToken) + subViper.Set(options.RequestAccessTokenExpiryOption.ViperKey, tokenExpiry) + err = profiles.GetMainConfig().SaveProfile(pName, subViper) if err != nil { return "", err } diff --git a/internal/profiles/validate.go b/internal/profiles/validate.go index 4d56bab..4357419 100644 --- a/internal/profiles/validate.go +++ b/internal/profiles/validate.go @@ -11,24 +11,31 @@ import ( "github.com/spf13/viper" ) -func Validate() error { +func Validate() (err error) { // Get a slice of all profile names configured in the config.yaml file profileNames := GetMainConfig().ProfileNames() // Validate profile names - if err := validateProfileNames(profileNames); err != nil { + if err = validateProfileNames(profileNames); err != nil { return err } // Make sure selected active profile is in the configuration file - activeProfile := GetMainConfig().ActiveProfile().Name() - if !slices.Contains(profileNames, activeProfile) { - return fmt.Errorf("failed to validate Ping CLI configuration: active profile '%s' not found in configuration file %s", activeProfile, GetMainConfig().ViperInstance().ConfigFileUsed()) + activeProfileName, err := GetOptionValue(options.RootActiveProfileOption) + if err != nil { + return fmt.Errorf("failed to validate Ping CLI configuration: %v", err) + } + if !slices.Contains(profileNames, activeProfileName) { + return fmt.Errorf("failed to validate Ping CLI configuration: active profile '%s' not found in configuration "+ + "file %s", activeProfileName, GetMainConfig().ViperInstance().ConfigFileUsed()) } - // for each profile key, set the profile based on mainViper.Sub() and validate the profile + // for each profile key, validate the profile viper for _, pName := range profileNames { - subViper := GetMainConfig().ViperInstance().Sub(pName) + subViper, err := GetMainConfig().GetProfileViper(pName) + if err != nil { + return fmt.Errorf("failed to validate Ping CLI configuration: %v", err) + } if err := validateProfileKeys(pName, subViper); err != nil { return fmt.Errorf("failed to validate Ping CLI configuration: %v", err) diff --git a/internal/profiles/viper.go b/internal/profiles/viper.go index 6cf20d4..6a5dba1 100644 --- a/internal/profiles/viper.go +++ b/internal/profiles/viper.go @@ -22,14 +22,8 @@ import ( "gopkg.in/yaml.v3" ) -type ActiveProfile struct { - name string - viperInstance *viper.Viper -} - type MainConfig struct { viperInstance *viper.Viper - activeProfile *ActiveProfile } var ( @@ -40,7 +34,6 @@ var ( func NewMainConfig() (newMainViper *MainConfig) { newMainViper = &MainConfig{ viperInstance: viper.New(), - activeProfile: nil, } return newMainViper @@ -55,10 +48,6 @@ func (m MainConfig) ViperInstance() *viper.Viper { return m.viperInstance } -func (m MainConfig) ActiveProfile() *ActiveProfile { - return m.activeProfile -} - func (m *MainConfig) ChangeActiveProfile(pName string) (err error) { if err = m.ValidateExistingProfileName(pName); err != nil { return err @@ -80,11 +69,6 @@ func (m *MainConfig) ChangeActiveProfile(pName string) (err error) { return err } - m.activeProfile = &ActiveProfile{ - name: pName, - viperInstance: m.ViperInstance().Sub(pName), - } - return nil } @@ -103,7 +87,10 @@ func (m MainConfig) ChangeProfileName(oldPName, newPName string) (err error) { return err } - subViper := m.ViperInstance().Sub(oldPName) + subViper, err := m.GetProfileViper(oldPName) + if err != nil { + return err + } if err = m.DeleteProfile(oldPName); err != nil { return err @@ -121,7 +108,11 @@ func (m MainConfig) ChangeProfileDescription(pName, description string) (err err return err } - subViper := m.ViperInstance().Sub(pName) + subViper, err := m.GetProfileViper(pName) + if err != nil { + return err + } + subViper.Set(options.ProfileDescriptionOption.ViperKey, description) if err = m.SaveProfile(pName, subViper); err != nil { @@ -131,6 +122,19 @@ func (m MainConfig) ChangeProfileDescription(pName, description string) (err err return nil } +func (m MainConfig) GetProfileViper(pName string) (subViper *viper.Viper, err error) { + if err = m.ValidateExistingProfileName(pName); err != nil { + return nil, err + } + + subViper = m.ViperInstance().Sub(pName) + if subViper == nil { + return nil, fmt.Errorf("failed to get profile viper: profile '%s' does not exist", pName) + } + + return subViper, nil +} + // Viper gives no built-in delete or unset method for keys // Using this "workaround" described here: https://github.com/spf13/viper/issues/632 func (m MainConfig) DeleteProfile(pName string) (err error) { @@ -138,7 +142,12 @@ func (m MainConfig) DeleteProfile(pName string) (err error) { return err } - if pName == m.ActiveProfile().Name() { + activeProfileName, err := GetOptionValue(options.RootActiveProfileOption) + if err != nil { + return err + } + + if strings.EqualFold(activeProfileName, pName) { return fmt.Errorf("'%s' is the active profile and cannot be deleted", pName) } @@ -278,7 +287,10 @@ func (m MainConfig) ProfileToString(pName string) (yamlStr string, err error) { return "", err } - subViper := m.ViperInstance().Sub(pName) + subViper, err := m.GetProfileViper(pName) + if err != nil { + return "", err + } yaml, err := yaml.Marshal(subViper.AllSettings()) if err != nil { @@ -293,7 +305,10 @@ func (m MainConfig) ProfileViperValue(pName, viperKey string) (yamlStr string, e return "", err } - subViper := m.ViperInstance().Sub(pName) + subViper, err := m.GetProfileViper(pName) + if err != nil { + return "", err + } if !subViper.IsSet(viperKey) { return "", fmt.Errorf("configuration key '%s' is not set in profile '%s'", viperKey, pName) @@ -310,7 +325,11 @@ func (m MainConfig) ProfileViperValue(pName, viperKey string) (yamlStr string, e func (m MainConfig) DefaultMissingViperKeys() (err error) { // For each profile, if a viper key from an option doesn't exist, set it to the default value for _, pName := range m.ProfileNames() { - subViper := m.ViperInstance().Sub(pName) + subViper, err := m.GetProfileViper(pName) + if err != nil { + return err + } + for _, opt := range options.Options() { if opt.ViperKey == "" || opt.ViperKey == options.RootActiveProfileOption.ViperKey { continue @@ -319,7 +338,7 @@ func (m MainConfig) DefaultMissingViperKeys() (err error) { subViper.Set(opt.ViperKey, opt.DefaultValue) } } - err := m.SaveProfile(pName, subViper) + err = m.SaveProfile(pName, subViper) if err != nil { return fmt.Errorf("Failed to save profile '%s': %v", pName, err) } @@ -328,83 +347,98 @@ func (m MainConfig) DefaultMissingViperKeys() (err error) { return nil } -func (a ActiveProfile) ViperInstance() *viper.Viper { - return a.viperInstance -} - -func (a ActiveProfile) Name() string { - return a.name -} - func GetOptionValue(opt options.Option) (pFlagValue string, err error) { - if opt.CobraParamValue != nil && opt.Flag.Changed { - pFlagValue = opt.CobraParamValue.String() - return pFlagValue, nil + // 1st priority: cobra param flag value + cobraParamValue, ok := cobraParamValueFromOption(opt) + if ok { + return cobraParamValue, nil } + // 2nd priority: environment variable value pFlagValue = os.Getenv(opt.EnvVar) if pFlagValue != "" { return pFlagValue, nil } - mainConfig := GetMainConfig() - if opt.ViperKey != "" && mainConfig != nil { - var vValue any + // 3rd priority: viper value + viperValue, ok, err := viperValueFromOption(opt) + if err != nil { + return "", err + } + if ok { + return viperValue, nil + } - mainViperInstance := mainConfig.ViperInstance() - // This recursive call is safe, as options.RootProfileOption.ViperKey is not set - definedProfileName, err := GetOptionValue(options.RootProfileOption) - if err != nil { - return "", err - } + // 4th priority: default value + if opt.DefaultValue != nil { + pFlagValue = opt.DefaultValue.String() + return pFlagValue, nil + } - // 3 Cases: - // - 1) Viper Key is the ActiveProfile Key, get value from main viper instance - // - 2) --profile flag has been set, get value from set profile viper instance - // - 3) no --profile flag set, get value from active profile viper instance defined in main viper instance + // This is a error, as it means the option is not configured internally to contain one of the 4 values above. + // This should never happen, as all options should at least have a default value. + return "", fmt.Errorf("failed to get option value: no value found: %v", opt) +} +func cobraParamValueFromOption(opt options.Option) (value string, ok bool) { + if opt.CobraParamValue != nil && opt.Flag.Changed { + return opt.CobraParamValue.String(), true + } + + return "", false +} + +func viperValueFromOption(opt options.Option) (value string, ok bool, err error) { + mainConfig := GetMainConfig() + if opt.ViperKey != "" && mainConfig != nil { + var ( + vValue any + mainViperInstance = mainConfig.ViperInstance() + ) + + // Case 1: Viper Key is the ActiveProfile Key, get value from main viper instance if opt.ViperKey == options.RootActiveProfileOption.ViperKey && mainViperInstance != nil { - // Case 1 vValue = mainViperInstance.Get(opt.ViperKey) - } else if definedProfileName != "" { - // Case 2 - profileViperInstance := mainViperInstance.Sub(definedProfileName) - if profileViperInstance != nil { - vValue = profileViperInstance.Get(opt.ViperKey) - } } else { - // Case 3 - activeProfile := mainConfig.ActiveProfile() - if activeProfile != nil { - profileViperInstance := activeProfile.ViperInstance() - if profileViperInstance != nil { - vValue = profileViperInstance.Get(opt.ViperKey) + // Case 2: --profile flag has been set, get value from set profile viper instance + // Case 3: no --profile flag set, get value from active profile viper instance defined in main viper instance + // This recursive call is safe, as options.RootProfileOption.ViperKey is not set + pName, err := GetOptionValue(options.RootProfileOption) + if err != nil { + return "", false, err + } + if pName == "" { + pName, err = GetOptionValue(options.RootActiveProfileOption) + if err != nil { + return "", false, err } } + + subViper, err := mainConfig.GetProfileViper(pName) + if err != nil { + return "", false, err + } + + vValue = subViper.Get(opt.ViperKey) } switch typedValue := vValue.(type) { case nil: - // Do nothing + return "", false, nil case string: - return typedValue, nil + return typedValue, true, nil case []string: - return strings.Join(typedValue, ","), nil + return strings.Join(typedValue, ","), true, nil case []any: strSlice := []string{} for _, v := range typedValue { strSlice = append(strSlice, fmt.Sprintf("%v", v)) } - return strings.Join(strSlice, ","), nil + return strings.Join(strSlice, ","), true, nil default: - return fmt.Sprintf("%v", typedValue), nil + return fmt.Sprintf("%v", typedValue), true, nil } } - if opt.DefaultValue != nil { - pFlagValue = opt.DefaultValue.String() - return pFlagValue, nil - } - - return pFlagValue, fmt.Errorf("failed to get option value: no value found: %v", opt) + return "", false, nil }