I want to fix source spoke which was somewhat broken and I found that this change will be somewhat bigger than I expected :).
So in short: * Change hashing of repositories to special IDs (they are attached to existing RepoData instances) * Checks can now test even not selected objects * Check repositories URL before leaving spoke * Some minor fixes
*This is some kind of RFE so please free to tell me if there is better solution*
From: Jiri Konecny jkonecny@redhat.com
In SourceSpoke before this patch InpuChecks reads information from GtkWidgets which means only selected repository could be tested. This is problematic in some circumstances for example when we want to run tests in on_back_clicked method. --- pyanaconda/ui/gui/spokes/source.py | 73 ++++++++++++++++++++++++++++---------- 1 file changed, 55 insertions(+), 18 deletions(-)
diff --git a/pyanaconda/ui/gui/spokes/source.py b/pyanaconda/ui/gui/spokes/source.py index d909ec9..a041f42 100644 --- a/pyanaconda/ui/gui/spokes/source.py +++ b/pyanaconda/ui/gui/spokes/source.py @@ -27,6 +27,7 @@
import os, signal, re from collections import namedtuple +from urllib.parse import urlsplit
import gi gi.require_version("GLib", "2.0") @@ -940,36 +941,62 @@ def _sanitize_model(self, model):
# This method is shared by the checks on urlEntry and repoUrlEntry def _checkURL(self, inputcheck, combo): - url_string = self.get_input(inputcheck.input_obj).strip() + # If combo is not set inputcheck holds repo + is_additional_repo = combo is None + if is_additional_repo: + # Input object contains repository name + repo = self._get_repo_by_name(inputcheck.input_obj) + protocol = urlsplit(repo.baseurl)[0] + url_string = repo.baseurl.strip()[len(protocol) + 3:] + else: + url_string = self.get_input(inputcheck.input_obj).strip() + protocol = combo.get_active_id()
# If this is HTTP/HTTPS/FTP, use the URL_PARSE regex - combo_protocol = combo.get_active_id() - if combo_protocol in (PROTOCOL_HTTP, PROTOCOL_HTTPS, PROTOCOL_FTP): + if protocol in (PROTOCOL_HTTP, PROTOCOL_HTTPS, PROTOCOL_FTP): if not url_string: - return _("URL is empty") + if is_additional_repo and repo.name: + return _("Repository %s has empty url") % repo.name + else: + return _("URL is empty")
m = URL_PARSE.match(url_string) if not m: - return _("Invalid URL") + if is_additional_repo and repo.name: + return _("Repository %s has invalid url") % repo.name + else: + return _("Invalid URL")
# Matching protocols in the URL should already have been removed # by _removeUrlPrefix. If there's still one there, it's wrong. url_protocol = m.group('protocol') if url_protocol: - return _("Protocol in URL does not match selected protocol") + if is_additional_repo and repo.name: + return _("Repository %s does not match selected protocol" % repo.name) + else: + return _("Protocol in URL does not match selected protocol") elif combo_protocol == PROTOCOL_NFS: if not url_string: - return _("NFS server is empty") + if is_additional_repo and repo.name: + return _("Repository %s has empty NFS server") % repo.name + else: + return _("NFS server is empty")
# Make sure the part before the colon looks like a hostname, # and that the path is not empty host, _colon, path = url_string.partition(':')
if not re.match('^' + HOSTNAME_PATTERN_WITHOUT_ANCHORS + '$', host): - return _("Invalid host name") + if is_additional_repo and repo.name: + return _("Repository %s has invalid host name") % repo.name + else: + return _("Invalid host name")
if not path: - return _("Remote directory is required") + if is_additional_repo and repo.name: + return _("Repository %s required remote directory") % repo.name + else: + return _("Remote directory is required")
return InputCheck.CHECK_OK
@@ -977,7 +1004,7 @@ def _checkURLEntry(self, inputcheck): return self._checkURL(inputcheck, self._protocolComboBox)
def _checkRepoURL(self, inputcheck): - return self._checkURL(inputcheck, self._repoProtocolComboBox) + return self._checkURL(inputcheck, None)
# Update the check on urlEntry when the sensitity or selected protocol changes def _updateURLEntryCheck(self, *args): @@ -994,7 +1021,8 @@ def _checkDuplicateRepos(self, inputcheck): return InputCheck.CHECK_OK
def _checkRepoName(self, inputcheck): - repo_name = self.get_input(inputcheck.input_obj).strip() + # Input object is name of the repository + repo_name = inputcheck.input_obj
if not repo_name: return _("Empty repository name") @@ -1011,19 +1039,21 @@ def _checkRepoName(self, inputcheck): return InputCheck.CHECK_OK
def _checkRepoProxy(self, inputcheck): + # Input object contains repo name + repo = self._get_repo_by_name(inputcheck.input_obj) # If nfs is selected as the protocol, skip the proxy check - if self._repoProtocolComboBox.get_active_id() == PROTOCOL_NFS: + if repo.baseurl.startswith(PROTOCOL_NFS): return InputCheck.CHECK_OK
- # Empty proxies are OK, as long as the username and password are empty too - proxy_string = self.get_input(inputcheck.input_obj).strip() - username_set = self._repoProxyUsernameEntry.is_sensitive() and self._repoProxyUsernameEntry.get_text().strip() - password_set = self._repoProxyPasswordEntry.is_sensitive() and self._repoProxyPasswordEntry.get_text().strip() + if not repo.proxy: + return InputCheck.CHECK_OK
- if not (proxy_string or username_set or password_set): + # Empty proxies are OK, as long as the username and password are empty too + proxy_obj = ProxyString(repo.proxy) + if not (repo.proxy or proxy_obj.username or proxy_obj.password): return InputCheck.CHECK_OK
- return _validateProxy(proxy_string, username_set, password_set) + return _validateProxy(proxy_obj.noauth_url, proxy_obj.username, proxy_obj.password)
# Signal handlers. def on_source_toggled(self, button, relatedBox): @@ -1243,6 +1273,13 @@ def _unique_repo_name(self, name): highest_index = max(matches) if matches else 0 return name + ("_%d" % (highest_index + 1))
+ def _get_repo_by_name(self, name): + """ Return a repository by given name""" + for repo in self._repoStore: + if repo.name == name: + return repo + return None + def on_repoSelection_changed(self, *args): """ Called when the selection changed.
From: Jiri Konecny jkonecny@redhat.com
Choose to use identifiers for the repositories because I have found nothing which wasn't changing in time. This seems to fix most of the problems with the Source spoke. --- pyanaconda/ui/gui/spokes/source.py | 63 ++++++++++++++++++++++---------------- 1 file changed, 37 insertions(+), 26 deletions(-)
diff --git a/pyanaconda/ui/gui/spokes/source.py b/pyanaconda/ui/gui/spokes/source.py index a041f42..479c41a 100644 --- a/pyanaconda/ui/gui/spokes/source.py +++ b/pyanaconda/ui/gui/spokes/source.py @@ -391,6 +391,7 @@ def __init__(self, *args, **kwargs): self._proxyUrl = "" self._proxyChange = False self._cdrom = None + self._repo_counter = 0
self._repoChecks = {}
@@ -945,7 +946,7 @@ def _checkURL(self, inputcheck, combo): is_additional_repo = combo is None if is_additional_repo: # Input object contains repository name - repo = self._get_repo_by_name(inputcheck.input_obj) + repo = self._get_repo_by_id(inputcheck.input_obj) protocol = urlsplit(repo.baseurl)[0] url_string = repo.baseurl.strip()[len(protocol) + 3:] else: @@ -1022,7 +1023,7 @@ def _checkDuplicateRepos(self, inputcheck):
def _checkRepoName(self, inputcheck): # Input object is name of the repository - repo_name = inputcheck.input_obj + repo_name = self._get_repo_by_id(inputcheck.input_obj).name
if not repo_name: return _("Empty repository name") @@ -1040,7 +1041,7 @@ def _checkRepoName(self, inputcheck):
def _checkRepoProxy(self, inputcheck): # Input object contains repo name - repo = self._get_repo_by_name(inputcheck.input_obj) + repo = self._get_repo_by_id(inputcheck.input_obj) # If nfs is selected as the protocol, skip the proxy check if repo.baseurl.startswith(PROTOCOL_NFS): return InputCheck.CHECK_OK @@ -1216,6 +1217,8 @@ def _reset_repoStore(self): If the list has no element, clear the repo entry fields. """
+ log.debug("Clearing checks in source spoke") + # Remove the repo checks for checks in self._repoChecks.values(): self.remove_check(checks.name_check) @@ -1235,6 +1238,9 @@ def _reset_repoStore(self): enabled=repo.enabled) # Track the original name, user may change .name ks_repo.orig_name = name + # Add addon repository id for identification + ks_repo.addon_repo_id = self._repo_counter + self._repo_counter += 1 self._repoStore.append([self.payload.isRepoEnabled(name), ks_repo.name, ks_repo]) @@ -1273,11 +1279,12 @@ def _unique_repo_name(self, name): highest_index = max(matches) if matches else 0 return name + ("_%d" % (highest_index + 1))
- def _get_repo_by_name(self, name): - """ Return a repository by given name""" + def _get_repo_by_id(self, id): + """ Return a repository by given name + """ for repo in self._repoStore: - if repo.name == name: - return repo + if repo[REPO_OBJ].addon_repo_id == id: + return repo[REPO_OBJ] return None
def on_repoSelection_changed(self, *args): @@ -1401,6 +1408,9 @@ def on_addRepo_clicked(self, button): repo = self.data.RepoData(name=name) repo.ks_repo = True repo.orig_name = "" + # Set addon repo id and increment counter + repo.addon_repo_id = self._repo_counter + self._repo_counter += 1
itr = self._repoStore.append([True, repo.name, repo]) self._repoSelection.select_iter(itr) @@ -1415,10 +1425,10 @@ def on_removeRepo_clicked(self, button):
# Remove the input validation checks for this repo repo = self._repoStore[itr][REPO_OBJ] - self.remove_check(self._repoChecks[repo.name].name_check) - self.remove_check(self._repoChecks[repo.name].url_check) - self.remove_check(self._repoChecks[repo.name].proxy_check) - del self._repoChecks[repo.name] + self.remove_check(self._repoChecks[repo.addon_repo_id].name_check) + self.remove_check(self._repoChecks[repo.addon_repo_id].url_check) + self.remove_check(self._repoChecks[repo.addon_repo_id].proxy_check) + del self._repoChecks[repo.addon_repo_id]
self._repoStore.remove(itr) if len(self._repoStore) == 0: @@ -1439,17 +1449,12 @@ def on_repoNameEntry_changed(self, entry): repo = self._repoStore[itr][REPO_OBJ] name = self._repoNameEntry.get_text().strip()
- old_name = repo.name - if name == old_name: - # nothing changed - return - repo.name = name self._repoStore.set_value(itr, REPO_NAME_COL, name) - - self._repoChecks[name] = self._repoChecks[old_name] - del self._repoChecks[old_name] - self._repoChecks[name].name_check.update_check_status() + # do not update check status if check are not yet set up + # (populationg/refreshing the spoke) + if repo.addon_repo_id in self._repoChecks: + self._repoChecks[repo.addon_repo_id].name_check.update_check_status()
def on_repoUrl_changed(self, editable, data=None): """ proxy url or protocol changed @@ -1466,7 +1471,10 @@ def on_repoUrl_changed(self, editable, data=None): else: repo.baseurl = proto + url
- self._repoChecks[repo.name].url_check.update_check_status() + # do not update check status if check are not yet set up + # (populationg/refreshing the spoke) + if repo.addon_repo_id in self._repoChecks: + self._repoChecks[repo.addon_repo_id].url_check.update_check_status()
# Check for and remove a URL prefix that matches the protocol dropdown self._removeUrlPrefix(editable, self._repoProtocolComboBox, self.on_repoUrl_changed) @@ -1503,7 +1511,7 @@ def on_repoProxy_changed(self, *args): # do not update check status if checks are not yet set up # (populating/refreshing the spoke) if repo.name in self._repoChecks: - self._repoChecks[repo.name].proxy_check.update_check_status() + self._repoChecks[repo.addon_repo_id].proxy_check.update_check_status()
try: proxy = ProxyString(url=url, username=username, password=password) @@ -1533,13 +1541,16 @@ def on_repoStore_row_inserted(self, model, path, itr, user_data=None): # to this method is different from the iter used everywhere else, and is useless # once this method returns. Instead, create a TreeRowReference and work backwards # from that using paths any time we need to reference the store. - self._repoChecks[repo.name] = RepoChecks(InputCheckHandler.add_check(self, self._repoNameEntry, + self._repoChecks[repo.addon_repo_id] = RepoChecks(InputCheckHandler.add_check(self, + repo.addon_repo_id, self._checkRepoName, Gtk.TreeRowReference.new(model, path)), - InputCheckHandler.add_check(self, self._repoUrlEntry, + InputCheckHandler.add_check(self, + repo.addon_repo_id, self._checkRepoURL, Gtk.TreeRowReference.new(model, path)), - InputCheckHandler.add_check(self, self._repoProxyUrlEntry, + InputCheckHandler.add_check(self, + repo.addon_repo_id, self._checkRepoProxy, Gtk.TreeRowReference.new(model, path)))
@@ -1555,4 +1566,4 @@ def on_repoProtocolComboBox_changed(self, combobox, user_data=None): itr = self._repoSelection.get_selected()[1] if itr: repo = self._repoStore[itr][REPO_OBJ] - self._repoChecks[repo.name].proxy_check.update_check_status() + self._repoChecks[repo.addon_repo_id].proxy_check.update_check_status()
In reply to line 1243 of pyanaconda/ui/gui/spokes/source.py:
I think ``self._repo_counter`` should be a generator and we should use the ``next()`` method to get the next value and increase the counter with no way to do forget about updating the counter.
I think we should either use Python's ``id()`` function (if we are really interested in the object ID) or change ``addon_repo_id`` to ``repo_id`` and add it to ``pyanaconda.kickstart.RepoData`` (our "overrides" for kickstart objects).
From: Jiri Konecny jkonecny@redhat.com
Iterate all repositories at on_back_clicked and check for url so user gets warning about bad url before leaving the spoke. And one small line wrap. --- pyanaconda/ui/gui/spokes/source.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/pyanaconda/ui/gui/spokes/source.py b/pyanaconda/ui/gui/spokes/source.py index 479c41a..8817f18 100644 --- a/pyanaconda/ui/gui/spokes/source.py +++ b/pyanaconda/ui/gui/spokes/source.py @@ -623,7 +623,8 @@ def _grabObjects(self): # Call InputCheckHandler directly since this check operates on rows of a TreeModel # instead of GtkEntry inputs. Updating the check is handled by the signal handlers # connected to repoStore. - self._duplicateRepoCheck = InputCheckHandler.add_check(self, self._repoStore, self._checkDuplicateRepos) + self._duplicateRepoCheck = InputCheckHandler.add_check(self, self._repoStore, + self._checkDuplicateRepos)
# Create a dictionary for the checks on fields in individual repos # These checks will be added and removed as repos are added and removed from repoStore @@ -1069,6 +1070,10 @@ def on_back_clicked(self, button): """If any input validation checks failed, keep the user on the screen. Otherwise, do the usual thing."""
+ # Check repositories on bad url + for repo in self._repoStore: + self._repoChecks[repo[REPO_OBJ].addon_repo_id].url_check.update_check_status() + failed_check = next(self.failed_checks, None)
# If the failed check is the duplicate repo check, focus the repo TreeView
From: Jiri Konecny jkonecny@redhat.com
ProxyString object for parsing proxy was misused. When using proxy with username the password is need too and reverse. --- pyanaconda/ui/gui/spokes/source.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/pyanaconda/ui/gui/spokes/source.py b/pyanaconda/ui/gui/spokes/source.py index 8817f18..688bc7c 100644 --- a/pyanaconda/ui/gui/spokes/source.py +++ b/pyanaconda/ui/gui/spokes/source.py @@ -1519,7 +1519,10 @@ def on_repoProxy_changed(self, *args): self._repoChecks[repo.addon_repo_id].proxy_check.update_check_status()
try: - proxy = ProxyString(url=url, username=username, password=password) + if username and password: + proxy = ProxyString(url=url, username=username, password=password) + else: + proxy = ProxyString(url=url) repo.proxy = proxy.url except ProxyStringError as e: log.error("Failed to parse proxy - %s:%s@%s: %s", username, password, url, e)
From: Jiri Konecny jkonecny@redhat.com
--- pyanaconda/ui/gui/spokes/source.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/pyanaconda/ui/gui/spokes/source.py b/pyanaconda/ui/gui/spokes/source.py index 688bc7c..f466369 100644 --- a/pyanaconda/ui/gui/spokes/source.py +++ b/pyanaconda/ui/gui/spokes/source.py @@ -948,8 +948,11 @@ def _checkURL(self, inputcheck, combo): if is_additional_repo: # Input object contains repository name repo = self._get_repo_by_id(inputcheck.input_obj) - protocol = urlsplit(repo.baseurl)[0] - url_string = repo.baseurl.strip()[len(protocol) + 3:] + protocol = urlsplit(repo.baseurl)[0] # extract protocol part (http, https, nfs...) + if repo.mirrorlist: + url_string = repo.mirrorlist.strip()[len(protocol) + 3:] # +3 for "://" part + else: + url_string = repo.baseurl.strip()[len(protocol) + 3:] # +3 for "://" part else: url_string = self.get_input(inputcheck.input_obj).strip() protocol = combo.get_active_id()
Added label: master.
Looks good to me othewise.
I think self._repo_counter should be a generator and we should use the next() method to get the next value and increase the counter with no way to do forget about updating the counter.
I like this idea. I will change it today.
I think we should either use Python's id() function (if we are really interested in the object ID) or change addon_repo_id to repo_id and add it to pyanaconda.kickstart.RepoData (our "overrides" for kickstart objects).
Yeah I was thinking about it and I don't want to use Python's id() because from time to time the repositories are changing (I don't know why but they are recreated) and recreation change the Python's id(). I think changing the name to `repo_id` is good idea. About the RepoData I was thinking about this too but I didn't want to add `repo_id` to Pykickstart when It's used only in SourceSpoke. What do you think should this be in pykickstart instead of this injection?
About the RepoData I was thinking about this too but I didn't want to add repo_id to Pykickstart when It's used only in SourceSpoke. What do you think should this be in pykickstart instead of this injection?
We have our own ``RepoData`` class in ``pyanaconda.kickstart`` that inherits from the Pykickstart's one. That's the place where this extra attribute should be added and we do the same for some others.
Neat! Thank you for this information. I will upgrade this PR.
anaconda-patches@lists.fedorahosted.org
-
dashea