[node-patches] Change in ovirt-node[master]: Add a confirmation page

[fabiand at fedoraproject.org]

Fabian Deutsch has posted comments on this change.

Change subject: Add a confirmation page
......................................................................


Patch Set 5: Code-Review-1

(2 comments)

http://gerrit.ovirt.org/#/c/27074/5/src/ovirt/node/installer/core/confirmation_page.py
File src/ovirt/node/installer/core/confirmation_page.py:

Line 34:         super(Plugin, self).__init__(app)
Line 35:         self.storage_discovery = StorageDiscovery(app.args.dry)
Line 36:         self.storage_discovery.start()
Line 37:         self._header = "{!s:8.8} {!s:48.48} {!s:9.9}"
Line 38:         self._tagged_pvs = [x.split()[0] for x in
Please move some of this code into a class, to make it more programatically accessible (will be easier to add unit-tests/doctests).

utils/system.py

    class LVM(…):
        def vgs(self):
            return [LVM.VG(
        class VG(…):
            def tags(self):
                return …

then

    def is_tagged(name):
        if name in LVM().vgs() and "storage_domain" LVM.vg(name).tags()

or something along this lines.

Mainly to keep much of the low-level logic in the utils class.

Basically split business/domain knowledge (if tagged storage_domain, then it's in use) here, and the low-level/generic helper stuff in the utils/* classes
Line 39:                             process.check_output(["lvm", "vgs",
Line 40:                                                   "--noheadings",
Line 41:                                                   "-o", "pv_name,tags"]).split(
Line 42:                                 "\n") if "storage_domain" in x]


Line 60:         align = lambda l: l.ljust(16)
Line 61:         if not self._model:
Line 62:             self._build_model()
Line 63: 
Line 64:         ws = [ui.Header("header[0]", _("Confirm disk selections")),
Maybe we should add a more inforamtionl text like

"Please review this page carefully. The data on the disks below will be erased"

Or something like this. you can also set the UX keyword on the bug and someone from teh UX team can take a look.
Line 65:               ui.Header("boot.header", _("Boot device")),
Line 66:               DiskDetails("boot.device", self,
Line 67:                           self._model["boot.device.current"])]
Line 68: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If0c099fbfbfda9bc609a024964905ee59e04280f
Gerrit-PatchSet: 5
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>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes