fix: return the config file not found error (#5972)

* return the file not found error Signed-off-by: terry.hung <terryhung1228@gmail.com> * add the new line at the end of file and fix the lint error Signed-off-by: terry.hung <terryhung1228@gmail.com> * fix: remove the duplicated test case for updateConfig Signed-off-by: terry.hung <terryhung1228@gmail.com> * fix: remove useless code and fix the lint Signed-off-by: terry.hung <terryhung1228@gmail.com> * remove useless file Signed-off-by: terry.hung <terryhung1228@gmail.com> * feat: add the unit test for viper Signed-off-by: terry.hung <terryhung1228@gmail.com> * fix: lint error (gci) Signed-off-by: terry.hung <terryhung1228@gmail.com> --------- Signed-off-by: terry.hung <terryhung1228@gmail.com>
flyteorg · Nov 18, 2024 · d2ddfe2 · d2ddfe2
1 parent 57f208a
commit d2ddfe2
Show file tree

Hide file tree

Showing 5 changed files with 51 additions and 21 deletions.
diff --git 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
@@ -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
@@ -0,0 +1,2 @@
+test:
+  field: 1
diff --git 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
@@ -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)
+	})
+}