[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