Correctly match environment variables to YAML-inlined structs in configuration #4639
+79
−0
Conversation
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
Signed-off-by: evanebb <git@evanus.nl>
milosgajdos
approved these changes
May 21, 2025
Signed-off-by: evanebb <git@evanus.nl>
|
@milosgajdos I've added a test to the configuration package in b65ccca. |
|
PTAL @thaJeztah |
thaJeztah
reviewed
May 23, 2025
There was a problem hiding this comment.
Left some suggestions; here's a quick write-up of them combined if you want to copy/paste;
// For fields i
8000
nlined in the YAML configuration file, the environment variables also need to be inlined
// Example struct tag for inlined field: `yaml:",inline"`
_, yamlOpts, _ := strings.Cut(sf.Tag.Get("yaml"), ",")
if yamlOpts == "inline" && sf.Type.Kind() == reflect.Struct {
// Inlined struct, check whether the env variable corresponds to a field inside this struct
// Maps could also be inlined, but since we don't need it right now it is not supported
inlined := v.Field(i)
for j := range inlined.NumField() {
if strings.EqualFold(inlined.Type().Field(j).Name, path[0]) {
return p.overwriteFields(inlined, fullpath, path, payload)
}
}
}
Comment on lines
+212
to
+217
| yamlTag := sf.Tag.Get("yaml") | ||
| split := strings.Split(yamlTag, ",") | ||
| // Skip the first entry, which is the field name | ||
| isInlined := slices.Contains(split[1:], "inline") | ||
|
|
||
| if isInlined && sf.Type.Kind() == reflect.Struct { |
There was a problem hiding this comment.
Can probably use strings.Cut for this;
Suggested change
| yamlTag := sf.Tag.Get("yaml") | |
| split := strings.Split(yamlTag, ",") | |
| // Skip the first entry, which is the field name | |
| isInlined := slices.Contains(split[1:], "inline") | |
| if isInlined && sf.Type.Kind() == reflect.Struct { | |
| _, yamlOpts, _ := strings.Cut(sf.Tag.Get("yaml"), ",") | |
| if yamlOpts == "inline" && sf.Type.Kind() == reflect.Struct { |
| // Inlined struct, check whether the env variable corresponds to a field inside this struct | ||
| // Maps could also be inlined, but since we don't need it right now it is not supported | ||
| inlined := v.Field(i) | ||
| for j := 0; j < inlined.NumField(); j++ { |
There was a problem hiding this comment.
Current versions of Go allow simplifying this;
for j := range inlined.NumField() {| // Maps could also be inlined, but since we don't need it right now it is not supported | ||
| inlined := v.Field(i) | ||
| for j := 0; j < inlined.NumField(); j++ { | ||
| if strings.ToUpper(inlined.Type().Field(j).Name) == path[0] { |
There was a problem hiding this comment.
Better to use Equalfold for these, as it doesn't allocate;
Suggested change
| if strings.ToUpper(inlined.Type().Field(j).Name) == path[0] { | |
| if strings.EqualFold(inlined.Type().Field(j).Name, path[0]) { |
| } | ||
| } | ||
| } | ||
|
|
||
| upper := strings.ToUpper(sf.Name) | ||
| if _, present := byUpperCase[upper]; present { | ||
| panic(fmt.Sprintf("field name collision in configuration object: %s", sf.Name)) |
There was a problem hiding this comment.
Unrelated to this PR, but why are we using a panic() here? This function has an error return; can't we just return an error? 🤔
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.
This is a quick and dirty fix for #4621. The underlying issue is as mentioned in #4621 (comment).
This fixes it by checking for the
inlinedirective inside YAML struct tags when parsing environment variables into the configuration struct. If it finds this directive, it will check if the environment variable corresponds to a field inside the inlined struct and overwrite that instead.In order to keep things backward compatible for people using the workaround, the code will still accept an environment variable with the full inlined struct field name in it.
For example, for the Redis configuration, which has an inline struct field named Options, both
REGISTRY_REDIS_OPTIONS_ADDRSandREGISTRY_REDIS_ADDRSwill point to the same struct field.Feel free to close this if you'd like to see it handled differently; tying custom env variable parsing logic to the
yamlstruct tag is pretty janky, but it is a simple fix. A rewrite of the entire configuration package (maybe just using a third-party package that handles all of this) would be nice, but that would warrant some more discussion I'd guess.