Skip to content

Support for YAML configuration files#1116

Closed
lminiero wants to merge 13 commits intomasterfrom
yaml-config
Closed

Support for YAML configuration files#1116
lminiero wants to merge 13 commits intomasterfrom
yaml-config

Conversation

@lminiero
Copy link
Copy Markdown
Member

See discussion in this post on the Google group.

This patch adds support for YAML configuration files. Whether the INI or YAML parser is used only depends on the extension of the file you feed it: if it's .yaml, YAML is used, otherwise we parse the file exactly as before. In both cases, you end up with a janus_config file, so the very same methods (involving categories and items) work no matter the format, which means it's completely backwards compatible. Saving a file to YAML or INI, instead, depends on the is_yaml property on janus_config, e.g.:

if(config) {
	/* Save as YAML */
	config->is_yaml = TRUE;
	janus_config_save(config, "/home/lminiero/", "test");
	/* Save as INI */
	config->is_yaml = FALSE;
	janus_config_save(config, "/home/lminiero/", "test");
}

The patch also lays the ground for future enhancements, namely subcategories and sequences/array. At the moment subcategories are defined but not parsed/used (no plugin currently uses them anyway). For sequences/arrays I'll have to think about how to represent them internally (it will probably be some new API in config.h to represent the new object). In order to facilitate all this, I got rid of the original janus_config_category, which is now an alias for janus_config_item for backwards compatibility.

As a proof of concept, I've converted the janus.cfg file to a janus.yaml file, and it seems to be working as expected. Feedback welcome!

@fbertone
Copy link
Copy Markdown
Contributor

fbertone commented Jan 5, 2018

Very useful!
Any idea on when it will be officially integrated?

@lminiero
Copy link
Copy Markdown
Member Author

lminiero commented Jan 5, 2018

Not sure yet, I'll first need to add support for those things that are still missing, or it won't add anything to the existing INI support. You can contribute to the discussion here: https://groups.google.com/forum/#!topic/meetecho-janus/yrSPbANpDVM

@fbertone
Copy link
Copy Markdown
Contributor

fbertone commented Jan 5, 2018

Thank you, I will try it next week. Even if it does not add anything new it's still a feature.
By missing things you mean subcategories and arrays?
Do you need help with testing specific things or implementation?

@lminiero
Copy link
Copy Markdown
Member Author

lminiero commented Jan 6, 2018

By missing things you mean subcategories and arrays?

Yep.

Do you need help with testing specific things or implementation?

At the moment the core is the only part that tries to load a .yaml file, so even just playing with janus.yaml and the different settings and making sure they're all still picked correctly would already help. Converting a plugin .cfg file to .yaml and have the plugin load that one instead would also be helpful, as we want to make that step too sooner or later.

@ibc
Copy link
Copy Markdown

ibc commented Feb 27, 2018

😎

@ibc
Copy link
Copy Markdown

ibc commented Feb 27, 2018

IS IT DONE?

@ibc
Copy link
Copy Markdown

ibc commented Feb 27, 2018

AND NOW?

@lminiero
Copy link
Copy Markdown
Member Author

No

@s-ol
Copy link
Copy Markdown
Contributor

s-ol commented Mar 1, 2018

With the nesting abilities of YAML wouldn't it make sense to inline the plugin configuration in the main configuration file?

Of course this is a bigger change and not easily made backwards-compatible.

One thing I should clarify from the start is that, while the core does require INI files right now, plugins are not really required to use INI config files. The fact that the EchoTest plugin loads the janus.plugin.echotest.cfg file and expects it to be an INI, for instance, is just a convention, something we did from the start to keep some consistence in terms of configurations.

I suppose the other reason is that you want to keep this convention relaxed and not force plugins to serialize to/from passed-in YAML objects?

@lminiero
Copy link
Copy Markdown
Member Author

lminiero commented Mar 1, 2018

With the nesting abilities of YAML wouldn't it make sense to inline the plugin configuration in the main configuration file?

No, as plugins can load whatever kind of configuration they want: the fact the stock plugins follow the same format as the core is just a convention in our repo, but nothing prevents you to write your own plugin that loads an XML config, for instance. All plugins get at startup is the path where configurations can be found, and they're free to ignore that too.

@lminiero
Copy link
Copy Markdown
Member Author

lminiero commented Mar 2, 2018

This last commit adds support for subcategories (no arrays yet). Core and plugins don't use them yet, but this still required some breaking change in how the janus_config methods were conceived, as otherwise navigating the configuration file or adding/removing/looking for stuff in a possibly more indented document would have been impossible. Notice that it doesn't make a difference if you use INI or YAML config files, the C methods are the same: of course, any subcategory will be dropped when you save to INI.

I tried to make the changes in the API as simple as possible, so I'll summarize them here for plugin developers that currently make use of this:

janus_config_get_categories

OLD: GList *janus_config_get_categories(janus_config *config);
NEW: GList *janus_config_get_categories(janus_config *config, janus_config_category *parent);

Notice the extra parent argument. If it's NULL you're asking for the categories in the root (exactly as it has worked so far), if it's a valid category instance you'll get its subcategories instead.

janus_config_get_category

OLD: janus_config_category *janus_config_get_category(janus_config *config, const char *name);
NEW: janus_config_category *janus_config_get_category(janus_config *config, ...);

