Change in vdsm[master]: replace configure_libvirt.py with python code.

mtayer at redhat.com mtayer at redhat.com
Fri May 16 20:28:39 UTC 2014


mooli tayer has posted comments on this change.

Change subject: replace configure_libvirt.py with python code.
......................................................................


Patch Set 9:

(5 comments)

http://gerrit.ovirt.org/#/c/27298/9/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:

Line 25: import filecmp
Line 26: import rpm
Line 27: import shutil
Line 28: import traceback
Line 29: import uuid
> Import should be sorted.
it wasn't sorted originally,
added patch to sort the whole thing
Line 30: 
Line 31: from .. import utils
Line 32: from . import service, expose, validate_ovirt_certs
Line 33: from configfile import ConfigFile, ParserWrapper


Line 109:             conf.prefixLines(self.PREFIX)
Line 110:             with open(
Line 111:                     self.getFile(content['prependFile']),
Line 112:                     'r'
Line 113:             ) as src_conf:
> It is hard to read when you format this in such a non-standard way. Please 
Agree about removing the lines.
Not really to the temporary variable.
But this isn't too long.
Line 114:                 conf.prependSection(src_conf.read())
Line 115: 
Line 116:     def _removeFile(self, content, context):
Line 117:         """


Line 122:         except OSError as e:
Line 123:             if e.errno != errno.ENOENT:
Line 124:                 raise
Line 125: 
Line 126:     # On removeConf functions
> Missing empty line.
Where? is it a pep8 thing?
Line 127:     def _unprefixAndRemoveSection(self, path):
Line 128:         """
Line 129:         undo changes done by _prefixAndPrepend.
Line 130:         """


Line 135: 
Line 136:     def _removeSection(self, path):
Line 137:         """
Line 138:         remove entire 'configuration section by vdsm' section.
Line 139:         section is remove regardless of it's version.
> remove ->  removed
Done.
Line 140:         """
Line 141:         if os.path.exists(path):
Line 142:             with self._openConfig(path) as conff:
Line 143:                 conff.removeConf()


Line 389:         applyFragment = True
Line 390:         for key, val in fragment['conditions'].items():
Line 391:             if context[key] != val:
Line 392:                 applyFragment = False
Line 393:         return applyFragment
> One would expect that this function does apply the fragment to something, b
1.) Will rename to isApplicable. in English what it should
 mean is "determine if the this fragment should be included
 according to current configuration"

2.) doing
ret=False
for ...:
  if ...
    ret=True
return ret

 Is a conscious decision. Alon convinced me saying functional
 code should never return mid clause. I could add a 
  if ...
    ret=True
    break
but since this code checks 0-4 conditions I am not concerned about speed. 

3.)
>A fragment should be applied if one of its conditions
>are not met according to context.
But that's not what the code does. Maybe you mean:
A fragment should be applied UNLESS one of its conditions
are not met according to context.

I think the description says what it does. if there are no
conditions it is a vacuous truth.

4.) I will change value to booleanValue I will also rename context to vdsmConfiguration.

5.) Good idea! will change order.
 -- interface functions
 -- private functions
 -- private     # On configure/removeConf 
 (function called from huge configuration)
 -- configuration
Line 394: 
Line 395:     def _openConfig(self, path):
Line 396:         return ConfigFile(path, **self.VDSM_CONF_SECTION)
Line 397: 


-- 
To view, visit http://gerrit.ovirt.org/27298
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I74bfe05bb4b5f5d09021f21b324f9b7d5d0fdaab
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: mooli tayer <mtayer at redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alonbl at redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: Zhou Zheng Sheng <zhshzhou at linux.vnet.ibm.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list