Shahar Havivi has posted comments on this change. Change subject: External hypervisor VMs integration ......................................................................
Patch Set 16: (7 comments) http://gerrit.ovirt.org/#/c/33309/16/tests/v2vTests.py File tests/v2vTests.py: Line 82: @MonkeyPatch(libvirtconnection, 'open_connection', hypervisorConnect) Line 83: def testGetExternalVMList(self): Line 84: vm = v2v.getExternalVMList('esx://mydomain', 'user', Line 85: 'password')[0] Line 86: self.assertTrue(vm is not None) > How can vm be None? no... need to check the the getExternalVMList count is one Line 87: self.assertEquals(vm['vmId'], '564d7cb4-8e3d-06ec-ce82-7b2b13c6a611') Line 88: self.assertEquals(vm['memSize'], 2048) Line 89: self.assertEquals(vm['smp'], 1) Line 90: self.assertTrue(vm['disks'] is not None) Line 86: self.assertTrue(vm is not None) Line 87: self.assertEquals(vm['vmId'], '564d7cb4-8e3d-06ec-ce82-7b2b13c6a611') Line 88: self.assertEquals(vm['memSize'], 2048) Line 89: self.assertEquals(vm['smp'], 1) Line 90: self.assertTrue(vm['disks'] is not None) > How can disks be None? Done Line 91: self.assertTrue(vm['networks'] is not None) Line 92: disk = vm['disks'][0] Line 93: self.assertEquals(disk['dev'], 'sda') Line 94: self.assertEquals(disk['alias'], '[datastore1] RHEL/RHEL.vmdk') Line 87: self.assertEquals(vm['vmId'], '564d7cb4-8e3d-06ec-ce82-7b2b13c6a611') Line 88: self.assertEquals(vm['memSize'], 2048) Line 89: self.assertEquals(vm['smp'], 1) Line 90: self.assertTrue(vm['disks'] is not None) Line 91: self.assertTrue(vm['networks'] is not None) > Same Done Line 92: disk = vm['disks'][0] Line 93: self.assertEquals(disk['dev'], 'sda') Line 94: self.assertEquals(disk['alias'], '[datastore1] RHEL/RHEL.vmdk') Line 95: network = vm['networks'][0] http://gerrit.ovirt.org/#/c/33309/16/vdsm/v2v.py File vdsm/v2v.py: Line 63: return size * 1024 Line 64: elif unit in ('TiB', 'T'): Line 65: return size * 1024 * 1024 Line 66: else: Line 67: raise InvalidVMConfigurationException('Invalid memory unit: ' + unit) > Better use "description: %r" % value, so we can see what was the bad value. Done Line 68: Line 69: Line 70: def _addGeneralInfo(root, params): Line 71: e = root.find('./uuid') Line 75: e = root.find('./currentMemory') Line 76: if e is not None: Line 77: try: Line 78: size = int(e.text) Line 79: except: > Handle only ValueError - any other error is not expected. Done Line 80: raise InvalidVMConfigurationException('Invalid memory value: ' + Line 81: e.text) Line 82: unit = e.get('unit', 'KiB') Line 83: params['memSize'] = _memToMb(size, unit) Line 123: i['dev'] = target.get('dev') Line 124: params['networks'].append(i) Line 125: Line 126: Line 127: class InvalidVMConfigurationException(Exception): > No need to add "Exception" to the name. What elese can be "InvalidVMConfigu Done Line 128: def __init__(self, message): Line 129: self.message = message Line 130: Line 131: def __str__(self): Line 128: def __init__(self, message): Line 129: self.message = message Line 130: Line 131: def __str__(self): Line 132: return self.message > It is better not to implement __str__ since Exception builtin implementatio Done -- To view, visit http://gerrit.ovirt.org/33309 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7dcfb860626a844d1d08590274b508519a33f4a3 Gerrit-PatchSet: 16 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Antoni Segura Puimedon <asegu...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org 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
