New feature - the ability to include other YAML files using the "includes" property was added. #3287
Conversation
…udes" property was added. Files including feature support nesting. So one YAML file can include several files that in turn include another file and so on. Circular references are detected during inclusion, and an exception is thrown. The following formats of file paths are supported: 1. The absolute file path. 2. Path relative to including file. 3. Files prefixed with `classpath:` are searched inside class path. Files loaded through class path also support relative path references that will be resolved inside class path scope. Properties of included files are overwritten in order of inclusion, after that including file will overwrite those properties.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 3.8-dev #3287 +/- ##
==========================================
Coverage ? 76.44%
Complexity ? 14884
==========================================
Files ? 1159
Lines ? 72132
Branches ? 8061
==========================================
Hits ? 55144
Misses ? 14044
Partials ? 2944 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
thanks for submitting this. i think a lot of folks are off for the holiday period. it might take a bit longer than usual to review this work. |
| import org.yaml.snakeyaml.TypeDescription; | ||
| import org.yaml.snakeyaml.Yaml; | ||
| import org.yaml.snakeyaml.constructor.Constructor; | ||
| import org.yaml.snakeyaml.nodes.*; |
There was a problem hiding this comment.
nit: we tend to avoid wildcard imports and prefer including explicit imports.
| final NodeMapper constructor = createDefaultYamlConstructor(); | ||
| final Yaml yaml = new Yaml(); | ||
|
|
||
| HashSet<String> loadStack = new HashSet<>(); |
There was a problem hiding this comment.
nit: we tend to be explicit about final declarations - would you mind doing a pass through on the changes here to add final where applicable?
|
Thanks for your patience on this one (I think others are still away for the holiday so we may yet need a bit more time to get all the feedback in). I'd never thought to add this kind of functionality, but it seems like it could be useful for folks. I added a few nits as comments. Here's a couple other items to consider:
I'm happy to help with the documentation tasks if you'd like. |
|
|
||
| private static Node loadNodeRecursive(Yaml yaml, String currentPath, HashSet<String> loadStack) { | ||
| try { | ||
| if (loadStack.contains(currentPath)) { |
There was a problem hiding this comment.
This cycle detection is more restrictive than necessary. For example it would consider the following structure to contain a cycle and reject it when in fact it should be safe:
root.yaml ____ branch1.yaml ____ base1.yaml
\___ branch2.yaml ____ base1.yaml
\___ base2.yaml
|
Hi @andrii0lomakin, my apologies for the delay in getting to this PR. I would like to get your thoughts on a potential alternative design. If we allowed the server to be initialized by a list of yaml files instead of the current single path setup, we could gain much of the modularity of this "inclusion" model, without the complexity of recursive parsing, cycle detection, path normalization... In essence, users could launch the server in manners such as this: Anytime there is a conflict where a single setting is defined in multiple files, we could give precedence to the first file listed. A "set of configs" approach is theoretically less powerful than your proposed "inclusion approach", however I suspect that either approach would be sufficient for the needs expressed in your earlier devlist post. The "set of configs" approach may also lead to other benefits as it may allow new users a lot of flexibility to mix and match different partial configs which are included directly in the server distribution without needing to write their own config yamls. This could be useful for users experimenting with the server via docker, where the need to mount a volume with custom configs often acts as a barrier to new users. It might be nice if users could simply run something like this to get the exact configuration they are looking for: |
4 participants
New feature - the ability to include other YAML files using the "includes" property was added.
Files including feature support nesting.
So one YAML file can include several files that, in turn, include another file and so on.
Circular references are detected during inclusion, and an exception is thrown.
The following formats of file paths are supported:
classpath:are searched inside the class path.Files loaded through the class path also support relative path references that will be resolved inside the class path scope.
Properties of included files are overwritten in order of inclusion. After that, the including file will overwrite those properties.