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

[dhbarboza82 at gmail.com]

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)
 
-- 
2.7.4