Sergey Gotliv has posted comments on this change. Change subject: vm: Unify checks for vdsm image ......................................................................
Patch Set 2: (1 comment) .................................................... File vdsm/vm.py Line 88: :type drive: dict or vm.Drive Line 89: :return: bool Line 90: """ Line 91: if drive['device'] != 'disk': Line 92: return False @Nir, >'m not arguing if the device == 'disk' check is needed or not - this patch is >not about >removing unneeded checks but about removing code duplication, make >the code >simpler to understand and easier to use. I have no problem with unifying both isVdsmImage() methods and avoiding code duplication! I am really fine with that and will give this part +1. I am against adding "device == disk" part to the isVdsmImage - it has nothing to do with UNIFYING or VdsmImage definition (please read my previous comment). * Regarding http://gerrit.ovirt.org/22363 let's keep discussion there. I mentioned it here just to make reviewers of this patch aware of it, so the can review it either. For example, if this patch wouldn't contain this additional check inside of isVdsmImage my mention of 22363 would become totally irrelevant. Line 93: required = ('volumeID', 'domainID', 'imageID', 'poolID') Line 94: return all(k in drive for k in required) Line 95: Line 96: -- To view, visit http://gerrit.ovirt.org/22370 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iaa109a19aeec56f09ed2e6d64dee636c7779d426 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Ayal Baron <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Michal Skrivanek <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Sergey Gotliv <[email protected]> Gerrit-Reviewer: Vinzenz Feenstra <[email protected]> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
