Dan Kenigsberg has posted comments on this change. Change subject: Move validate_libvirt_certs to vdsm-tool ......................................................................
Patch Set 4: I would prefer that you didn't submit this (1 inline comment) it's a grand step forward, but I wouldn't like to take this sys.path mangling. I hope you agree to have a simple re-implementation of execCmd. A better solution would be to take execCmd out of the storage package, and make it a top-level package. but that may be too much work for now .................................................... File vdsm-tool/validate_ovirt_certs.py.in Line 26: sys.path.append("%s" % constants.P_VDSM) ok, you've found a bug in vdsm.execCmd - it requires an implementation that is outside site-packages. that's bad (and not so easy to fix). Still, messing with sys.path within a module is a bad practice. I'm afraid that we would have to revert to a simple wrapper around subprocess.Popen.communicate(). But please define it here, and don't catch exceptions within it. -- To view, visit http://gerrit.ovirt.org/5961 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5b6504ae2e3c8ffa0c33d7ba22a7c16597a51945 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Wenyi Gao <we...@linux.vnet.ibm.com> Gerrit-Reviewer: Adam Litke <a...@us.ibm.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Federico Simoncelli <fsimo...@redhat.com> Gerrit-Reviewer: Wenyi Gao <we...@linux.vnet.ibm.com> _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches