[f20-branch/rhel7] Do error checking of repository names on "Installation Source" screen.

mulhern amulhern at redhat.com
Mon Oct 21 21:37:24 UTC 2013


Resolves fedora #985103

Addresses several possible errors the user can make in the choice of additional
repository names. Also, corrects a bug whereby if two repositories swap names
one repository picks up the other repository's data.

The errors the user can make which are addressed are:

1) Badly formatted names, currently defined as those outside the format accepted
by Yum or empty strings.

2) Name conflicts, where the user selects a repo name which anaconda is already
using.

3) Duplicate names in the list of additional repositories.

The changes are:

*) A warning label is added below the Name Entry box and
manipulated appropriately on badly formatted names or name conflicts.

*) A warning label is added in the repositories grid, below the list of
repositories, and manipulated appropriately when duplicate repository names are
entered.

*) If the user does choose duplicate repository names, a warning message is
displayed at the bottom of the screen using set_warning.

*) When the user presses Done, if there are duplicate entries, the repo changes
are not saved and the user remains on the installation source spoke.

*) The error messages returned from status() method are separated out a bit, so
that one sees different messages for self._error and not self.payload.baseRepo
and not self._currentIsoFile

*) Removal of repos from addOns is based on original names of repos in the list
presented to the user, rather than their current names.

*) The addOn list is updated as if each change were executed in parallel, so
that swapping two repo names works correctly.

*) The default name of a new repository is changed from "New Repository" to
"New_Repository" and so is well-formed.

*) The tooltip on the Reset button now says "Revert to the previous list of
repositories" rather than "Revert to the default list of repositories".

*) Made default_repos a field of the YumPayload class rather than file level
for easier access via payload object.

Notes: Some lines are removed by glade from source.glade without my
intervention. They all are property elements with attribute
name="use_action_appearance". I think this has no effect on the screens.

PyGobject does not handle the editing-done signal properly, so the changed
signal is the only signal that can be used to detect any changes in the Name
field. Thus, names are checked, and may be assigned, on every keypress, instead
of just when the user presses enter. This is frustrating but it has been true
for a long time, so it's just the way it works.

Code in yumpayload.py may disable a repo that the user had enabled. The user
will not see any message notifying them of this if it occurs. Ideally, some
useful message should show up when the user gets back to the hub. The enabled
checkboxes are updated properly if the user re-enters the spoke. It might
be useful to have a Verify button in the Additonal Repos grid so that the
user can ask for the verification on the additional repo through the screen.

Signed-off-by: mulhern <amulhern at redhat.com>

Conflicts:
	pyanaconda/ui/gui/spokes/source.glade
---
 pyanaconda/packaging/yumpayload.py    |   7 +-
 pyanaconda/ui/gui/spokes/source.glade | 118 +++++++++++++++++++++++++----
 pyanaconda/ui/gui/spokes/source.py    | 139 ++++++++++++++++++++++++++++------
 3 files changed, 222 insertions(+), 42 deletions(-)

diff --git a/pyanaconda/packaging/yumpayload.py b/pyanaconda/packaging/yumpayload.py
index 0eb5cf1..0dfbe8d 100644
--- a/pyanaconda/packaging/yumpayload.py
+++ b/pyanaconda/packaging/yumpayload.py
@@ -84,7 +84,6 @@ from pyanaconda.localization import langcode_matches_locale
 from pykickstart.constants import GROUP_ALL, GROUP_DEFAULT, KS_MISSING_IGNORE
 
 YUM_PLUGINS = ["blacklist", "whiteout", "fastestmirror", "langpacks"]
-default_repos = [productName.lower(), "rawhide"]
 
 import inspect
 import threading
@@ -121,12 +120,16 @@ class YumPayload(PackagePayload):
         only exist in yum. Lastly, the base repo exists in yum and in
         ksdata.method.
     """
+
+
     def __init__(self, data):
         if rpm is None or yum is None:
             raise PayloadError("unsupported payload type")
 
         PackagePayload.__init__(self, data)
 
+        self.default_repos = [productName.lower(), "rawhide"]
+
         self._root_dir = "/tmp/yum.root"
         self._repos_dir = "/etc/yum.repos.d,/etc/anaconda.repos.d,/tmp/updates/anaconda.repos.d,/tmp/product/anaconda.repos.d"
         self._yum = None
@@ -374,7 +377,7 @@ reposdir=%s
 
     @property
     def baseRepo(self):
-        repo_names = [BASE_REPO_NAME] + default_repos
+        repo_names = [BASE_REPO_NAME] + self.default_repos
         base_repo_name = None
         with _yum_lock:
             for repo_name in repo_names:
diff --git a/pyanaconda/ui/gui/spokes/source.glade b/pyanaconda/ui/gui/spokes/source.glade
index 554d673..88ec245 100644
--- a/pyanaconda/ui/gui/spokes/source.glade
+++ b/pyanaconda/ui/gui/spokes/source.glade
@@ -787,11 +787,9 @@
                         <child>
                           <object class="GtkButton" id="proxyButton">
                             <property name="label" translatable="yes">_Proxy setup...</property>
-                            <property name="use_action_appearance">False</property>
                             <property name="visible">True</property>
                             <property name="can_focus">True</property>
                             <property name="receives_default">True</property>
-                            <property name="use_action_appearance">False</property>
                             <property name="use_underline">True</property>
                             <signal name="clicked" handler="on_proxy_clicked" swapped="no"/>
                           </object>
@@ -805,13 +803,11 @@
                         <child>
                           <object class="GtkCheckButton" id="mirrorlistCheckbox">
                             <property name="label" translatable="yes">This URL refers to a _mirror list.</property>
-                            <property name="use_action_appearance">False</property>
                             <property name="visible">True</property>
                             <property name="can_focus">True</property>
                             <property name="receives_default">False</property>
                             <property name="no_show_all">True</property>
                             <property name="hexpand">True</property>
-                            <property name="use_action_appearance">False</property>
                             <property name="use_underline">True</property>
                             <property name="xalign">0</property>
                             <property name="draw_indicator">True</property>
@@ -939,6 +935,7 @@
                     <property name="visible">True</property>
                     <property name="can_focus">False</property>
                     <property name="halign">start</property>
+                    <property name="xalign">0.50999999046325684</property>
                     <property name="label" translatable="yes">Additional repositories</property>
                     <attributes>
                       <attribute name="font-desc" value="Cantarell 10"/>
@@ -1076,8 +1073,8 @@
                             <property name="visible">True</property>
                             <property name="can_focus">False</property>
                             <property name="has_tooltip">True</property>
-                            <property name="tooltip_markup" translatable="yes">Revert to the default list of repositories.</property>
-                            <property name="tooltip_text" translatable="yes">Revert to the default list of repositories.</property>
+                            <property name="tooltip_markup" translatable="yes">Revert to the previous list of repositories.</property>
+                            <property name="tooltip_text" translatable="yes">Revert to the previous list of repositories.</property>
                             <property name="label" translatable="yes">Reset</property>
                             <property name="use_underline">True</property>
                             <property name="icon_name">view-refresh</property>
@@ -1114,7 +1111,7 @@
                           </object>
                           <packing>
                             <property name="left_attach">1</property>
-                            <property name="top_attach">1</property>
+                            <property name="top_attach">2</property>
                             <property name="width">1</property>
                             <property name="height">1</property>
                           </packing>
@@ -1131,7 +1128,7 @@
                           </object>
                           <packing>
                             <property name="left_attach">1</property>
-                            <property name="top_attach">3</property>
+                            <property name="top_attach">4</property>
                             <property name="width">1</property>
                             <property name="height">1</property>
                           </packing>
@@ -1148,7 +1145,7 @@
                           </object>
                           <packing>
                             <property name="left_attach">1</property>
-                            <property name="top_attach">4</property>
+                            <property name="top_attach">5</property>
                             <property name="width">1</property>
                             <property name="height">1</property>
                           </packing>
@@ -1165,7 +1162,7 @@
                           </object>
                           <packing>
                             <property name="left_attach">1</property>
-                            <property name="top_attach">5</property>
+                            <property name="top_attach">6</property>
                             <property name="width">1</property>
                             <property name="height">1</property>
                           </packing>
@@ -1184,7 +1181,7 @@
                           </object>
                           <packing>
                             <property name="left_attach">1</property>
-                            <property name="top_attach">2</property>
+                            <property name="top_attach">3</property>
                             <property name="width">1</property>
                             <property name="height">1</property>
                           </packing>
@@ -1226,7 +1223,7 @@
                           </object>
                           <packing>
                             <property name="left_attach">0</property>
-                            <property name="top_attach">1</property>
+                            <property name="top_attach">2</property>
                             <property name="width">1</property>
                             <property name="height">1</property>
                           </packing>
@@ -1245,7 +1242,7 @@
                           </object>
                           <packing>
                             <property name="left_attach">0</property>
-                            <property name="top_attach">3</property>
+                            <property name="top_attach">4</property>
                             <property name="width">1</property>
                             <property name="height">1</property>
                           </packing>
@@ -1264,7 +1261,7 @@
                           </object>
                           <packing>
                             <property name="left_attach">0</property>
-                            <property name="top_attach">4</property>
+                            <property name="top_attach">5</property>
                             <property name="width">1</property>
                             <property name="height">1</property>
                           </packing>
@@ -1283,7 +1280,52 @@
                           </object>
                           <packing>
                             <property name="left_attach">0</property>
-                            <property name="top_attach">5</property>
+                            <property name="top_attach">6</property>
+                            <property name="width">1</property>
+                            <property name="height">1</property>
+                          </packing>
+                        </child>
+                        <child>
+                          <object class="GtkBox" id="repoNameWarningBox">
+                            <property name="can_focus">False</property>
+                            <property name="no_show_all">True</property>
+                            <property name="spacing">6</property>
+                            <child>
+                              <object class="GtkImage" id="image1">
+                                <property name="visible">True</property>
+                                <property name="can_focus">False</property>
+                                <property name="ypad">3</property>
+                                <property name="stock">gtk-dialog-warning</property>
+                                <property name="icon-size">2</property>
+                              </object>
+                              <packing>
+                                <property name="expand">False</property>
+                                <property name="fill">True</property>
+                                <property name="position">0</property>
+                              </packing>
+                            </child>
+                            <child>
+                              <object class="GtkLabel" id="repoNameWarningLabel">
+                                <property name="visible">True</property>
+                                <property name="can_focus">False</property>
+                                <property name="xalign">0.43000000715255737</property>
+                                <property name="label" translatable="yes">Invalid repository name. Try something else?</property>
+                                <attributes>
+                                  <attribute name="font-desc" value="Cantarell 10"/>
+                                  <attribute name="scale" value="1"/>
+                                  <attribute name="foreground" value="#cccc00000000"/>
+                                </attributes>
+                              </object>
+                              <packing>
+                                <property name="expand">False</property>
+                                <property name="fill">True</property>
+                                <property name="position">1</property>
+                              </packing>
+                            </child>
+                          </object>
+                          <packing>
+                            <property name="left_attach">1</property>
+                            <property name="top_attach">1</property>
                             <property name="width">1</property>
                             <property name="height">1</property>
                           </packing>
@@ -1309,6 +1351,9 @@
                         <child>
                           <placeholder/>
                         </child>
+                        <child>
+                          <placeholder/>
+                        </child>
                       </object>
                       <packing>
                         <property name="left_attach">1</property>
@@ -1317,6 +1362,49 @@
                         <property name="height">2</property>
                       </packing>
                     </child>
+                    <child>
+                      <object class="GtkBox" id="repoNamesWarningBox">
+                        <property name="can_focus">False</property>
+                        <property name="no_show_all">True</property>
+                        <property name="spacing">6</property>
+                        <child>
+                          <object class="GtkImage" id="image2">
+                            <property name="visible">True</property>
+                            <property name="can_focus">False</property>
+                            <property name="stock">gtk-dialog-warning</property>
+                            <property name="icon-size">2</property>
+                          </object>
+                          <packing>
+                            <property name="expand">False</property>
+                            <property name="fill">True</property>
+                            <property name="position">0</property>
+                          </packing>
+                        </child>
+                        <child>
+                          <object class="GtkLabel" id="repoNamesWarningLabel">
+                            <property name="visible">True</property>
+                            <property name="can_focus">False</property>
+                            <property name="label" translatable="yes">Duplicate names in list of additional repositories.</property>
+                            <attributes>
+                              <attribute name="font-desc" value="Cantarell 10"/>
+                              <attribute name="scale" value="1"/>
+                              <attribute name="foreground" value="#cccc00000000"/>
+                            </attributes>
+                          </object>
+                          <packing>
+                            <property name="expand">False</property>
+                            <property name="fill">True</property>
+                            <property name="position">1</property>
+                          </packing>
+                        </child>
+                      </object>
+                      <packing>
+                        <property name="left_attach">0</property>
+                        <property name="top_attach">2</property>
+                        <property name="width">2</property>
+                        <property name="height">1</property>
+                      </packing>
+                    </child>
                   </object>
                   <packing>
                     <property name="expand">True</property>
diff --git a/pyanaconda/ui/gui/spokes/source.py b/pyanaconda/ui/gui/spokes/source.py
index d9216ab..10ff0d4 100644
--- a/pyanaconda/ui/gui/spokes/source.py
+++ b/pyanaconda/ui/gui/spokes/source.py
@@ -25,7 +25,7 @@ import time
 import logging
 log = logging.getLogger("anaconda")
 
-import os, signal
+import os, signal, string
 
 from gi.repository import GLib
 
@@ -502,7 +502,9 @@ class SourceSpoke(NormalSpoke):
             return _("Checking software dependencies...")
         elif not self.ready:
             return _(BASEREPO_SETUP_MESSAGE)
-        elif self._error or not self.payload.baseRepo:
+        elif not self.payload.baseRepo:
+            return _("Error setting up base repository")
+        elif self._error:
             return _("Error setting up software source")
         elif self.data.method.method == "url":
             return self.data.method.url or self.data.method.mirrorlist
@@ -512,7 +514,7 @@ class SourceSpoke(NormalSpoke):
             return _("CD/DVD drive")
         elif self.data.method.method == "harddrive":
             if not self._currentIsoFile:
-                return _("Error setting up software source")
+                return _("Error setting up ISO file")
             return os.path.basename(self._currentIsoFile)
         elif self.payload.baseRepo:
             return _("Closest mirror")
@@ -575,6 +577,12 @@ class SourceSpoke(NormalSpoke):
         else:
             really_hide(self._updatesBox)
 
+        self._repoNameWarningBox = self.builder.get_object("repoNameWarningBox")
+        self._repoNameWarningLabel = self.builder.get_object("repoNameWarningLabel")
+
+        self._repoNamesWarningBox = self.builder.get_object("repoNamesWarningBox")
+        self._repoNamesWarningLabel = self.builder.get_object("repoNamesWarningLabel")
+
         threadMgr.add(AnacondaThread(name=constants.THREAD_SOURCE_WATCHER, target=self._initialize))
 
     def _initialize(self):
@@ -776,6 +784,16 @@ class SourceSpoke(NormalSpoke):
             self._noUpdatesCheckbox.set_active(False)
             self._updatesBox.set_sensitive(True)
 
+    def on_back_clicked(self, button):
+        """If the user entered duplicate repo names, keep them on the screen.
+           Otherwise, do the usual thing."""
+        ui_repo_names = [r[REPO_OBJ].name for r in self._repoStore]
+
+        if len(ui_repo_names) != len(frozenset(ui_repo_names)):
+            return
+        else:
+            NormalSpoke.on_back_clicked(self, button)
+
     def on_chooser_clicked(self, button):
         dialog = IsoChooser(self.data)
 
@@ -864,8 +882,17 @@ class SourceSpoke(NormalSpoke):
         """
         REPO_ATTRS=("name", "baseurl", "mirrorlist", "proxy", "enabled")
         changed = False
-        for repo in [r[REPO_OBJ] for r in self._repoStore]:
-            orig_repo = self.payload.getAddOnRepo(repo.orig_name)
+
+        ui_orig_names = [r[REPO_OBJ].orig_name for r in self._repoStore]
+
+        # Remove repos from payload that were removed in the UI
+        for repo_name in [r for r in self.payload.addOns if r not in ui_orig_names]:
+            repo = self.payload.getAddOnRepo(repo_name)
+            # TODO: Need an API to do this w/o touching yum (not addRepo)
+            self.payload.data.repo.dataList().remove(repo)
+            changed = True
+
+        for repo, orig_repo in [(r[REPO_OBJ],self.payload.getAddOnRepo(r[REPO_OBJ].orig_name)) for r in self._repoStore]:
             if not orig_repo:
                 # TODO: Need an API to do this w/o touching yum (not addRepo)
                 self.payload.data.repo.dataList().append(repo)
@@ -875,30 +902,19 @@ class SourceSpoke(NormalSpoke):
                     setattr(orig_repo, attr, getattr(repo, attr))
                 changed = True
 
-        # Remove repos from payload that were removed in the UI
-        ui_repo_names = [r[REPO_OBJ].name for r in self._repoStore]
-        for repo_name in self.payload.addOns:
-            if repo_name not in ui_repo_names:
-                repo = self.payload.getAddOnRepo(repo_name)
-                # TODO: Need an API to do this w/o touching yum (not addRepo)
-                self.payload.data.repo.dataList().remove(repo)
-                changed = True
-
         return changed
 
     def _reset_repoStore(self):
-        """ Reset the list of repos to the default list and select first entry
+        """ Reset the list of repos.
+
+            Populate the list with all the addon repos from payload.addOns.
 
-            Populate it with all the addon repos from payload.getAddOns
-            If there are none, clear the repo entry fields
+            If the list has no element, clear the repo entry fields.
         """
         self._repoStore.clear()
         repos = self.payload.addOns
         log.debug("Setting up repos: %s", repos)
         for name in repos:
-            if name in [constants.BASE_REPO_NAME, "updates"]:
-                continue
-
             repo = self.payload.getAddOnRepo(name)
             ks_repo = self.data.RepoData(name=repo.name,
                                          baseurl=repo.baseurl,
@@ -958,6 +974,8 @@ class SourceSpoke(NormalSpoke):
         """
         self._repoNameEntry.set_text(repo.name)
 
+        self._display_repo_name_message(repo, repo.name)
+
         self._repoMirrorlistCheckbox.handler_block_by_func(self.on_repoMirrorlistCheckbox_toggled)
         if repo.mirrorlist:
             url = repo.mirrorlist
@@ -995,6 +1013,72 @@ class SourceSpoke(NormalSpoke):
                 log.error("Failed to parse proxy for repo %s: %s", repo.name, e)
                 return
 
+    def _verify_repo_names(self):
+        """ Returns an appropriate error message if the list of repo names
+            contains duplicates.
+        """
+        repo_names = [r[REPO_OBJ].name for r in self._repoStore]
+        if len(repo_names) != len(frozenset(repo_names)):
+            return N_("Duplicate repository names.")
+        return None
+
+    def _display_repo_names_message(self):
+        """ Displays a warning if the list of repo names is not valid.
+            Returns the warning message displayed, if any.
+        """
+        warning_msg = self._verify_repo_names()
+        if warning_msg:
+            self._repoNamesWarningLabel.set_text(_(warning_msg))
+            really_show(self._repoNamesWarningBox)
+            self.set_warning(_("Duplicate repository names not allowed; choose a unique name for each repository."))
+            self.window.show_all()
+        else:
+            self._repoNamesWarningLabel.set_text("")
+            really_hide(self._repoNamesWarningBox)
+            self.clear_info()
+        return warning_msg
+
+    def _verify_repo_name(self, repo, name):
+        """ Returns an appropriate error message if the given name
+            is not valid for this repo.
+            Performs these checks:
+            *) Checks if the string is empty
+            *) Checks if the format is accepted by yum.
+            *) Checks if the repository name coincides with any of the
+               non-additional repositories.
+            :param repo: kickstart repository object
+            :type repo: RepoData
+            :param name: the designated name for the repo
+            :type name: string
+        """
+        if name == "":
+            return N_("Empty repository name.")
+
+        allowed_chars = string.ascii_letters + string.digits + '-_.:'
+        if [c for c in name if c not in allowed_chars]:
+            return N_("Invalid repository name.")
+
+        if name in [r for r in self.payload.repos if r not in self.payload.addOns] + [constants.BASE_REPO_NAME] + self.payload.default_repos:
+            return N_("Repository name conflicts with internal repository name.")
+        return None
+
+    def _display_repo_name_message(self, repo, name):
+        """ Displays a warning if the repo name is not valid.
+            Returns the warning message displayed, if any.
+            :param repo: kickstart repository object
+            :type repo: RepoData
+            :param name: the designated name for the repo
+            :type name: string
+        """
+        warning_msg = self._verify_repo_name(repo, name)
+        if warning_msg:
+            self._repoNameWarningLabel.set_text(_(warning_msg))
+            really_show(self._repoNameWarningBox)
+        else:
+            self._repoNameWarningLabel.set_text("")
+            really_hide(self._repoNameWarningBox)
+        return warning_msg
+
     def on_noUpdatesCheckbox_toggled(self, *args):
         """ Toggle the enable state of the updates repo
 
@@ -1012,13 +1096,15 @@ class SourceSpoke(NormalSpoke):
     def on_addRepo_clicked(self, button):
         """ Add a new repository
         """
-        repo = self.data.RepoData(name="New Repository")
+        repo = self.data.RepoData(name="New_Repository")
         repo.ks_repo = True
         repo.orig_name = ""
+
         itr = self._repoStore.append([True, repo.name, repo])
         self._repoSelection.select_iter(itr)
         self._repoEntryBox.set_sensitive(True)
-
+        self._display_repo_name_message(repo, repo.name)
+        self._display_repo_names_message()
 
     def on_removeRepo_clicked(self, button):
         """ Remove the selected repository
@@ -1030,6 +1116,7 @@ class SourceSpoke(NormalSpoke):
         if len(self._repoStore) == 0:
             self._clear_repo_info()
             self._repoEntryBox.set_sensitive(False)
+        self._display_repo_names_message()
 
     def on_resetRepos_clicked(self, button):
         """ Revert to the default list of repositories
@@ -1043,10 +1130,12 @@ class SourceSpoke(NormalSpoke):
         if not itr:
             return
         repo = self._repoStore[itr][REPO_OBJ]
-
         name = self._repoNameEntry.get_text().strip()
-        self._repoStore.set_value(itr, REPO_NAME_COL, name)
-        repo.name = name
+
+        if not self._display_repo_name_message(repo, name):
+            self._repoStore.set_value(itr, REPO_NAME_COL, name)
+            repo.name = name
+        self._display_repo_names_message()
 
     def on_repoUrl_changed(self, *args):
         """ proxy url or protocol changed
-- 
1.8.3.1



More information about the anaconda-patches mailing list