[BUG] New versions of viper breaks config loading
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](https://github.com/flyteorg/flyte/blob/master/flytestdlib/config/viper/viper.go#L303). 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
<img width="508" alt="v1 11 0" src="https://github.com/flyteorg/flyte/assets/1274471/61feb4a4-d3ee-4297-8232-5e38a7250290">
v.1.18.2
<img width="507" alt="v1 18 2" src="https://github.com/flyteorg/flyte/assets/1274471/30f36d9e-38b5-4bc1-8786-e60ab870ae3c">
### Are you sure this issue hasn't been raised already?
- [X] Yes
### Have you read the Code of Conduct?
- [X] Yes
Thank you for opening your first issue here! 🛠
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.
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
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.
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: https://github.com/flyteorg/flyte/commit/75e81706e29832075c012782bc1cdd704e165b5b
I'm curious to see what you did..
I just run the go get github.com/spf13/[email protected].
Then this is the diff after upgrading the viper: https://github.com/Terryhung/flyte/commit/6a440cb57d3509eed8d8bf80da6563f6f9eeb6bb.
I found the viper version in your diff is: v1.19.0
I just run the
go get github.com/spf13/[email protected].Then this is the diff after upgrading the viper: https://github.com/Terryhung/flyte/commit/6a440cb57d3509eed8d8bf80da6563f6f9eeb6bb.
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.
@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....
"Hello 👋, this issue has been inactive for over 90 days. To help maintain a clean and focused backlog, we'll be marking this issue as stale and will close the issue if we detect no activity in the next 7 days. Thank you for your contribution and understanding! 🙏"
Hello 👋, this issue has been inactive for over 90 days and hasn't received any updates since it was marked as stale. We'll be closing this issue for now, but if you believe this issue is still relevant, please feel free to reopen it. Thank you for your contribution and understanding! 🙏