Robottelo: Replace INI reader with Dynaconf

[pgagne]

I think we should look to change configuration file from using ConfigParser-style INI files to using YAML.

I am filing this issue to solicit feedback and discuss things that need to be done to accomplish this. Once I hear some I can work on trying to get a PR for this.

Why I think we need this

Right now we use pythons configparser INI format for robottelo.properties. Which allows 2 levels of hierarchy. Options must with be in the "DEFAULT" section or a named `[section_name]:

# Make a copy of this file named robottelo.properties
[server]
# Server hostname
hostname=
# Path to private ssh key to be used when connecting via SSH.
ssh_key=

This is good for simple k/v variables, but it gets a little unwieldy when you want to have more structured options. For my example of this see https://github.com/SatelliteQE/robottelo/pull/6334#issuecomment-428580300

I believe there are other cases of this, and more will come up as we work with more complex integrations.

While we have added support via Casts for list, dict , etc this only allows 1 more level of hierarchy and could make the file a bit unreadable. PyYAML has builtin parsing of dicts, lists, etcs.

Possible solutions

We have a fair amount of code that looks at the Settings objects, luckily we have abstracted a lot of the reading of the config parser into INIReader. I believe the fastest approach would be to create a new class like YAMLReader that has the same methods as INIReader. This would mean we wouldn't have to readwrite any of the FeatureSettings code to handle the new format.

There would be some other refactoring required, anywhere INIReader is called.

We can create a script to convert existing robottelo.properties to robottelo.yaml.

Questions

Here are some question I would like some feedback on.

1. Do people likes this idea?

2. Would we want to support both the new YAML and old INI at the same time, or can we do a clean cut over (assuming we have an easy way to convert the new files.

3. What should the file be called?
   -Do we want to call it robottelo.properties but have it contain YAML, just so it will be compatible with existing CI/tooling?
   -We could probably support multiple names robottelo.yaml, 'robottelo.properties`, etc.

4. Does anyone see any advantage to moving to using a config directory, with multiple files instead of one. Something like :robottelo.config.d. In some ways I think this would be better, but I go back and forth.

5. The INIReader compantable YAMLReader would be good for existing configs, since the INI format is only 2 levels. However YAML allows for much more complicated configs, what sorts of methods will we want to have for "newer" configuration features?

[abalakh]

Issue description focuses on _how_ we should switch to yaml, but doesn't answer most important question - _why_ we should do that? Do we have any use cases current ini reader can't handle? Or what's the value of such change?
I'm not against yaml, just curious if there's a reason for such change other that "yaml is just better". 🙂

[pgagne]

@abalakh You right, I forgot to go into some detail on that, I updated this description with some of my thoughts on why I think this is needed. Please let me know if you have any questions.

[rochacbruno]

https://dynaconf.readthedocs.io/en/latest/ :)

[flatfender]

My feeling is the yaml structure would allow us to parameterize things easier, especially when you want to iterate over over a list of parameters.

[pgagne]

@rochacbruno I have read some stuff around dynaconf, but haven't really be able to wrap my head around how best to integrate it into robotello. Perhaps you might be able to help me understand it better by providing some example code (maybe a gist or something) of a couple of FeatureSettings in YAML format and how they would be parsed with dynaconf.

[JacobCallahan]

I'm partial to YAML, so I'm sure you know my answer.

[omkarkhatavkar]

I'm in favour of.yaml if it adding easiness, flexibility and speed. I won't feel to support both files like .properties and .yaml in a framework. This will add an overhead to know which config is placed in which file. I like the idea of adding multiple .yaml files in '.robottelo.config.d' folder to base on the objective. But we have to make sure that we should not end up with adding a lot of files. (there has to a criteria)

[pgagne]

@san7ket any thoughts on this from a CI perspective?

[mirekdlugosz]

Do people likes this idea?

Well, YAML would be better and would allow us to do some cool things that I would be excited to have. It could help to fix the problem that you encountered in cited PR.

