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

On 20/01/2017 10:15, dhbarboza82(a)gmail.com wrote: > From: Daniel Henrique Barboza <danielhb(a)linux.vnet.ibm.com&gt; > > 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(a)linux.vnet.ibm.com&gt; > --- > 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.