search

vert-x3 / vertx-config

Vert.x Configuration Service
Apache License 2.0
54 stars 64 forks source link

Set to JsonObject.mergeIn(deep = true) for FileSet and stores #89

Closed daltrogama closed 5 years ago

daltrogama commented 5 years ago

When loading some configuration from directories with multiple files that need to be merged, the actual code only does the merge operation in the first level of the config tree, causing some unwanted side effects.

This pull request turns recursive all multi-file config merge process in 3.8 branch.

vietj commented 5 years ago

can you add a test for this ?

daltrogama commented 5 years ago

@vietj Added unit tests covering the fix, for all branches.

tsegismont commented 5 years ago

@cescoffier does this look good to you?

@vietj anything else needed?

@daltrogama is there a reason for a PR against 3.8 instead of master?

vietj commented 5 years ago

I believe it's fine and it should be for 3.8 and master, so you can merge the PR @tsegismont and cherrypick to master

daltrogama commented 5 years ago

@daltrogama is there a reason for a PR against 3.8 instead of master?

@tsegismont, it was just to contribute also with the next 3.8 minor version, too. My intention was to fix this for the next 3.7 and 3.6 minors (in other PRs), in case of new releases.

Thanks!