Zhou Zheng Sheng has posted comments on this change.
Change subject: Introducing configurator package in vdsm-tool
......................................................................
Patch Set 15:
(4 comments)
....................................................
Commit Message
Line 5: CommitDate: 2013-10-21 16:11:52 +0300
Line 6:
Line 7: Introducing configurator package in vdsm-tool
Line 8:
Line 9: This package union libvirt and sanlock configuration operations for now.
I think "union" is a noun. I guess you wanted to write "unites libvirt and
sanlock", or "This package is a union of libvirt and sanlock ....".
And what does "for now" means exactly? Do we have another plan in future? I
guess you meant "unite libvirt and sanlock configuration from present
implementations".
Line 10: The package defines ModuleConfigure interface and API for modules
configuration
Line 11: tool. The format for the configure command is modified to:
Line 12:
Line 13: vdsm-tool configure --modules libvirt,sanlock --force --autoreset
Line 11: tool. The format for the configure command is modified to:
Line 12:
Line 13: vdsm-tool configure --modules libvirt,sanlock --force --autoreset
Line 14:
Line 15: --dry option is available to check if configure is required
"--dry" usually means it does not touch the actual files and let the use
examine the result. Using "--dry" to test if it is configured is not a straight
forward use, kindly like a side effect.
For me, better names are "--probe"/"--test" for configuration detection, and
"--verify"/"--validate" for checking conflicts.
Line 16: and --test to verify current configuration's conflicts.
Line 17:
Line 18: For reuse and easier additional configurers for other services, all the
Line 19: developer needs to add is an object that inherit ModuleConfigure and
....................................................
File lib/vdsm/tool/configurator.py
Line 43: raise RuntimeError("Module %s is up. To force
reconfiguration use "
Line 44: "--force flag")
Line 45: else:
Line 46: ret = 0
Line 47: return ret
This return value is not used by any caller. If you are using exception here,
I'd suggest the following style: when configure fails, raises an exception,
when it succeeds, returns nothing.
Line 48:
Line 49: def isconfigured(self):
Line 50: return 0
Line 51:
Line 46: ret = 0
Line 47: return ret
Line 48:
Line 49: def isconfigured(self):
Line 50: return 0
+1
Line 51:
Line 52: def getName(self):
Line 53: return None
Line 54:
--
To view, visit http://gerrit.ovirt.org/20100
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I16bf5894e7e55a84b4c2a0caacde383ae7c19242
Gerrit-PatchSet: 15
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybron...@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com>
Gerrit-Reviewer: Zhou Zheng Sheng <zhshz...@linux.vnet.ibm.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
