[Kimchi-devel] [PATCH] [WoK 9/9] Removing configobj, use file operations to edit conf files

[Lucio Correia]

On 20/01/2017 10:15, dhbarboza82 at gmail.com wrote:
> From: Daniel Henrique Barboza <danielhb at linux.vnet.ibm.com>
>
> Before the implementation of the /config/plugins API, WoK used
> cherrypy.lib.reprconf.Parser to parse the .conf files of the
> plug-ins. In the implementation of this API we have the need to
> edit these config files, setting the 'enable = ' line from the WoK
> section to either True or False. Using this same parser for writing
> the files presented to be a problem because the writing process were
> deleting the comments presented in the conf files. No option is
> available to make the write() call of cherrypy.lib.reprconf.Parser
> preserve the comments.
>
> To solve this, the implementation of /config/plugins introduced
> configobj.ConfigObj, a python standard parser, to make this write
> operation in the conf files. The write() call of this parser retains
> the comments. So we ended up using cherrypy.lib.reprconf.Parser to parse
> the conf files to the plug-ins and configobj.ConfigObj to edit/write the
> 'enable =' option. It is worth mentioning that although
> cherrypy.lib.reprconf.Parser implementation extends configobj.ConfigObj,
> his write() call extends another class called
> 'backports.configparser.RawConfigParser'. This explains why the write()
> call from both parsers differs.
>
> A problem in this approach was detected when dealing with the conf
> file of the Ginger plug-in. This is the relevant portion of the Ginger
> conf file:
>
> [backup]
> default_include = ['/etc', '/var/spool/cron']
> default_exclude = ['/etc/init.d', '/etc/rc.d', '/etc/rc?.d']
>
> This declaration of default_include and default_exclude is invalid to
> the configobj.ConfigObj parser:
>
>>>> import configobj
>>>> parser = configobj.ConfigObj('ginger.conf')
> Traceback (most recent call last):
>   File "<stdin>", line 1, in <module>
>   File "/usr/lib/python2.7/site-packages/configobj.py", line 1229, in __init__
>     self._load(infile, configspec)
>   File "/usr/lib/python2.7/site-packages/configobj.py", line 1318, in _load
>     raise error
> configobj.ConfigObjError: Parsing failed with several errors.
> First error at line 6.
>>>>
>
> This resulted in the /enable and /disable plug-ins being broken when
> dealing with Ginger. After experimentation with the ginger.conf format
> no common ground was found between both parsers to proper handle this
> list declaration of the Ginger file.
>
> One solution that was speculated was to get rid of
> cherrypy.lib.reprconf.Parser altogether and use configobj.ConfigObj in all
> cases, meaning that we would use it to parse the conf files of the plug-ins
> too. At first only the Ginger conf file was going to be changed but then
> we realized that ConfigObj parses boolean in a different fashion too.
>
> This is the parsing of the current kimchi.conf file using both parsers:
>
>>>> import configobj
>>>> parser = configobj.ConfigObj('kimchi.conf')
>>>> parser.dict()
> {'kimchi': {'create_iso_pool': 'True', 'federation': 'False'}, 'wok': {'enable': 'True'}}
>>>>
>>>> from cherrypy.lib.reprconf import Parser
>>>> Parser().dict_from_file('kimchi.conf')
> {'kimchi': {'create_iso_pool': True, 'federation': False}, 'wok': {'enable': True}}
>>>>
>
> cherrypy.lib.reprconf.Parser delivers boolean values in the dict,
> ConfigObj delivers string values 'True' and 'False'. This means that the
> change to use ConfigObj in the parsing of the conf files would need either:
>
> • change Kimchi and Gingerbase plug-ins to handle the now string values
> 'True' or 'False' in its backend logic
>
> or
>
> • WoK would need to convert the string values to boolean before sending it
> to the plug-ins. This can be annoying considering that the conf file
> supports multiple sections and subsections.
>
> The saner solution this patch implements is to get rid of configobj.ConfigObj
> to do the write of the conf files. This is now being manually made with
> standard Python file operations. The file editing required in this feature
> is too simple to warrant for all this hassle with two differents parsers,
> different INI formats and so on.
>
> A unit test was edited to make the .conf file a little trickier to ensure
> that our file operation is editing and writing the right attribute of the
> conf file.
>
> Signed-off-by: Daniel Henrique Barboza <danielhb at linux.vnet.ibm.com>
> ---
>  src/wok/utils.py    | 22 ++++++++++++++++++----
>  tests/test_utils.py |  5 +++++
>  2 files changed, 23 insertions(+), 4 deletions(-)
>
> diff --git a/src/wok/utils.py b/src/wok/utils.py
> index d841371..c2d4724 100644
> --- a/src/wok/utils.py
> +++ b/src/wok/utils.py
> @@ -35,7 +35,6 @@ import traceback
>  import xml.etree.ElementTree as ET
>
>  from cherrypy.lib.reprconf import Parser
> -from configobj import ConfigObj
>  from datetime import datetime, timedelta
>  from multiprocessing import Process, Queue
>  from threading import Timer
> @@ -117,9 +116,24 @@ def set_plugin_state(name, state):
>      if not plugin_conf:
>          return
>
> -    config = ConfigObj(plugin_conf)
> -    config['wok']['enable'] = str(state)
> -    config.write()
> +    config_contents = None
> +
> +    with open(plugin_conf, 'r') as f:
> +        config_contents = f.readlines()
> +
> +    wok_section_found = False
> +
> +    for i in range(0, len(config_contents)):
> +        if config_contents[i] == '[wok]\n':
> +            wok_section_found = True
> +            continue
> +
> +        if config_contents[i].startswith('enable =') and wok_section_found:
> +            config_contents[i] = 'enable = %s\n' % str(state)
> +            break
> +
> +    with open(plugin_conf, 'w') as f:
> +        f.writelines(config_contents)
>
>
>  def get_all_tabs():
> diff --git a/tests/test_utils.py b/tests/test_utils.py
> index c0e1009..6f1a783 100644
> --- a/tests/test_utils.py
> +++ b/tests/test_utils.py
> @@ -76,12 +76,17 @@ class UtilsTests(unittest.TestCase):
>
>      def _get_fake_config_file_content(self, enable=True):
>          return """\
> +[a_random_section]
> +# a random section for testing purposes
> +enable = 1
> +
>  [wok]
>  # Enable plugin on Wok server (values: True|False)
>  enable = %s
>
>  [fakeplugin]
>  # Yet another comment on this config file
> +enable = 2
>  very_interesting_option = True
>  """ % str(enable)
>
>
I believe we may have complaints about problems when user uses:
enabled=1  (without spaces).

This is supported by INI files and we should handle it as well.


-- 
Lucio Correia