Yaniv Bronhaim has posted comments on this change. Change subject: Initial commit for vdsm-tool testing infrastructure ......................................................................
Patch Set 10: (4 comments) http://gerrit.ovirt.org/#/c/25263/10/.gitignore File .gitignore: Line 33: init/upstart/vdsmd.upstart Line 34: init/vdsmd_init_common.sh Line 35: lib/vdsm/config.py Line 36: lib/vdsm/constants.py Line 37: lib/vdsm/tool/tests/logrotate-libvirtd > why are you creating these files in tree? you should run the test in a temp oh, i changed it and forgot to remove those Line 38: lib/vdsm/tool/tests/libvirtd.conf Line 39: lib/vdsm/tool/tests/libvirtd Line 40: lib/vdsm/tool/tests/qemu.conf Line 41: lib/vdsm/tool/tests/qemu-sanlock.conf http://gerrit.ovirt.org/#/c/25263/10/lib/vdsm/tool/configurator.py File lib/vdsm/tool/configurator.py: Line 131: if not self.testenv and os.getuid() != 0: Line 132: raise UserWarning("Must run as root") Line 133: Line 134: rc, out, err = utils.execCmd( Line 135: [ > why not tupple? with tupple the execCmd with sudo fails when it tries to add the sudo parameters to the command (TypeError: can only concatenate tuple (not "list") to tuple) im leaving this change fix here. Line 136: '/usr/sbin/usermod', Line 137: '-a', Line 138: '-G', Line 139: ','.join(self.SANLOCK_GROUPS), Line 139: ','.join(self.SANLOCK_GROUPS), Line 140: 'sanlock' Line 141: ], Line 142: raw=True, Line 143: sudo=True if self.testenv else False, > Why run this configurator at all during tests? I find it a bit rude. how will i test this part ? I just won't?.. any better ideas? but i agree with you, and i want this patch in asap to make it easier for others to add more tool tests, so until finding better way to test sanlock config part im removing it. Line 144: ) Line 145: sys.stdout.write(out) Line 146: sys.stderr.write(err) Line 147: if rc != 0: http://gerrit.ovirt.org/#/c/25263/10/lib/vdsm/tool/libvirt_configure.sh.in File lib/vdsm/tool/libvirt_configure.sh.in: Line 301: rm -f "${lnetwork}" Line 302: fi Line 303: Line 304: if [ "${LIBVIRT_LOGROTATE}" = '' ]; then Line 305: local llogr=/etc/logrotate.d/libvirtd > this is equivalent to much better. thanks! Line 306: else Line 307: local llogr="${LIBVIRT_LOGROTATE}" Line 308: fi Line 309: -- To view, visit http://gerrit.ovirt.org/25263 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iae0fec9b2057c5ba932f38ed5d34d654486043ef Gerrit-PatchSet: 10 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: mooli tayer <[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
