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