Francesco Romani has posted comments on this change. Change subject: v2v: Import VM from OVA file ......................................................................
Patch Set 6: Code-Review-1 (2 comments) On overall, looks good. But a couple of questions inside. -1 for visibility https://gerrit.ovirt.org/#/c/43367/6/vdsm/v2v.py File vdsm/v2v.py: Line 164: Line 165: Line 166: def convert_external_vm(uri, username, password, vminfo, job_id, irs): Line 167: job = ImportVm.from_libvirt(uri, username, password, vminfo, job_id, irs) Line 168: job.start_libvirt() I don't really like this. It partially defeats the purpose of factory functions (ImportVm.from_*) Can't we hide the indirection inside the class? so from the outside we see job = ImportVm.from_*(parameters) job.start() _add_job(job_id, job) return response.success() Line 169: _add_job(job_id, job) Line 170: return {'status': doneCode} Line 171: Line 172: Line 294: @contextmanager Line 295: def password_file(job_id, file_name, password): Line 296: if file_name is None: Line 297: yield Line 298: return still needed? Line 299: fd = os.open(file_name, os.O_WRONLY | os.O_CREAT, 0o600) Line 300: try: Line 301: os.write(fd, password.value) Line 302: finally: -- To view, visit https://gerrit.ovirt.org/43367 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0e440748ecc503f4d61e8f4f61bb0c7387589354 Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek <mskri...@redhat.com> 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
