[master/rhel7-branch][PATCH] Add support for network configuration in TUI. (#909299)

Samantha N. Bueno sbueno+anaconda at redhat.com
Tue Aug 20 17:08:20 UTC 2013


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.

...

@@ -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.

        num = None

         try: 
             num = int(key)
-            if args == 2:
-                # configure network
-                if num:
-                    # 'num' is the IP protocol they wish to configure, ipv4 or
-                    # ipv6 pass this option to the configuration spoke so it
-                    # knows which options to display to the user
-                    newspoke = ConfigureNetworkSpoke(self.app, self.data, self.storage,
-                                            self.payload, self.instclass, num)
-                    self.app.switch_screen_modal(newspoke)
-                    self.apply()
-                    return True
+        except ValueError:
+            return key
+
+        if num == 1:
I think this should probably be:
         if num and num == 1:

+            # set hostname
+            self.app.switch_screen_modal(self.hostname_dialog, Entry(_("Hostname"),
+                                "hostname", re.compile(".*$"), True))
+            self.apply()
+            return True
+        elif 2 <= num <= len(self.supported_devices) + 1:
Likewise:
        elif num and (2 <= num <= len(self.supported_devices) + 1):

...

     def _update_network_data(self):
-        """ Update all of the network data. """

I'd at least leave the docstring. ;)

...

 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

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"]
 
     def setup(self, environment="anaconda"):
         TUIHub.setup(self, environment=environment)
-- 
1.8.3.1

Samantha


More information about the anaconda-patches mailing list