Nir Soffer has posted comments on this change. Change subject: qemuimg: Add qcow_compat optional parameter. ......................................................................
Patch Set 12: (4 comments) https://gerrit.ovirt.org/#/c/64169/12/lib/vdsm/qemuimg.py File lib/vdsm/qemuimg.py: Line 110: cmd.extend(("-f", format)) Line 111: if format == FORMAT.QCOW2: Line 112: if qcow2_compat is None: Line 113: qcow2_compat = default_qcow2_compat() Line 114: if qcow2_compat in ("0.10", "1.1"): We have a constant for this. Line 115: cmd.extend(('-o', 'compat=' + qcow2_compat)) Line 116: else: Line 117: raise ValueError("Invalid compat version '%s'" % qcow2_compat) Line 118: Line 113: qcow2_compat = default_qcow2_compat() Line 114: if qcow2_compat in ("0.10", "1.1"): Line 115: cmd.extend(('-o', 'compat=' + qcow2_compat)) Line 116: else: Line 117: raise ValueError("Invalid compat version '%s'" % qcow2_compat) Use %r instead of '%s'. Please always use the happy path idiom: if qcow2_compat not in _QCOW2_COMPAT_SUPPORTED: raise ... normal flow Line 118: Line 119: if backing: Line 120: if not os.path.isabs(backing): Line 121: cwdPath = os.path.dirname(image) Line 176: qcow2_compat = default_qcow2_compat() Line 177: if qcow2_compat in ("0.10", "1.1"): Line 178: cmd.extend(('-o', 'compat=' + qcow2_compat)) Line 179: else: Line 180: raise ValueError("Invalid compat version '%s'" % qcow2_compat) We do this logic twice - maybe extract a little helper for this, so we can write: qcow2_compat = _validate_qcow2_compat(qcow2_compat) None will return the default format, and invalid value will raise. Line 181: Line 182: if backing: Line 183: if not os.path.isabs(backing): Line 184: cwdPath = os.path.dirname(srcImage) https://gerrit.ovirt.org/#/c/64169/12/tests/qemuimg_test.py File tests/qemuimg_test.py: Line 170: qemuimg.create('image', format='qcow2') Line 171: Line 172: def test_qcow2_compat_invalid(self): Line 173: with self.assertRaises(ValueError): Line 174: qemuimg.create('image', format='qcow2', qcow2_compat='1.11') Nice! But we should test also convert. Then we should test also both valid value and None, in both create and convert. Line 175: Line 176: def test_invalid_config(self): Line 177: config = make_config([('irs', 'qcow2_compat', '1.2')]) Line 178: with MonkeyPatchScope([(qemuimg, 'config', config)]): -- To view, visit https://gerrit.ovirt.org/64169 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8bbf8a60d0af1f99b3fae2c30ac06b36d5986180 Gerrit-PatchSet: 12 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Adam Litke <ali...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Yaniv Kaul <yk...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org
