Francesco Romani has posted comments on this change. Change subject: v2v: ova should support zip and extracted directory formats ......................................................................
Patch Set 2: Code-Review-1 (2 comments) nice, a couple more of possible improvements. https://gerrit.ovirt.org/#/c/48129/2/vdsm/v2v.py File vdsm/v2v.py: Line 751: Line 752: Line 753: def _read_ovf_from_ova_dir(ova_path): Line 754: files = os.listdir(ova_path) Line 755: for ova_file in files: please see also comment at line 766 which may make this irrelevant minor: here you use ova_file; below you use ova_file. The difference is just one char. Maybe using for entry in files: if '.ovf' == os.path.splitext(entry)[1].lower(): with open(os.path.join(ova_path, entry), 'r') as ovf_file: return ovf_file.read() leads to better code. Your call, as you feels is most fit. Line 756: if '.ovf' == os.path.splitext(ova_file)[1].lower(): Line 757: with open(os.path.join(ova_path, ova_file), 'r') as ovf_file: Line 758: return ovf_file.read() Line 759: raise ClientError('OVA directory %s does not contain ovf file' % ova_path) Line 762: def _read_ovf_from_zip_ova(ova_path): Line 763: with open(ova_path, 'rb') as fh: Line 764: zf = zipfile.ZipFile(fh) Line 765: for name in zf.namelist(): Line 766: if '.ovf' == os.path.splitext(name)[1].lower(): we can extract one helper here: def _find_ovf(entries): for entry in entries: if '.ovf' == os.path.splitext(entry)[1].lower(): return entry return None use it like: def _read_ovf_from_zip_ova(ova_path): with open(ova_path, 'rb') as fh: zf = zipfile.ZipFile(fh) name = _find_ovf(zf.namelist()) if name is not None: return zf.read(name) raise ClientError('OVA does not contains file with .ovf suffix') and, above: def _read_ovf_from_ova_dir(ova_path): name = _find_ovf(ova_path) if name is not None: with open(os.path.join(ova_path, name), 'r') as ovf_file: return ovf_file.read() raise ClientError('OVA directory %s does not contain ovf file' % ova_path) Line 767: return zf.read(name) Line 768: raise ClientError('OVA does not contains file with .ovf suffix') Line 769: Line 770: -- To view, visit https://gerrit.ovirt.org/48129 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I494b88709c4c23fd39690b589eff1134e74f81ba Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches