[Kimchi-devel] [PATCH v3 1/4] Move configuration parsing to config.py

[Royce Lv]

On 2014年01月08日 17:42, Mark Wu wrote:
> On 01/08/2014 01:51 PM, Royce Lv wrote:
>> On 2014年01月08日 10:55, Royce Lv wrote:
>>> On 2014年01月07日 16:56, Mark Wu wrote:
>>>> The configuration is also needed for other code except starting kimchi
>>>> server. So it should be moved to a separate module, config.py. Then 
>>>> the
>>>> configuration can be accessed directly by importing config module.
>>>>
>>>> Signed-off-by: Mark Wu <wudxw at linux.vnet.ibm.com>
>>>> ---
>>>>   src/kimchi/config.py.in | 24 ++++++++++++++++++++++++
>>>>   src/kimchid.in          | 20 +-------------------
>>>>   2 files changed, 25 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/src/kimchi/config.py.in b/src/kimchi/config.py.in
>>>> index f3c408a..72308ff 100644
>>>> --- a/src/kimchi/config.py.in
>>>> +++ b/src/kimchi/config.py.in
>>>> @@ -27,6 +27,7 @@ import os
>>>>   import platform
>>>>
>>>>
>>>> +from ConfigParser import SafeConfigParser
>>>>   from glob import iglob
>>>>
>>>>
>>>> @@ -166,5 +167,28 @@ def get_plugin_tab_xml(name):
>>>>       return os.path.join(_get_plugin_ui_dir(name), 
>>>> 'config/tab-ext.xml')
>>>>
>>>>
>>>> +def _get_config():
>>>> +    config = SafeConfigParser()
>>>> +    config.add_section("server")
>>>> +    config.set("server", "host", "0.0.0.0")
>>>> +    config.set("server", "port", "8000")
>>>> +    config.set("server", "ssl_port", "8001")
>>>> +    config.set("server", "ssl_cert", "")
>>>> +    config.set("server", "ssl_key", "")
>>>> +    config.set("server", "environment", "development")
>>>> +    config.add_section("logging")
>>>> +    config.set("logging", "log_dir", get_default_log_dir())
>>>> +    config.set("logging", "log_level", DEFAULT_LOG_LEVEL)
>>>> +
>>>> +    if os.path.exists(CONFIG_FILE):
>>>> +        config.read(CONFIG_FILE)
>>>> +    return config
>>>> +
>>>> +
>>>> +CONFIG_FILE = "%s/kimchi.conf" % get_config_dir()
>>>> +DEFAULT_LOG_LEVEL = "debug"
>>>> +config = _get_config()
>>>> +
>>>> +
>>>>   if __name__ == '__main__':
>>>>       print get_prefix()
>>>> diff --git a/src/kimchid.in b/src/kimchid.in
>>>> index 7865713..548fa52 100644
>>>> --- a/src/kimchid.in
>>>> +++ b/src/kimchid.in
>>>> @@ -33,31 +33,13 @@ import kimchi.config
>>>>   if kimchi.config.without_installation():
>>>>       sys.path.append(kimchi.config.get_prefix())
>>>>
>>>> -from ConfigParser import SafeConfigParser
>>>> +from kimchi.config import config
>>>>   from optparse import OptionParser
>>>>
>>>>   ACCESS_LOG = "kimchi-access.log"
>>>>   ERROR_LOG = "kimchi-error.log"
>>>> -CONFIG_FILE = "%s/kimchi.conf" % kimchi.config.get_config_dir()
>>>> -DEFAULT_LOG_DIR = kimchi.config.get_default_log_dir()
>>>> -DEFAULT_LOG_LEVEL = "debug"
>>>>
>>>>   def main(options):
>>>> -    config = SafeConfigParser()
>>>> -    config.add_section("server")
>>>> -    config.set("server", "host", "0.0.0.0")
>>>> -    config.set("server", "port", "8000")
>>>> -    config.set("server", "ssl_port", "8001")
>>>> -    config.set("server", "ssl_cert", "")
>>>> -    config.set("server", "ssl_key", "")
>>>> -    config.set("server", "environment", "development")
>>>> -    config.add_section("logging")
>>>> -    config.set("logging", "log_dir", DEFAULT_LOG_DIR)
>>>> -    config.set("logging", "log_level", DEFAULT_LOG_LEVEL)
>>>> -
>>>> -    if os.path.exists(CONFIG_FILE):
>>>> -        config.read(CONFIG_FILE)
>>>> -
>>> Starting here:
>>>>       host = config.get("server", "host")
>>>>       port = config.get("server", "port")
>>>>       ssl_port = config.get("server", "ssl_port")
>> And don't forget to update config object if you want to use it else 
>> where, because the right configuration is processed by cmd args.In 
>> your patch it won't cause problem because you don't accepted cmd args 
>> of vnc port, but if in the future if we want to use config object, 
>> that'll cause problem.
> I think we don't need update the config object with the values passed 
> from the command lines.
> We could consider we have two kinds of configurations in the 
> config.py: one could be overrode
> by the command line options and the other doesn't have corresponding 
> option in command line.
> For the first kind,  kimchi server use the merged 'options' instead of 
> directly getting config from
> config module.  For the second case,  it's not related to server and 
> it doesn't make sense to pass
> the option from command line and pass around the option inside 
> kimchi.  The user module should
> be able to get its config directly.
>
> You could feel that it has some potential inconsistent.  I think we 
> can simply the command line options
> to resolve it.  We could remove the options which are in the config 
> and use one parameter: the path of configuration file instead.  It 
> will not cause any inconsistency.  Are you fine to resolve it in a 
> following up patch after we get agreement on this direction?
This idea is cool for me, so I think I have no complaints about this 
version  then.
>
> Thanks!
> Mark.
>
>
>
>
>>> This may be a further refactor , but seems we can use set_defaults 
>>> of optionparser to set config dict as default value of option 
>>> parser, so that the annoying dirty lines of reading every item of 
>>> config can be removed.
>>> REF: 
>>> http://docs.python.org/3.3/library/optparse.html#optparse.OptionParser.set_defaults
>>>
>>> _______________________________________________
>>> Kimchi-devel mailing list
>>> Kimchi-devel at ovirt.org
>>> http://lists.ovirt.org/mailman/listinfo/kimchi-devel
>>
>