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

mtayer at redhat.com mtayer at redhat.com
Thu May 15 13:07:26 UTC 2014


mooli tayer has posted comments on this change.

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


Patch Set 8:

(1 comment)

>ok, than I would suggest to:
>1. Rename allTrue to needsUpdate - because it seems that this is what it means.

How about 'applyFragment'?

>2. When you check for inequality use != instead of not:
>    if context[key] != val:
>        needsUpdate = True

OK

>3. Even cleaner: factor out the check to separate function
>   def needsUpdate(a, b):
>       for key, val in b.iteritems():
>           if a[key] != val:
>               return True
>        return False

OK

>And then your loop becomes:
>    for fragment in content['fragments']:
>        if self.needsUpdate(fragment['conditions'], context):
>            configuration.update(fragment['content'])

OK

>4. And docstring telling what this method does cannot hurt - as you see it fooled me.

Very well

>5. The method name is confusing - this is not about adding sections but about updating them, right?

What it ultimatly does is add one section in a configuration file that look something like:

## beginning of configuration section by vdsm-4.13.0
key=val
...
## end of configuration section by vdsm-4.13.0
That section is created by updating maps but the importent line is:
conff.addEntry(key, val)
these entries create the new section

http://gerrit.ovirt.org/#/c/27298/8//COMMIT_MSG
Commit Message:

Line 3: AuthorDate: 2014-04-30 14:29:36 +0300
Line 4: Commit:     Mooli Tayer <mtayer at redhat.com>
Line 5: CommitDate: 2014-05-14 14:12:24 +0300
Line 6: 
Line 7: replace configure_libvirt.py with python code.
> It would be helpful to describe here why this change is needed, and if this
OK i'm writing a very descriptive (hope so) one.
Line 8: 
Line 9: Change-Id: I74bfe05bb4b5f5d09021f21b324f9b7d5d0fdaab


-- 
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: 8
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: 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