[node-patches] Change in ovirt-node[master]: Add a diagnostic TUI page to view disk information

[fabiand at fedoraproject.org]

Fabian Deutsch has posted comments on this change.

Change subject: Add a diagnostic TUI page to view disk information
......................................................................


Patch Set 3: I would prefer that you didn't submit this

(3 inline comments)

I'n general nice, I just found a coule of smaller things.

....................................................
File scripts/tui/src/ovirt/node/setup/diagnostic_page.py
Line 50:               ui.Label("diagnostic.info", "Select one of the tools below. \n" +
Line 51:                        "Press 'q' to quit when viewing output"),
Line 52:               ui.Divider("diagnostic.divider"),
Line 53:               ui.Table("diagnostic.tools", "", "Available diagnostics",
Line 54:                        self.__diagnostics(), height=len(self.__diagnostics())),
height=len(...)

This might brake the usability on small screens, did you test it with a default ... 24rows height?

I think we should set an upper limit for height here. so min(len(...), UPPER_LIMIT)
Line 55:               ]
Line 56: 
Line 57:         page = ui.Page("page", ws)
Line 58:         self.widgets.add(page)


Line 68:             if "diagnostic.tools" in changed_field:
Line 69:                 cmds = {"multipath":"multipath -ll | less",
Line 70:                         "fdisk":"fdisk -l | less",
Line 71:                         "parted":"parted -l | less"}
Line 72:             cmd = cmds[changes[changed_field]] if changes[changed_field] in cmds else None
you can use: cmd = cmds.get(changes[changed_field], None) here
Line 73:             if cmd:
Line 74:                 with self.application.ui.suspended():
Line 75:                     utils.process.call("reset")
Line 76:                     utils.process.call(cmd)


Line 74:                 with self.application.ui.suspended():
Line 75:                     utils.process.call("reset")
Line 76:                     utils.process.call(cmd)
Line 77: 
Line 78:     def __diagnostics(self):
Theinformation seems to be a bit redundant to the informations in line 69 - maybe you could combine these two blocks.
Line 79:         return [("multipath", "multipath -ll"),
Line 80:                 ("fdisk", "fdisk -l"),


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia6055bca2369398e2050561fa9388f3a4dfb8535
Gerrit-PatchSet: 3
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: Ryan Barry <rbarry at redhat.com>