feat: Support multiple configuration files #389
Conversation
henningjanssen
force-pushed
the
326-distributed-config
branch
from
de7823b
to
ea0d855
Compare
henningjanssen
changed the title
henningjanssen
changed the title
TwiN
changed the title
| func TestLoadConfigurationFile(t *testing.T) { | ||
| _, err := LoadConfiguration("../test-conf/config.yaml") | ||
| if nil != err { | ||
| t.Error("Should not have returned an error, because the configuration file exists at the provided path") | ||
| } | ||
| } | ||
|
|
||
| func TestLoadDistributedConfiguration(t *testing.T) { | ||
| _, err := LoadConfiguration("../test-conf/conf.d/") | ||
| if nil != err { | ||
| t.Error("Should not have returned an error, because configuration files exist at the provided path for distributed files") | ||
| } | ||
| } | ||
|
|
||
| func TestLoadCombinedConfiguration(t *testing.T) { | ||
| _, err := LoadConfiguration("../test-conf/empty-conf.d/") | ||
| if nil == err { | ||
| t.Error("Should have returned an error, because the configuration directory does not contain any configuration files") | ||
| } | ||
| } |
There was a problem hiding this comment.
Note to self: rewrite that in a single Test (use test table method) and dynamically generate the config directories and files instead of using the test-conf folder. Just make sure to defer clean up
Codecov ReportBase: 81.99% // Head: 82.36% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #389 +/- ##
==========================================
+ Coverage 81.99% 82.36% +0.37%
==========================================
Files 55 55
Lines 4148 4163 +15
==========================================
+ Hits 3401 3429 +28
+ Misses 589 579 -10
+ Partials 158 155 -3
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
@henningjanssen Thank you for your contribution! |
|
By the way, I did some testing locally and it turns out your implementation doesn't work 😓 |
|
@henningjanssen I'm really sorry; I had to revert it because I wasn't able to fix it before signing off for the day (it's nearly 4AM for me) and I don't want to leave the master branch in an inaccurate state. I'll have a look at it later. |
|
Yeah, because there is no "correct" way of handling duplicate nested keys, especially if they have different types. But this should also be an issue with duplicating keys inside the same yaml-file as this implementation sees no difference between a single and multiple files due to concatenation of byte streams before parsing. The advantage of this approach is to have f.e. alert-config (containing access tokens) separated from endpoints so that the latter can be checked into version control while the former is deployed on the host directly. While probably breaking the YAML protocol, splitting endpoints into multiple files is possible by omitting the top-level key. The load order can be influenced by filename ( |
|
Oh it's definitely possible, don't worry :P Personally, I think there would be no advantage implementing this feature if we can't define endpoints in multiple files -- that's what people want the most, because the endpoint configuration is the longest configuration. It's going to require some testing, but long story short, I'll make it deep merge the yaml files, append keys that have a slice as value rather than overwrite other slices, and prevent duplicate keys with primitive as value. I'll re-add your changes with the fix in #396 |
|
Surprisingly, outside of yq (which had too many dependencies for my liking), I haven't found any decent libraries to deep merge YAML files, so I made one: https://github.com/TwiN/deepmerge Granted it could be because I haven't searched for that long, but 🤷♂️ |
|
Superseded by #396 |
Summary
This PR allows configuration to split across multiple files using the common
config.d/-path style. The approach reads all*.ymland*.yamlfiles and merges them in memory into a single file before parsing. This seemed easier und required fewer changes than parsing each file individually and merging the objects afterwards.Fixes #326
As I did not work with golang before, feel free to remark on obvious errors and bad patterns.
I envisaged to write tests, but did not figure out yet, how invasive tests are intended to be structured for this project. I hope the chosen way is in you interest.
Checklist
README.md, if applicable.