Page MenuHomePhabricator

Changeprop should be able to read multiple config files
Open, Needs TriagePublic

Description

In my case, I'm trying to add some changeprop routes in mw-vagrant, only when the "jade" role is enabled. I can't see a nice way to do this, while there's a single config file. I could savagely stdlib::concat for now, but this isn't a sustainable solution

Event Timeline

Pchelolo subscribed.

@awight I don't think we need multiple config.yaml files, because they're generic for all the services and it's not entirely clear how to merge them? Recursively merge all the objects in several configs? That could be very error-prone and can easily get not understandable.

Instead, I can envision 2 solutions here:

  1. We can add support for referencing rule definition files from the config.yaml file. Then we could have ores.rules.yaml, restbase.rules.yaml etc and in config.yaml just put a list of paths to these files and merge them when config is loaded. Alternatively, we can use the Jinja2 (template engine used by Scap3) {{% include %}} command to render them all together. This will add some separation of concerns, but will not solve your problem of only enabling the rule if the Jade role is enabled.
  2. This is a much simpler option, we can hard-code the rule into the config and have a conditional around it depending on whether ores_uris is defined or not. We can conditionally set ores_uri in the init.pp depending whether the Jade role (and subsequently jade class) is defined using defined(Class['jade'])

I personally like the second option more as it's much simpler and we're already using it for some conditional config params in RESTBase Vagrant manifests.

Great, either of these solutions sound good to me, especially the simpler one :-)

The only catch is that @bd808 gave me a strong warning to not create this kind of dependency between modules, since compilation order during dependency resolution is hard to control, if I understood correctly. I'd like to get his seal of approval before continuing with the suggested patch...

The only catch is that @bd808 gave me a strong warning to not create this kind of dependency between modules, since compilation order during dependency resolution is hard to control, if I understood correctly. I'd like to get his seal of approval before continuing with the suggested patch...

Any solution that's been discussed will need to have some kind of the inter-module dependency like that. Even if we implement multi-config solution, we will still need to know whether to include the ores config or not. I'm not sure what's the state of the art solution here, let's wait for @bd808 wisdom.

Using something like defined(Class['jade']) might work like you think it should and it might not. "Puppet depends on the configuration’s evaluation order when checking whether a resource is declared." -- puppet docs. This means that defined(Class['jade']) will only be true in the scope of your template evaluation if the Puppet class/module that it is attached to comes after ::jade in the compiled Puppet manifest. This can be ensured with something like Class['jade'] -> Class['changeprop'] (names need to match the actual Puppet resources of course) in the ::jade class. If you don't do that however, it will be kind of random whether it works or not from machine to machine.

The bullet proof solution is something like:

  • a Puppet define that manages a conf.d style config snippet for changeprop
  • an exec that requires all the conf.d snippets and then compiles a single config file for changeprop

OR

  • native support in changeprop for a conf.d style configuration system

Y'all can build whatever thing you need to build to get your stuff working, but defined() is less magical than you might think at first glance.