Fix configuration override

[agateau-gg]

Context

If a key is set in the global configuration file, but not in the local one, then the global value is ignored unless it's a list key.

The current code loads each configuration file (global and local) as UserConfig instances, then merge the instances.

When the local UserConfig instance is loaded, if a key is not set, its field uses the default value. This is a problem because then the code responsible for merging the global and local UserConfig instances considers that the local value has explicitly been set and ignores the global value.

With this change, ggshield now loads each config as a dict, merges the dicts and creates a UserConfig instance from the merged dict. This fixes the bug because a dict keeps the notion of whether a key was set or not, so the merge works as expected.

Review

Sorry the changes are quite large to review as I had to rework a good chunk of the loading code.

Important aspects:

• Most of the config loading code moved from UserConfig._update_from_file() to a free function _load_from_dict().
• The UserV1Config class is gone, replaced with a conversion function in a dedicated module: v1_config.

[agateau-gg]

Everything looks good to me. I'm just a bit skeptical about convert_v1_config_dict function, is there a reason you decided to switch from a class to a util function to convert the config?

The previous code used a class because it made use of Marshmallow ability to turn a dict into a Python class. The new code does not return a class: it takes a dict and processes it to return another dict. The fields declared in the UserV1Config class were not used anymore, so there was no benefit for a class there.

[agateau-gg]

This MR looks good to me but I have a question with regard to migrating the configs and replacing - with => If you have a config with the same variable but one with - and one with you will silently take the one with the dashes , maybe you could issue a warning to the user in this (unlikely) case ?

That's quite a corner case, but I agree it's an odd behavior, especially since the code gives precedence to the deprecated syntax. Going to change this to ignore the dash version. I don't know if it's worth adding a warning for this because the key will be reported as deprecated anyway, and I'd rather avoid the complexity of having to produce different warning messages for "key is deprecated" and "key has been ignored".