Revert "Revert "Port over viper fixes from master (#7755)"" - #7781
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR (a revert-of-a-revert) updates Flyte’s Viper-based config handling to align with newer Viper behavior and restores correct decoding for case-sensitive/dotted YAML keys (notably for annotations-like maps), while updating related dependencies and tests.
Changes:
- Upgrades
github.com/spf13/vipertov1.21.0and switches mapstructure usage togithub.com/go-viper/mapstructure/v2. - Adds config post-processing to restore case-sensitive map keys inside arrays and to undo Viper’s dotted-key splitting, with unit + end-to-end tests.
- Updates module dependencies (
go.mod/go.sum) to match the new Viper/mapstructure/YAML usage.
Reviewed changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| runs/config/config_flags_test.go | Switches generated test code to use github.com/go-viper/mapstructure/v2. |
| go.mod | Updates Viper version, removes direct mitchellh/mapstructure, adds direct yaml.v3. |
| go.sum | Reflects dependency graph changes from Viper upgrade and mapstructure swap. |
| flytestdlib/config/viper/viper.go | Implements YAML re-read + restoration for case-sensitive array keys and dotted keys; updates mapstructure import. |
| flytestdlib/config/viper/viper_test.go | Adds focused unit tests for dotted-key restoration helpers. |
| flytestdlib/config/tests/testdata/dotted_keys_config.yaml | Adds YAML fixture covering dotted annotation keys and array element maps. |
| flytestdlib/config/tests/accessor_test.go | Adds end-to-end test asserting dotted keys and array element map keys decode correctly. |
Files not reviewed (1)
- runs/config/config_flags_test.go: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sovietaced
previously approved these changes
Aug 5, 2026
EngHabu
previously approved these changes
Aug 5, 2026
wild-endeavor
marked this pull request as ready for review
August 6, 2026 16:03
Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>
Sovietaced
previously approved these changes
Aug 6, 2026
wild-endeavor
enabled auto-merge (squash)
August 6, 2026 16:09
pmahindrakar-oss
previously approved these changes
Aug 6, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 7 changed files in this pull request and generated 3 comments.
Files not reviewed (1)
- runs/config/config_flags_test.go: Generated file
Suppressed comments (2)
flytestdlib/config/viper/viper.go:310
- This does an O(n) scan of
rawDatafor every key inviperData, making each map level O(n²). To keep behavior the same but avoid quadratic work for larger configs, build a single index map once per call (e.g.,lower(rawKey) -> rawKey) and use it for lookups.
for lowerKey, viperVal := range viperData {
// Find matching key in rawData (case-insensitive match)
var rawVal interface{}
for rawKey, rv := range rawData {
if strings.EqualFold(rawKey, lowerKey) {
rawVal = rv
break
}
}
flytestdlib/config/viper/viper.go:292
- The restoration routines are intended to fix key-shape/casing issues without changing effective values, but the current approach can interact with precedence (flags/env vs file). Add a regression test that sets a dotted key (and/or a value under an array subtree) in the YAML file and overrides it via env var or pflag, then asserts the override still wins after
UpdateConfig().
restoreCaseSensitiveArrayKeys(settings, rawSettings)
restoreDottedMapKeys(settings, rawSettings)
Comment on lines
+336
to
+353
| func restoreDottedMapKeys(viperData, rawData map[string]interface{}) { | ||
| for rawKey, rawVal := range rawData { | ||
| if strings.Contains(rawKey, keyDelim) { | ||
| // Drop the nested skeleton viper built from this dotted key, then | ||
| // reinsert the raw value under the original key. Lowercase the | ||
| // path because viper lowercases all keys. | ||
| lowerKey := strings.ToLower(rawKey) | ||
| pruneSplitPath(viperData, strings.Split(lowerKey, keyDelim)) | ||
| // Viper sometimes lowercases a dotted key without splitting it, | ||
| // leaving a flat lowercased duplicate that pruneSplitPath (which | ||
| // only removes split skeletons) does not touch. Drop it unless the | ||
| // lowercased spelling is itself a genuine key in the raw YAML. | ||
| if _, isRealKey := rawData[lowerKey]; lowerKey != rawKey && !isRealKey { | ||
| delete(viperData, lowerKey) | ||
| } | ||
| viperData[rawKey] = rawVal | ||
| continue | ||
| } |
Comment on lines
+301
to
+319
| func restoreCaseSensitiveArrayKeys(viperData, rawData map[string]interface{}) { | ||
| for lowerKey, viperVal := range viperData { | ||
| // Find matching key in rawData (case-insensitive match) | ||
| var rawVal interface{} | ||
| for rawKey, rv := range rawData { | ||
| if strings.EqualFold(rawKey, lowerKey) { | ||
| rawVal = rv | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if rawVal == nil { | ||
| continue | ||
| } | ||
|
|
||
| switch viperVal.(type) { | ||
| case []interface{}: | ||
| // Replace the lowercased array with the case-preserved original | ||
| viperData[lowerKey] = rawVal |
Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>
wild-endeavor
dismissed stale reviews from pmahindrakar-oss and Sovietaced
via
August 6, 2026 19:52
413924f
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 7 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- runs/config/config_flags_test.go: Generated file
Suppressed comments (3)
flytestdlib/config/viper/viper.go:326
- restoreCaseSensitiveArrayKeys
replaces an array value withrawValwithout verifyingrawValis actually a[]interface{}. IfviperValis an array but the raw value is not (due to mismatched types, merge behavior, or file format differences), this will silently mutatesettingsinto an unexpected shape. Recommend type-checkingrawValbefore assignment (only replace whenrawVal` is also a slice).
func restoreCaseSensitiveArrayKeys(viperData, rawData map[string]interface{}) {
for lowerKey, viperVal := range viperData {
// Find matching key in rawData (case-insensitive match)
var rawVal interface{}
for rawKey, rv := range rawData {
if strings.EqualFold(rawKey, lowerKey) {
rawVal = rv
break
}
}
if rawVal == nil {
continue
}
switch viperVal.(type) {
case []interface{}:
// Replace the lowercased array with the case-preserved original
viperData[lowerKey] = rawVal
case map[string]interface{}:
if rawMap, ok := rawVal.(map[string]interface{}); ok {
restoreCaseSensitiveArrayKeys(viperVal.(map[string]interface{}), rawMap)
}
}
}
}
flytestdlib/config/viper/viper.go:310
- restoreCaseSensitiveArrayKeys
does a full scan ofrawDatafor every key inviperData, making it O(n²) per map level. If config maps are large (or this runs on frequent reloads), this can become noticeable. Consider building a one-time lookup map fromstrings.ToLower(rawKey)(orEqualFold-normalized) torawValfor eachrawData` map level, then doing O(1) lookups during the walk.
for rawKey, rv := range rawData {
if strings.EqualFold(rawKey, lowerKey) {
rawVal = rv
break
}
}
flytestdlib/config/viper/viper.go:301
- The new restoration behavior is critical to config correctness, but there are no direct unit tests covering
restoreCaseSensitiveArrayKeys(only the E2E config test). Adding a focused unit test suite for this helper (e.g., nested arrays-of-maps, mixed scalar/map siblings, and “rawVal is not a slice” mismatch) would make regressions easier to catch and localize.
// restoreCaseSensitiveArrayKeys walks viperData and rawData in parallel.
// When an array value is found in viperData, it is replaced with the
// corresponding value from rawData to preserve the original key casing.
func restoreCaseSensitiveArrayKeys(viperData, rawData map[string]interface{}) {
Comment on lines
+270
to
+293
| settings := v.viper.AllSettings() | ||
|
|
||
| // Viper v1.21+ recursively lowercases all map keys, including those inside | ||
| // arrays. This breaks the array-based workaround for case-sensitive map keys | ||
| // (see: https://github.com/spf13/viper#does-viper-support-case-sensitive-keys). | ||
| // Re-read config files directly to restore case-sensitive keys within arrays. | ||
| for _, configFile := range v.viper.ConfigFilesUsed() { | ||
| if configFile == "" { | ||
| continue | ||
| } | ||
|
|
||
| data, err := os.ReadFile(configFile) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to read config file %q for case-sensitive key restoration: %w", configFile, err) | ||
| } | ||
|
|
||
| var rawSettings map[string]interface{} | ||
| if err := yaml.Unmarshal(data, &rawSettings); err != nil { | ||
| return fmt.Errorf("failed to parse config file %q for case-sensitive key restoration: %w", configFile, err) | ||
| } | ||
|
|
||
| restoreCaseSensitiveArrayKeys(settings, rawSettings) | ||
| restoreDottedMapKeys(settings, rawSettings) | ||
| } |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 7 changed files in this pull request and generated 3 comments.
Files not reviewed (1)
- runs/config/config_flags_test.go: Generated file
Suppressed comments (9)
flytestdlib/config/viper/viper.go:289
- This unconditionally re-parses each config file as YAML. If a repo/user uses JSON/TOML/HCL (all supported by Viper) this will fail and prevent config loading. Suggestion (mandatory): detect the actual config type (e.g., via file extension or Viper’s configured type) and parse accordingly, or restrict this restoration path to YAML-only config files and skip others without error.
for _, configFile := range v.viper.ConfigFilesUsed() {
if configFile == "" {
continue
}
data, err := os.ReadFile(configFile)
if err != nil {
return fmt.Errorf("failed to read config file %q for case-sensitive key restoration: %w", configFile, err)
}
var rawSettings map[string]interface{}
if err := yaml.Unmarshal(data, &rawSettings); err != nil {
return fmt.Errorf("failed to parse config file %q for case-sensitive key restoration: %w", configFile, err)
}
flytestdlib/config/viper/viper_test.go:58
- These test variables are named
viper, which is easy to confuse with the package name and the Viper library. Suggestion (optional): rename to something likeviperData,settings, orviperSettingsfor clarity.
viper := map[string]interface{}{
flytestdlib/config/viper/viper_test.go:84
- These test variables are named
viper, which is easy to confuse with the package name and the Viper library. Suggestion (optional): rename to something likeviperData,settings, orviperSettingsfor clarity.
viper := map[string]interface{}{
flytestdlib/config/viper/viper_test.go:105
- These test variables are named
viper, which is easy to confuse with the package name and the Viper library. Suggestion (optional): rename to something likeviperData,settings, orviperSettingsfor clarity.
viper := map[string]interface{}{
flytestdlib/config/viper/viper_test.go:128
- These test variables are named
viper, which is easy to confuse with the package name and the Viper library. Suggestion (optional): rename to something likeviperData,settings, orviperSettingsfor clarity.
viper := map[string]interface{}{
flytestdlib/config/viper/viper_test.go:149
- These test variables are named
viper, which is easy to confuse with the package name and the Viper library. Suggestion (optional): rename to something likeviperData,settings, orviperSettingsfor clarity.
viper := map[string]interface{}{
flytestdlib/config/viper/viper_test.go:177
- These test variables are named
viper, which is easy to confuse with the package name and the Viper library. Suggestion (optional): rename to something likeviperData,settings, orviperSettingsfor clarity.
viper := map[string]interface{}{
flytestdlib/config/viper/viper_test.go:200
- These test variables are named
viper, which is easy to confuse with the package name and the Viper library. Suggestion (optional): rename to something likeviperData,settings, orviperSettingsfor clarity.
viper := map[string]interface{}{
flytestdlib/config/viper/viper_test.go:147
- Correct 'usecases' to 'use cases' for grammar.
// This test case covers the usecases mentioned in issue #6166
Comment on lines
+291
to
+292
| restoreCaseSensitiveArrayKeys(settings, rawSettings) | ||
| restoreDottedMapKeys(settings, rawSettings) |
Comment on lines
+301
to
+326
| func restoreCaseSensitiveArrayKeys(viperData, rawData map[string]interface{}) { | ||
| for lowerKey, viperVal := range viperData { | ||
| // Find matching key in rawData (case-insensitive match) | ||
| var rawVal interface{} | ||
| for rawKey, rv := range rawData { | ||
| if strings.EqualFold(rawKey, lowerKey) { | ||
| rawVal = rv | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if rawVal == nil { | ||
| continue | ||
| } | ||
|
|
||
| switch viperVal.(type) { | ||
| case []interface{}: | ||
| // Replace the lowercased array with the case-preserved original | ||
| viperData[lowerKey] = rawVal | ||
| case map[string]interface{}: | ||
| if rawMap, ok := rawVal.(map[string]interface{}); ok { | ||
| restoreCaseSensitiveArrayKeys(viperVal.(map[string]interface{}), rawMap) | ||
| } | ||
| } | ||
| } | ||
| } |
Comment on lines
+336
to
+353
| func restoreDottedMapKeys(viperData, rawData map[string]interface{}) { | ||
| for rawKey, rawVal := range rawData { | ||
| if strings.Contains(rawKey, keyDelim) { | ||
| // Drop the nested skeleton viper built from this dotted key, then | ||
| // reinsert the raw value under the original key. Lowercase the | ||
| // path because viper lowercases all keys. | ||
| lowerKey := strings.ToLower(rawKey) | ||
| pruneSplitPath(viperData, strings.Split(lowerKey, keyDelim)) | ||
| // Viper sometimes lowercases a dotted key without splitting it, | ||
| // leaving a flat lowercased duplicate that pruneSplitPath (which | ||
| // only removes split skeletons) does not touch. Drop it unless the | ||
| // lowercased spelling is itself a genuine key in the raw YAML. | ||
| if _, isRealKey := rawData[lowerKey]; lowerKey != rawKey && !isRealKey { | ||
| delete(viperData, lowerKey) | ||
| } | ||
| viperData[rawKey] = rawVal | ||
| continue | ||
| } |
pingsutw
approved these changes
Aug 6, 2026
wild-endeavor
disabled auto-merge
August 7, 2026 00:20
pingsutw
approved these changes
Aug 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reverts #7779