Notice how name goes away, and is replaced by a variable list of arguments. This is needed because we may be looking for a subcategory of a subcategory of a category, rather than just one of the root categories as we did so far. The way this works is that you list the "path" to the category you need, and end the list with NULL, e.g.:

janus_config_category *c = janus_config_get_category(janus_config *config, "general", NULL);

to get a root category, or:

janus_config_category *c = janus_config_get_category(janus_config *config, "vehicles", "cars", "sport", NULL);

to traverse the document until you get to the "sport" category that is deep within those parent categories instead.

janus_config_add_category

OLD: janus_config_category *janus_config_add_category(janus_config *config, const char *category);
NEW: janus_config_category *janus_config_add_category(janus_config *config, janus_config_category *parent, const char *category);

As you can see, we just added a parent argument. If it's NULL, we add the category to the root (as the old method did), otherwise we're adding a subcategory to the specified parent.

janus_config_remove_category

OLD: int janus_config_remove_category(janus_config *config, const char *category);
NEW: int janus_config_remove_category(janus_config *config, janus_config_category *parent, const char *category);

Same exact considerations as for janus_config_add_category (new parent argument).

janus_config_add_item

OLD: janus_config_item *janus_config_add_item(janus_config *config, const char *category, const char *name, const char *value);
NEW: janus_config_item *janus_config_add_item(janus_config *config, janus_config_category *category, const char *name, const char *value);

Here we only changed the type of category. In fact, before a category could only be at the root, so there was no ambiguity. Now that we can have subcategories, we don't have that luxury, meaning we must provide the right category instance to add the item to.

Notice that if this means a subcategory, you must first look for the right category and then pass it to this method: previously the method would do that for you. Doing the same would have required a variable list of arguments in here as well, which seemed overkill and very inefficient.

janus_config_remove_item

OLD: int janus_config_remove_item(janus_config *config, const char *category, const char *name);
NEW: int janus_config_remove_item(janus_config *config, janus_config_category *category, const char *name);

Same exact considerations as for janus_config_add_item (type change for category).

janus_config_get_item

No change.

janus_config_get_item_drilldown

This method has gone, it's not part of the API anymore. See the considerations I made for janus_config_add_item on why: basically, envisaging a variable list of arguments in here as well, to traverse all the categries up to where the item is, and finally the name of the item itself, seemed overkill and very inefficient. Much easier to just look for the category first, and then call janus_config_get_item instead. This is what core and all plugins that used it before do now, e.g.:

janus_config_category *config_general = janus_config_add_category(config, NULL, "general");
janus_config_item *item = janus_config_get_item(config_general, "enabled");
[..]	
item = janus_config_get_item(config_general, "json");
[..]	

instead of:

janus_config_item *item = janus_config_get_item_drilldown(config, "general", "enabled");
[..]	
item = janus_config_get_item_drilldown(config, "general", "json");
[..]	

which should be more efficient as you don't look for the category each time you try to get an item, which is what janus_config_get_item_drilldown did.

Adapting your plugin to the new API

If you want to adapt your current plugin without using indented stuff yet, it should be very easy:

  1. if you're using janus_config_get_categories, add a NULL argument (no parent);
  2. if you're using janus_config_get_category, add a NULL argument as well (last item in variable arguments list);
  3. if you're using janus_config_add_category and/or janus_config_remove_category, add a NULL argument between config and category (no parent);
  4. if you're using janus_config_add_item and/or janus_config_remove_item, replace the category name with the category instance (possibly looking it up only once for all calls);
  5. if you're using janus_config_get_item_drilldown, replace it with a janus_config_get_category call (possibly only once for all items in the same category) followed by janus_config_get_item.

Again, looking at the diff for core and plugins should clarify how easy the transition is.

Feedback welcome!

@lminiero
Copy link
Copy Markdown
Member Author

lminiero commented Mar 7, 2018

Finally added support for sequences/arrays, even though just for strings and not for mappings/objects yet. Anyway, this forced me to considerable refactor the config API, meaning the summary I made a few days ago is now obsolete...

That said, I believe the refactoring I did now is much more flexible, and more importantly clean from a programming perspective. The old config API did a lot of assumptions, and masqued several things behind the curtains. This made it quite constrained, and proved unusable for these new concepts. The new API is much cleaner, and is inspired from the Jansson API. For instance, to add something to a config:

  1. you create a node object (item/category/array)
  2. you add that node to the root or to a parent (which is another node)

Lookup is cleaner as well, and so is the removal of nodes from the object. It's quite late here, so I don't have time to summarize the API changes right now, but again, even though this is a refactoring, updating your code to use the new API will be much less work than you fear. I'll make some examples tomorrow, by referring to how some of the plugins use it now.

Now that we have sequences, the plan is to also start using these new concepts in the Janus core configuration somehow. One example might be the ability to configure more than one STUN server, which is something someone showed interest for a few weeks ago. This should give us a practical usage for all this new stuff, which will anyway be very useful for the things we're planning next.

@lminiero
Copy link
Copy Markdown
Member Author

Just FYI, I ditched YAML for the libconfig format, which I like more in general and the library is much less of a pain. I'll publish a PR shortly. Closing this.

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.

4 participants