-
Notifications
You must be signed in to change notification settings - Fork 498
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
config: add yaml/json struct tags #5433
base: main
Are you sure you want to change the base?
config: add yaml/json struct tags #5433
Conversation
Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5433 +/- ##
=====================================
Coverage 62.4% 62.4%
=====================================
Files 189 189
Lines 11638 11638
=====================================
Hits 7263 7263
Misses 4158 4158
Partials 217 217
|
This needs a changelog. I see this adds additional tags, but I'm not 100% sure how this is used. It doesn't necessarily need to be part of the PR, but could you share an example of how this was used before and how it works now? |
Can you add tests that verify that the YAML/JSON files are parsed (thanks to the tags) as expected? |
@MadVikingGod @pellared happy to add some functionality to test it, i didn't want to overstep on this PR #4826 @carrbs if you're ok with it I can incorporate your work here, or you can pull this change in your PR, it's all the same to me :) |
I'm good with incorporating here, thanks @codeboten! |
So, what I was hoping for to understand better why this change is necessary was something like: input := map[string]interface{}
err := json.Unmarshal(rawJson, &input)
config := config{}
err = mapstructure.Decode(input, &config) But afterwards we have
While this looks to add direct ways to decode directly into our Config structures, it also means we have to test three different decoders (Mapstructure, Json, and Yaml) and find all the edge cases of each. |
This will make parsing the configuration easier