[node-patches] Change in ovirt-node[master]: Prevent rsyslog and netconsole configuration before networki...
fabiand at fedoraproject.org
fabiand at fedoraproject.org
Mon Feb 25 09:30:49 UTC 2013
Fabian Deutsch has posted comments on this change.
Change subject: Prevent rsyslog and netconsole configuration before networking. Add a Notice UI widget that prints red messages for above.
......................................................................
Patch Set 3: I would prefer that you didn't submit this
(13 inline comments)
In general quite nice - I like that you added the Notice widget - there are only a couple of things which need some attention.
....................................................
File scripts/tui/src/ovirt/node/setup/logging_page.py
Line 63: "netconsole.port": valid.Port(),
Line 64: }
Line 65:
Line 66: def ui_content(self):
Line 67:
Please remove the whitespaces in empty lines
Line 68: ws = [ui.Header("header[0]", "Logging"),
Line 69: ui.Entry("logrotate.max_size", "Logrotate Max Log " +
Line 70: "Size (KB):"),
Line 71: ui.Divider("divider[0]")
Line 69: ui.Entry("logrotate.max_size", "Logrotate Max Log " +
Line 70: "Size (KB):"),
Line 71: ui.Divider("divider[0]")
Line 72: ]
Line 73: if not self._network_configured():
Also remove trailing white spaces
git diff highlights such cases
Line 74: ws.extend([ui.Notice("network.notice","Networking is not configured, " +
Line 75: "please configure it before rsyslog and/or netconsole"),
Line 76: ui.Divider("notice.divider")])
Line 77: ws.extend([ui.Label("rsyslog.header", "RSyslog is an enhanced multi-" +
Line 71: ui.Divider("divider[0]")
Line 72: ]
Line 73: if not self._network_configured():
Line 74: ws.extend([ui.Notice("network.notice","Networking is not configured, " +
Line 75: "please configure it before rsyslog and/or netconsole"),
This and many of the following lines are to long (max. 80chars, including the new line break)
You could use a variable for self._network_configured() to reuced the length
Line 76: ui.Divider("notice.divider")])
Line 77: ws.extend([ui.Label("rsyslog.header", "RSyslog is an enhanced multi-" +
Line 78: "threaded syslogd"),
Line 79: ui.Entry("rsyslog.address", "Server Address:",enabled=self._network_configured()),
Line 126: txs += model.transaction()
Line 127:
Line 128: progress_dialog = ui.TransactionProgressDialog("dialog.txs", txs, self)
Line 129: progress_dialog.run()
Line 130:
White spaces
Line 131: def _network_configured(self):
Line 127:
Line 128: progress_dialog = ui.TransactionProgressDialog("dialog.txs", txs, self)
Line 129: progress_dialog.run()
Line 130:
Line 131: def _network_configured(self):
I'd love to see/move this function in/to utils.network, e.g. utils.network.is_configured()
....................................................
File scripts/tui/src/ovirt/node/ui/__init__.py
Line 215: return self._text
Line 216:
Line 217: def value(self, txt=None):
Line 218: return self.text(txt)
Line 219:
Please remove the white spaces
Line 220: class Notice(Label):
Line 221: def __init__(self,path,text):
Line 222: super(Notice, self).__init__(path, text)
Line 223:
Line 735: raise NotImplementedError
Line 736:
Line 737: def _build_header(self, ui_header):
Line 738: raise NotImplementedError
Line 739:
Please remove the white spaces
Line 740: def _build_notice(self, ui_notice):
Line 741: raise NotImplementedError
Line 742:
Line 743: def _build_button(self, ui_button):
....................................................
File scripts/tui/src/ovirt/node/ui/urwid_builder.py
Line 65: widget = uw.Header(ui_label.text(),
Line 66: ui_label.template)
Line 67: elif type(ui_label) is ui.Notice:
Line 68: widget = uw.Notice(ui_label.text())
Line 69:
Please remove the white spaces
Line 70: else:
Line 71: widget = uw.Label(ui_label.text())
Line 72:
Line 73: def on_item_text_change_cb(w, v):
Line 83: return self._build_label(ui_keywordlabel)
Line 84:
Line 85: def _build_header(self, ui_header):
Line 86: return self._build_label(ui_header)
Line 87:
Please remove the white spaces
Line 88: def _build_notice(self, ui_notice):
Line 89: return self._build_label(ui_notice)
Line 90:
Line 91: def _build_button(self, ui_button):
....................................................
File scripts/tui/src/ovirt/node/ui/widgets.py
Line 231: return self._label.get_text()
Line 232:
Line 233: def set_text(self, txt):
Line 234: self.text(txt)
Line 235:
Please remove the white spaces
And you need two blank lines between classes
Line 236: class Notice(Label):
Line 237: """A read only widget for notices
Line 238: """
Line 239: _notice_attr = "plugin.widget.notice"
Line 236: class Notice(Label):
Line 237: """A read only widget for notices
Line 238: """
Line 239: _notice_attr = "plugin.widget.notice"
Line 240:
Please remove the white spaces
Line 241: def __init__(self,text):
Line 242: super(Notice, self).__init__(text)
Line 243: self._label_attrmap.set_attr_map({None: self._notice_attr})
Line 244:
Line 237: """A read only widget for notices
Line 238: """
Line 239: _notice_attr = "plugin.widget.notice"
Line 240:
Line 241: def __init__(self,text):
There is a whitespace missing between the comma and text (self, text)
Line 242: super(Notice, self).__init__(text)
Line 243: self._label_attrmap.set_attr_map({None: self._notice_attr})
Line 244:
Line 245: class Header(Label):
Line 331: linebox_attr_map = {None: "plugin.widget.entry.frame"}
Line 332: if not is_enabled:
Line 333: edit_attr_map = {None: "plugin.widget.entry.disabled"}
Line 334: linebox_attr_map = {None: "plugin.widget.entry.frame.disabled"}
Line 335: self._label_attrmap.set_attr_map(edit_attr_map)
What do you want to achieve with this change? Isn't this affecting the Entry widget?
Line 336: self._edit_attrmap.set_attr_map(edit_attr_map)
Line 337: self._linebox_attrmap.set_attr_map(linebox_attr_map)
Line 338:
Line 339: def valid(self, is_valid):
--
To view, visit http://gerrit.ovirt.org/12338
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I53a264972a40e3f3ab80bb4c90c506622bb89444
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>
More information about the node-patches
mailing list