[node-patches] Change in ovirt-node[master]: Create a basic plugin for IPMI enablement

[fabiand at fedoraproject.org]

Fabian Deutsch has posted comments on this change.

Change subject: Create a basic plugin for IPMI enablement
......................................................................


Patch Set 2: Code-Review-1

(3 comments)

....................................................
File src/ovirt/node/setup/ipmi/ipmi_page.py
Line 57:             ws.extend([
Line 58:                 ui.Divider("divider[0]"),
Line 59:                 ui.Header("header[1]", "Fan Status:"),
Line 60:                 ui.Table("ipmi_output", "", "Fan status",
Line 61:                          process.check_output(
In the sense of separataion of concerns the calls to th eexternal ipmi tool should be wrapped in class (like we already do for several other tools in ovirt.node.utils).
E.g:

    class Ipmi(base.Base):
        def _ipmi(args):
            assert type(args) is list
            return process.check_output(["ipmitool"] + args, stderr=process.PIPE)
        def fan_status(self):
            ...

All stdout post processing and exception handling should happen there
Line 62:                              "ipmitool -I open sdr elist full",
Line 63:                              shell=True))
Line 64:             ])
Line 65: 


Line 62:                              "ipmitool -I open sdr elist full",
Line 63:                              shell=True))
Line 64:             ])
Line 65: 
Line 66:         page = ui.Page("page", ws)
The page should have no buttons - as there is nothing to save/reset.
Line 67:         self.widgets.add(ws)
Line 68:         return page
Line 69: 
Line 70:     def on_change(self, changes):


Line 74:         pass
Line 75: 
Line 76:     def _check_ipmi(self):
Line 77:         try:
Line 78:             process.check_call("ipmitool -I open chassis status", shell=True)
Please use the list syntax. That is more safe then using a shell.
E.g.:

    process.check_call(["ipmitool", "-I", "open", "chassis", "status"])

And this function should also be a member of the wrapper class mentioned above.
Line 79:             return True
Line 80:         except CalledProcessError:


-- 
To view, visit http://gerrit.ovirt.org/17985
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I74c53ffec3f9eb11312ccffec7c5b7083325d94a
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-node
Gerrit-Branch: master
Gerrit-Owner: Ryan Barry <rbarry at redhat.com>
Gerrit-Reviewer: Fabian Deutsch <fabiand at fedoraproject.org>
Gerrit-Reviewer: Joey Boggs <jboggs at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes