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
idxmgmt: fix config merging #4950
Conversation
01ac31c
to
aa573e3
Compare
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
Trends 🧪 |
jenkins run the tests please |
// resolved using the root of the left-most config in the merge. | ||
// We merge the user-defined config back over the defaults to | ||
// ensure they take precedence. | ||
return common.MergeConfigs(configRoot, defaultConfig, configRoot) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks complicated - looking at the common.MergeConfigs
code I couldn't figure out why we need the extra configRoot
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, hence the NOTE :)
Is the comment unclear?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tweaked the comment slightly, hopefully it's a bit clearer now.
Just found a bug, moving back into draft while I fx. |
Check before we merge, and use config.Has rather than unpacking twice.
* idxmgmt: fix config merging * Fix how we determine setup.template specified Check before we merge, and use config.Has rather than unpacking twice. * systemtest: improve logging for assertion # Conflicts: # changelogs/head.asciidoc
* idxmgmt: fix config merging * Fix how we determine setup.template specified Check before we merge, and use config.Has rather than unpacking twice. * systemtest: improve logging for assertion # Conflicts: # changelogs/head.asciidoc
* idxmgmt: fix config merging * Fix how we determine setup.template specified Check before we merge, and use config.Has rather than unpacking twice. * systemtest: improve logging for assertion # Conflicts: # changelogs/head.asciidoc
Motivation/summary
Fix a bug related to merging user-defined and default
setup.template
config.We were merging onto a new config, which meant root config attributes like
path.config
were no longer accessible. The fix is to merge defaults over the root config to ensure the root config attributes are still accessible, and then merge the root config back over that to ensure user-defined config takes precedence.Checklist
How to test these changes
See #4949
Related issues
Fixes #4949