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

Fri Jan 20 14:10:35 UTC 2017


On 01/20/2017 11:24 AM, Lucio Correia wrote:
> 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.
>
>

I'll add support for this format in the v2