Alon Bar-Lev has posted comments on this change. Change subject: hooks: Adding hooks for ovirt-node-upgrade ......................................................................
Patch Set 5: (9 comments) http://gerrit.ovirt.org/#/c/28251/5/Makefile.am File Makefile.am: Line 17 Line 18 Line 19 Line 20 Line 21 automake should avoid gmake specific syntax. Line 19 Line 20 Line 21 Line 22 Line 23 any reason you use bash? I even do not see it actually used properly. Line 29 Line 30 Line 31 Line 32 Line 33 in future patch, please fix the indent to be tab per consistency. Line 44 Line 45 Line 46 Line 47 Line 48 usually, you should create srpm and out of it the rpm. not sure why standard commands are within Makefile... we try to avoid these in newer projects. Line 49 Line 50 Line 51 Line 52 Line 53 why shell? $$(xxx) should have same effect. Line 50 Line 51 Line 52 Line 53 Line 54 highly non standard target.... regardless of this patch. Line 99 Line 100 Line 101 Line 102 Line 103 these should be scripts like we have done at engine/otopi/etc... http://gerrit.ovirt.org/#/c/28251/5/hooks/pre-upgrade/01-vdsm File hooks/pre-upgrade/01-vdsm: Line 20: rc, out, err = utils.execCmd( Line 21: ['vdsm-tool', 'service-stop', 'vdsmd'], Line 22: sudo=True, Line 23: raw=True Line 24: ) Please print: Stopping vdsm. And also: Failed to stop vdsm, error is: As we need context for messages. Line 25: sys.stdout.write(out) Line 26: sys.stderr.write(err) Line 27: Line 28: return rc http://gerrit.ovirt.org/#/c/28251/5/ovirt-node-plugin-vdsm.spec.in File ovirt-node-plugin-vdsm.spec.in: Line 72: %{recipe_root} Line 73: Line 74: %files Line 75: %{python_sitelib}/ovirt/node/setup/vdsm Line 76: %attr(755, root, root) %{_libexecdir}/ovirt-node/hooks/pre-upgrade/01-vdsm %attr(755, root, root) should be default, taken from filesystem, if there is an issue it should be solved differently, like _SCRIPTS suffix instead of _PYTHON. anyway, root can be probably replaced with - Line 77: %{_sysconfdir}/ovirt-plugins.d Line 78: Line 79: %changelog Line 80: * Fri May 17 2013 Mike Burns <[email protected]> 0.0.2 -- To view, visit http://gerrit.ovirt.org/28251 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I986fe6edb8f5619dcbca33cccab080245654fd02 Gerrit-PatchSet: 5 Gerrit-Project: ovirt-node-plugin-vdsm Gerrit-Branch: master Gerrit-Owner: Douglas Schilling Landgraf <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Douglas Schilling Landgraf <[email protected]> Gerrit-Reviewer: Fabian Deutsch <[email protected]> Gerrit-Reviewer: Joey Boggs <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
