[master/rhel7-branch][PATCH] Add support for network configuration in TUI. (#909299)
Samantha N. Bueno
sbueno+anaconda at redhat.com
Thu Aug 22 03:11:06 UTC 2013
On Wed, Aug 21, 2013 at 03:02:43PM +0200, Radek Vykydal wrote:
> Samantha, thanks for the review.
>
> I'd propose to send our squashed patches (with your remarks addressed) for
> review so that others can see into it too. I may create a video.
> If they are OK to push,I'd commit the two patches separately.
> What do you think?
That sounds like a good idea to me.
> On 08/20/2013 07:08 PM, Samantha N. Bueno wrote:
> >On Fri, Aug 16, 2013 at 04:03:38PM +0200, Radek Vykydal wrote:
> >>So this is the version I'd post applied to your patch for review
> >>- the amount of changes is such that I think it deserves to be
> >>reviewed squashed with Samantha's original patch.
> >Thanks; I'm just going to paste snippets where I've commented to avoid
> >sending a huge long email:
> >
> >- def _update_summary(self):
> >- """ Update summary screen with current dev info and hostname. """
> >+ def def _summary_text(self):
> >
> >A bit picky, but a docstring would be nice:
> >"""Current hostname and configuration text shown to user."""
> >Or similar.
>
> Fixed locally.
>
> >...
> >
> >@@ -268,148 +173,98 @@ class NetworkSpoke(EditTUISpoke):
> > """ Handle the input. """
> >
> >Since the below block of code is being removed from the try/except
> >block, there's no guarantee that 'num' will ever get set. I think at the
> >very least it should get set to 'None' initially, before we go through
> >the rest of the function.
>
> I am not sure I am getting your point, I think that either num will
> be int or we return from the function (conversion exception is caught).
Argh, yeah you're right--disregard my comment.
> def _update_network_data(self):
> - """ Update all of the network data. """
>
> I'd at least leave the docstring. ;)
>
>
> Agreed, fixed locally.
>
> >...
> >
> > class ConfigureNetworkSpoke(EditTUISpoke):
> > """ Spoke to set various configuration options for net devices. """
> >- title = _("Network settings")
> >+ title = _("Device configuration")
> > category = "network"
> > edit_fields = [
> >- Entry(_("IPv4 Address"), "ip", re.compile(".*$"), lambda self, args: self.proto == 1),
> >- Entry(_("Netmask"), "netmask", re.compile(".*$"), True),
> >- Entry(_("Gateway"), "gateway", re.compile(".*$"), lambda self, args: self.proto == 1),
> >- Entry(_("DNS"), "nameserver", re.compile(".*$"), True),
> >- Entry(_("IPv6 Address"), "ipv6", re.compile(".*$"), lambda self, args: self.proto == 2),
> >- Entry(_("Gateway"), "ipv6gateway", re.compile(".*$"), lambda self, args: self.proto == 2)
> >+ Entry(_('IPv4 configuration (address or "dhcp")'), "ip", re.compile(".*$"), True),
> >+ Entry(_("IPv4 netmask"), "netmask", re.compile(".*$"), True),
> >+ Entry(_("IPv4 gateway"), "gateway", re.compile(".*$"), True),
> >+ Entry(_('IPv6 configuration (address or "auto", "dhcp", "ignore")'), "ipv6", re.compile("
> >+ Entry(_("IPv6 default gateway"), "ipv6gateway", re.compile(".*$"), True),
> >+ Entry(_("Nameservers (comma separated)"), "nameserver", re.compile(".*$"), True),
> >+ Entry(_("Connect automatically after reboot"), "onboot", EditTUISpoke.CHECK, True),
> >+ Entry(_("Apply configuration in installer"), "_apply", EditTUISpoke.CHECK, True),
> > ]
> >
> >There should be better error checking for a couple of these options,
> >namely 'IPv4 configuration (address or "dhcp")' and 'IPv6 configuration
> >(address or "auto", "dhcp", "ignore")'.
> >While unlikely that someone will intentionally type something other than
> >the proffered options, it's possible someone will make a typo when
> >entering "dhcp", "auto", or "ignore", and the installer shouldn't crash
> >just because of that.
> >
> >This can be checked in the input() function of the parent spoke, before
> >calling network.update_settings_with_ksdata(), e.g.:
> >
> >@@ -190,14 +190,19 @@
> > self.payload, self.instclass, ndata)
> > self.app.switch_screen_modal(newspoke)
> >- if ndata.ip == "dhcp":
> >+ if type(ndata.ip) == str and ndata.ip.lower() != _("dhcp"):
> >+ self.errors.append(_("%s is an invalid option for device %s.") % (ndata.ip, devname))
> >+ elif ndata.ip == "dhcp":
> > ndata.bootProto = "dhcp"
> > ndata.ip = ""
> > else:
> > ndata.bootProto = "static"
> >- if ndata.ipv6 == "ignore":
> >- ndata.noipv6 = True
> >- ndata.ipv6 = ""
> >+ if type(ndata.ipv6) == str:
> >+ if ndata.ipv6.lower() not in [_("auto"), _("dhcp"), _("ignore")]:
> >+ self.errors.append(_("%s is an invalid option for device %s.") % (ndata.ipv6, devname))
> >+ elif ndata.ipv6 == "ignore":
> >+ ndata.noipv6 = True
> >+ ndata.ipv6 = ""
> > else:
> > ndata.noipv6 = False
>
> Yeah, we could add some check, but I think regexps of Entries should
> be used for that.
Probably a better idea; with my idea the error would only be shown once
the user had already exited the configuration spoke, so they would need
to re-visit it.
> The dhcp/ignore/... values should not be translated so I need to fix the
> string for translation in the Entry accordingly.
>
> Also note that ndata.ip is always str.
>
> Alternatively we could add checkboxes for method of configuration
> (dhcp/ignore/...)
> instead of using string values but this is something I'd left for
> follow-up patches.
> Actually I prefer current way of supplying the method right in place
> of static ip address
> (it also takes up less vertical space:))
I prefer the current way as well--it looks much cleaner.
> >Aside from that, I think this is all ok, although the network category
> >needs adding to the categories list so it's displayed:
> >
> >---
> > pyanaconda/ui/tui/hubs/summary.py | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >diff --git a/pyanaconda/ui/tui/hubs/summary.py
> >b/pyanaconda/ui/tui/hubs/summary.py
> >index 1ae5cca..3f5f53e 100644
> >--- a/pyanaconda/ui/tui/hubs/summary.py
> >+++ b/pyanaconda/ui/tui/hubs/summary.py
> >@@ -29,7 +29,7 @@ import time
> > class SummaryHub(TUIHub):
> > title = _("Installation")
> > ## FIXME: this should be pulling data from somewhere, not just a static list
> >- categories = ["source", "localization", "destination", "password", "software"]
> >+ categories = ["source", "localization", "destination", "password", "software", "network"]
>
> This is already part of your original patch.
Thanks,
Samantha
More information about the anaconda-patches
mailing list