From d2ddfe26041c4441139001c289c9c0e10bd3d319 Mon Sep 17 00:00:00 2001 From: "terry.hung" Date: Mon, 18 Nov 2024 09:59:58 +0800 Subject: [PATCH] fix: return the config file not found error (#5972) * return the file not found error Signed-off-by: terry.hung * add the new line at the end of file and fix the lint error Signed-off-by: terry.hung * fix: remove the duplicated test case for updateConfig Signed-off-by: terry.hung * fix: remove useless code and fix the lint Signed-off-by: terry.hung * remove useless file Signed-off-by: terry.hung * feat: add the unit test for viper Signed-off-by: terry.hung * fix: lint error (gci) Signed-off-by: terry.hung --------- Signed-off-by: terry.hung --- flytestdlib/config/tests/accessor_test.go | 6 ++-- flytestdlib/config/tests/config_cmd_test.go | 4 +-- .../viper/testdata/viper_test_config.yaml | 2 ++ flytestdlib/config/viper/viper.go | 30 ++++++++----------- flytestdlib/config/viper/viper_test.go | 30 +++++++++++++++++++ 5 files changed, 51 insertions(+), 21 deletions(-) create mode 100644 flytestdlib/config/viper/testdata/viper_test_config.yaml diff --git a/flytestdlib/config/tests/accessor_test.go b/flytestdlib/config/tests/accessor_test.go index 899938be64..86f01a37e5 100644 --- a/flytestdlib/config/tests/accessor_test.go +++ b/flytestdlib/config/tests/accessor_test.go @@ -431,9 +431,11 @@ func TestAccessor_UpdateConfig(t *testing.T) { key := strings.ToUpper("my-component.str3") assert.NoError(t, os.Setenv(key, "Set From Env")) defer func() { assert.NoError(t, os.Unsetenv(key)) }() - assert.NoError(t, v.UpdateConfig(context.TODO())) + err = v.UpdateConfig(context.TODO()) + assert.Error(t, err) + assert.EqualError(t, err, "Config File \"config\" Not Found in \"[]\"") r := reg.GetSection(MyComponentSectionKey).GetConfig().(*MyComponentConfig) - assert.Equal(t, "Set From Env", r.StringValue3) + assert.Equal(t, "", r.StringValue3) }) t.Run(fmt.Sprintf("[%v] Change handler", provider(config.Options{}).ID()), func(t *testing.T) { diff --git a/flytestdlib/config/tests/config_cmd_test.go b/flytestdlib/config/tests/config_cmd_test.go index 3a9d9f73ce..5049ae55d2 100644 --- a/flytestdlib/config/tests/config_cmd_test.go +++ b/flytestdlib/config/tests/config_cmd_test.go @@ -32,7 +32,7 @@ func TestDiscoverCommand(t *testing.T) { t.Run(fmt.Sprintf(testNameFormatter, provider(config.Options{}).ID(), "No config file"), func(t *testing.T) { cmd := config.NewConfigCommand(provider) output, err := executeCommand(cmd, config.CommandDiscover) - assert.NoError(t, err) + assert.Error(t, err) assert.Contains(t, output, "Couldn't find a config file.") }) @@ -57,7 +57,7 @@ func TestValidateCommand(t *testing.T) { t.Run(fmt.Sprintf(testNameFormatter, provider(config.Options{}).ID(), "No config file"), func(t *testing.T) { cmd := config.NewConfigCommand(provider) output, err := executeCommand(cmd, config.CommandValidate) - assert.NoError(t, err) + assert.Error(t, err) assert.Contains(t, output, "Couldn't find a config file.") }) diff --git a/flytestdlib/config/viper/testdata/viper_test_config.yaml b/flytestdlib/config/viper/testdata/viper_test_config.yaml new file mode 100644 index 0000000000..40902aace7 --- /dev/null +++ b/flytestdlib/config/viper/testdata/viper_test_config.yaml @@ -0,0 +1,2 @@ +test: + field: 1 \ No newline at end of file diff --git a/flytestdlib/config/viper/viper.go b/flytestdlib/config/viper/viper.go index 9ba4a552c2..712df96d20 100644 --- a/flytestdlib/config/viper/viper.go +++ b/flytestdlib/config/viper/viper.go @@ -128,38 +128,34 @@ func (v viperAccessor) updateConfig(ctx context.Context, r config.Section) error v.viper.AutomaticEnv() // read in environment variables that match - shouldWatchChanges := true // If a config file is found, read it in. if err = v.viper.ReadInConfig(); err == nil { logger.Debugf(ctx, "Using config file: %+v", v.viper.ConfigFilesUsed()) } else if asErrorCollection, ok := err.(stdLibErrs.ErrorCollection); ok { - shouldWatchChanges = false for i, e := range asErrorCollection { if _, isNotFound := errors.Cause(e).(viperLib.ConfigFileNotFoundError); isNotFound { - logger.Infof(ctx, "[%v] Couldn't find a config file [%v]. Relying on env vars and pflags.", + logger.Errorf(ctx, "[%v] Couldn't find a config file [%v]. Relying on env vars and pflags.", i, v.viper.underlying[i].ConfigFileUsed()) - } else { - return err + return e } } + return err } else if reflect.TypeOf(err) == reflect.TypeOf(viperLib.ConfigFileNotFoundError{}) { - shouldWatchChanges = false - logger.Info(ctx, "Couldn't find a config file. Relying on env vars and pflags.") + logger.Errorf(ctx, "Couldn't find a config file. Relying on env vars and pflags.") + return err } else { return err } - if shouldWatchChanges { - v.watcherInitializer.Do(func() { - // Watch config files to pick up on file changes without requiring a full application restart. - // This call must occur after *all* config paths have been added. - v.viper.OnConfigChange(func(e fsnotify.Event) { - logger.Debugf(ctx, "Got a notification change for file [%v] \n", e.Name) - v.configChangeHandler() - }) - v.viper.WatchConfig() + v.watcherInitializer.Do(func() { + // Watch config files to pick up on file changes without requiring a full application restart. + // This call must occur after *all* config paths have been added. + v.viper.OnConfigChange(func(e fsnotify.Event) { + logger.Debugf(ctx, "Got a notification change for file [%v] \n", e.Name) + v.configChangeHandler() }) - } + v.viper.WatchConfig() + }) return v.RefreshFromConfig(ctx, r, true) } diff --git a/flytestdlib/config/viper/viper_test.go b/flytestdlib/config/viper/viper_test.go index 881d4436f2..972ea0a201 100644 --- a/flytestdlib/config/viper/viper_test.go +++ b/flytestdlib/config/viper/viper_test.go @@ -1,11 +1,14 @@ package viper import ( + "context" "encoding/base64" "reflect" "testing" "github.com/stretchr/testify/assert" + + "github.com/flyteorg/flyte/flytestdlib/config" ) func Test_stringToByteArray(t *testing.T) { @@ -52,3 +55,30 @@ func Test_stringToByteArray(t *testing.T) { assert.NotEqual(t, []byte("hello"), res) }) } + +func TestViperAccessor_UpdateConfig(t *testing.T) { + ctx := context.Background() + t.Run("unable to find the config file", func(t *testing.T) { + // Create accessor + accessor := newAccessor(config.Options{ + SearchPaths: []string{".", "/etc/flyte/config", "$GOPATH/src/github.com/flyteorg/flyte"}, + StrictMode: false, + }) + + // Update config + err := accessor.updateConfig(ctx, accessor.rootConfig) + assert.EqualError(t, err, "Config File \"config\" Not Found in \"[]\"") + }) + + t.Run("find the config file", func(t *testing.T) { + // Create accessor + accessor := newAccessor(config.Options{ + SearchPaths: []string{"./testdata/viper_test_config.yaml"}, + StrictMode: false, + }) + + // Update config + err := accessor.updateConfig(ctx, accessor.rootConfig) + assert.NoError(t, err) + }) +}