Skip to content
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

Adding support for merging a list of configs external to the second fig #10

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sbrowndev
Copy link
Contributor

Resolves #9 @bjarkih this change is to allow the system parameter twigkit.conf.override to be implemented. Any configs given by the parameter will override any changes made through the overlay.

@sbrowndev
Copy link
Contributor Author

I've updated the syntax slightly to make certain properties accessible, for example:

mvn clean jetty:run -o -Dtwigkit.conf.override=services.suggestions.gsa[entries.max:6

The use of the [ separates the config path from the config property since the config property may also have a . in it

@sbrowndev
Copy link
Contributor Author

So, commands like this will now be possible:

mvn clean jetty:run -o -Dtwigkit.conf.overlay=file://src/dev/my_resources/conf -Dtwigkit.conf.override=services.suggestions.gsa[entries.max:6&cors[property:true

@bjarkih
Copy link
Contributor

bjarkih commented Feb 12, 2016

This is great! I'm not too fond of the bracket notation though. Actually your point on parameter with full stops in the name is a good one as there is some ambiguity in how we treat nodes vs node properties. For example, we might have the following configuration tree:

platforms
    solr
        - people.host: foo
        people
            - host: bar

Can we make the simplification to always assume that the last component of the override term (when split on full stops) represents the node property to set? Thoughts on that @mrolafsson?

@mrolafsson
Copy link
Contributor

I would prefer the syntax to be like -Dtwigkit.conf.set.some.property=Some Value. You would just have to validate (e.g. via unit test) that when a property has full stops that it ends up as a leaf in the tree just as if it was in a separate file from Bjarki's example above.

@mrolafsson
Copy link
Contributor

Can we ensure that the system property method is documented: http://dev.twigkit.net/docs/Getting_started/Configuration/Overlaying_configuration_values/

@bjarkih
Copy link
Contributor

bjarkih commented Mar 1, 2016

It hasn't been merged as syntax needs reviewing :)

@mrolafsson
Copy link
Contributor

I have reviewed the syntax :)

@sbrowndev
Copy link
Contributor Author

@mrolafsson @bjarkih using this syntax -Dtwigkit.conf.set.some.property=Some Value as a template, how about we use the following, bearing in mind that we have to know the path to the config as well as have the means to tell the difference between the path and the property, which may contains dots:

-Dtwigkit.conf.set="cors:cors.allowedOrigin=300"

and in cases where we have parent.child paths and more than one property to set:

-Dtwigkit.conf.set="cors:cors.allowedOrigin=300&services.suggestions.gsa:entries.max=6"

We don't need the quotes but it would help distinguish between setting the system parameter and setting the property value. Then we use : to separate the path from the property and & to allow multiple config properties to be set.

With this we avoid any ambiguity that the given property may not be unique and may not exist.

@mrolafsson
Copy link
Contributor

I'd prefer it if we constructed the tree correctly whether you have dot notation in properties or not. In that case there will never be ambiguity. I know this is not the same as our custom attribute syntax but that should go way, but I would find this more readable: services.suggestions.gsa[entries.max:6]?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants