Dan Kenigsberg has posted comments on this change. Change subject: util: fix execCmd to accept tuples ......................................................................
Patch Set 4: Code-Review-1 (3 comments) http://gerrit.ovirt.org/#/c/26070/4/lib/vdsm/utils.py File lib/vdsm/utils.py: Line 53: import threading Line 54: import time Line 55: import zombiereaper Line 56: Line 57: import cpopen I do not mind this change, but it's unrelated to this patch. Line 58: from .config import config Line 59: from . import constants Line 60: Line 61: # Buffsize is 1K because I tested it on some use cases and 1K was fastest. If http://gerrit.ovirt.org/#/c/26070/4/tests/utilsTests.py File tests/utilsTests.py: Line 24: import logging Line 25: Line 26: from testrunner import VdsmTestCase as TestCaseBase Line 27: from vdsm import utils Line 28: from vdsm import constants It's not completely your fault, but please separate: * stdlib * out-of-vdsm * vdsm-internal and keep each group sorted. Line 29: import time Line 30: from monkeypatch import Patch Line 31: import cpopen Line 32: Line 469: Line 470: def __init__(self, *args, **kwargs): Line 471: TestCaseBase.__init__(self, *args, **kwargs) Line 472: Line 473: self.patch = Patch([(cpopen, 'CPopen', self.patchedCPopen), I feel responsible, having suggested monkey-patching... However I must say that this unit test goes too deep into replicating the implementation of execCmd. It would be much better to have a black-box test, verifying that (echo, hello, world) executes as it should, and that touching a root-owned directory works with sudo and fails without it. Line 474: (utils, 'AsyncProc', self.patchedAsyncProc)]) Line 475: Line 476: def setUp(self): Line 477: self.patch.apply() -- To view, visit http://gerrit.ovirt.org/26070 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8d1269d9a26b1fef1552976ae626cf4596471283 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Dima Kuznetsov <[email protected]> Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Saggi Mizrahi <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