At the same time, it seems that this PR is the only one that actually requires diverging from INI file. Is there really no "dirty" way that we could work around it? Have we really reached the point where namespaced key-value config storage model is serious limitation to what we want to achieve? On top of that, I do believe that you can find problems with much larger business impact in SatelliteQE, probably even here in Robottelo.

Of course, I am not in position to tell anyone how they should spend their time and what they should work on. But all above makes me question whether YAML config is really worth to implement.

Would we want to support both the new YAML and old INI at the same time, or can we do a clean cut over (assuming we have an easy way to convert the new files.

Break backwards compatibility and support only YAML from certain point in time.
As far as I can tell, SatelliteQE members are only consumers of Robottelo. We can coordinate switch. Keeping backwards compatibility with INI file in framework itself is additional work that I can't find justification for.

What should the file be called?
-Do we want to call it robottelo.properties but have it contain YAML, just so it will be compatible with existing CI/tooling?

For better or worse, we have single CI configuration shared across Satellite-next and all supported z-streams. You can't use the same file name, unless you want to backport YAML reading to z-stream branches. And I would strongly recommend against it.

Also, see next answer.

Does anyone see any advantage to moving to using a config directory, with multiple files instead of one. Some like :robottelo.config.d:

Moving from configuration file to configuration directory is probably the strongest argument for that change. First, it's relatively easy to implement (have function that reads config from YAML file and updates global configuration store with new values; run function in loop for all files in directory). Second, it allows us to break config down into smaller, more manageable and focused pieces.

I think all files should add to global storage and share single namespace. This way we could provide sane defaults and documentation here, Satellite QE could slap internal config files on top of these and each contributor could create custom 99.local.yaml at the end. This solves the problem of people having to manually sync their config, SatelliteQE CI config and upstream config.

The INIReader compantable YAMLReader would be good for existing configs, since the INI format is only 2 levels. However YAML allows for much more complicated configs, what sorts of methods will we want to have for "newer" configuration features.

YAMLReader public interface must remain compatible with INIReader interface. Switching config file format is disruptive enough - if we have to update all the tests at the same time, I wouldn't even start.

Of course some changes might be required, and this could give us opportunity to refactor all the parts that worked around limitations of old config storage model. But they should be very limited.

[lpramuk]

@pgagne Did you also think about a tool for bash CI jobs to edit the entities

I mean what is currently done by sed
https://github.com/SatelliteQE/robottelo-ci/blob/master/scripts/satellite6-automation.sh#L5

If you have solution for this then I'm for the change, or do we want to sed yaml files? hehe :-)

[san7ket]

@mirzal there is a way in INI, we are already using it, to store list of dict's in current INI format.
https://github.com/SatelliteQE/robottelo/blob/master/robottelo.properties.sample#L120
We also need to consider the efforts, we need to re-write the helpers or port the current helper that parse the INI/YAML key values settings into setting object. It's a huge task, the eg
https://github.com/SatelliteQE/robottelo/pull/6334/files#diff-2bd9096c3ab5bccd030f8b1ac0c5c553R355

Also regarding the mentioned config problem, we can break it down. We are already syncing docker repos and registries. We can have those in constants file, we only kept or sourced private auth/authorisation details from robottelo.properties file. Something like,
registers=register1_url=adas,user1=,password1=pass,registery2_url=asaa.as,user2 etc. everything else goes to constants.py
if we start adding constants to config, we would end up have a config per feature and that would not be ideal for a generalised solution.

@pgagne Also, if we are going to find a solution, lets look at something that is baked, and not redo everything from scratch. Something like what @rochacbruno suggested.

[pgagne]

@rochacbruno I have read some stuff around dynaconf, but haven't really be able to wrap my head around how best to integrate it into robotello. Perhaps you might be able to help me understand it better by providing some example code (maybe a gist or something) of a couple of FeatureSettings in YAML format and how they would be parsed with dynaconf.

@rochacbruno did you think you might be able to provide me a quick example here?