Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] New versions of viper breaks config loading #5469

Open
2 tasks done
Sovietaced opened this issue Jun 11, 2024 · 8 comments
Open
2 tasks done

[BUG] New versions of viper breaks config loading #5469

Sovietaced opened this issue Jun 11, 2024 · 8 comments
Labels
bug Something isn't working help wanted Extra attention is needed housekeeping Issues that help maintain flyte and keep it tech-debt free

Comments

@Sovietaced
Copy link
Contributor

Sovietaced commented Jun 11, 2024

Describe the bug

My custom Flyte plugin has a depenency that pulls in a more recent version of viper v1.11.0 -> v1.18.2 and this appears to break config loading since flyteplugins config_load_test fails. I realize that this is not a present bug but I figured I would open this up in case anyone has an issue and in the event that Flyte eventually updates viper to a more recent version.

It appears that the latest entry in defined in a YAML map will be the only entry in the map that is parsed. For example:

=== RUN   TestLoadConfig/k8s-config-test
    config_load_test.go:30: 
        	Error Trace:	/home/jparraga/code/flyte/flyteplugins/go/tasks/config_load_test.go:30
        	Error:      	Not equal: 
        	            	expected: map[string]string{"annotationKey1":"annotationValue1", "annotationKey2":"annotationValue2"}
        	            	actual  : map[string]string{"annotationkey2":"annotationValue2"}
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1,4 +1,3 @@
        	            	-(map[string]string) (len=2) {
        	            	- (string) (len=14) "annotationKey1": (string) (len=16) "annotationValue1",
        	            	- (string) (len=14) "annotationKey2": (string) (len=16) "annotationValue2"
        	            	+(map[string]string) (len=1) {
        	            	+ (string) (len=14) "annotationkey2": (string) (len=16) "annotationValue2"
        	            	 }
        	Test:       	TestLoadConfig/k8s-config-test

It appears the error happens in decoding the custom config into the root config. From what I've seen there is a different in the typing of the configuration that is parsed by viper. Notably, map entries used to be parsed as map[interface{}]interface{} and are now parsed as map[string]interface{}. The code is bit hair but I figured I'd file a ticket before I dig deeper here.

Expected behavior

I would expect config loading to work correctly and unit tests to pass.

Additional context to reproduce

go get github.com/spf13/[email protected]

proceed to run unit tests in flyteplugins

Screenshots

v1.11.0
v1 11 0

v.1.18.2
v1 18 2

Are you sure this issue hasn't been raised already?

  • Yes

Have you read the Code of Conduct?

  • Yes
@Sovietaced Sovietaced added bug Something isn't working untriaged This issues has not yet been looked at by the Maintainers labels Jun 11, 2024
Copy link

welcome bot commented Jun 11, 2024

Thank you for opening your first issue here! 🛠

@Sovietaced
Copy link
Contributor Author

This appears to be related to some custom decoding which turns slices into maps: https://github.com/flyteorg/flyte/blob/master/flytestdlib/config/viper/viper.go#L177-L203

Fixing this includes all the key/values but unfortunately the keys are lowercased.

@Sovietaced
Copy link
Contributor Author

Instead of dumping more time into this I ended up just forcing the old version of viper.

replace github.com/spf13/viper v1.18.2 => github.com/spf13/viper v1.11.0

@Sovietaced Sovietaced changed the title [BUG] New versions of viper break config loading [BUG] New versions of viper breaks config loading Jun 12, 2024
@eapolinario eapolinario added housekeeping Issues that help maintain flyte and keep it tech-debt free help wanted Extra attention is needed and removed untriaged This issues has not yet been looked at by the Maintainers labels Jun 13, 2024
@Terryhung
Copy link
Contributor

Hi @Sovietaced, I can't reproduce the issue just after upgrading the viper version. Is there any other package had been changed?

go version: go version go1.22.8 darwin/arm64.

@Sovietaced
Copy link
Contributor Author

Hi @Sovietaced, I can't reproduce the issue just after upgrading the viper version. Is there any other package had been changed?

go version: go version go1.22.8 darwin/arm64.

Here is my diff, which fails: 75e8170

I'm curious to see what you did..

@Terryhung
Copy link
Contributor

I just run the go get github.com/spf13/[email protected].

Then this is the diff after upgrading the viper: Terryhung@6a440cb.

I found the viper version in your diff is: v1.19.0

@Sovietaced
Copy link
Contributor Author

Sovietaced commented Nov 12, 2024

I just run the go get github.com/spf13/[email protected].

Then this is the diff after upgrading the viper: Terryhung@6a440cb.

I found the viper version in your diff is: v1.19.0

I did 1.18.2 as well. The issue is you're updating the global go mod but flyteplugins has its own go mod, and that's where the failing unit test is.

@Terryhung
Copy link
Contributor

@Sovietaced After reviewing the code in viper, I found that it case insensitive problem can't be fix in flyte.

The code in viper.

Moreover, viper already has an old issue....

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed housekeeping Issues that help maintain flyte and keep it tech-debt free
Projects
None yet
Development

No branches or pull requests

3 participants