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 ? 14896
==========================================
Files ? 1159
Lines ? 72168
Branches ? 8068
==========================================
Hits ? 55171
Misses ? 14059
Partials ? 2938 ☔ 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. |
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/Settings.java
Show resolved
Hide resolved
|
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: |
…uplication of included file in different branches.
|
Hi @Cole-Greer , |
| if (includedNode instanceof MappingNode) { | ||
| mergeMappingNodes(accumulatedNode, (MappingNode) includedNode); | ||
| } else { | ||
| // Non-map include replaces everything |
There was a problem hiding this comment.
I'm curious what is the expected case which would trigger this else? Directly returning includedNode and ignoring all other configs seems like a very strong action.
I noticed while debugging through org.apache.tinkerpop.gremlin.server.SettingsTest#testIncludeBranches, the processing of the empty base1.yaml results in includedNode = null, which enters this else statement and entirely prevents the test from ever touching branch2.yaml and base2.yaml.
|
Hi @andrii0lomakin, As I expect this feature will be adopted first and foremost by YouTrackDB, I'm happy to defer to your preferences on this matter. I'm happy to proceed with this PR pending the resolution of above comments. |
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.