Pavel,
On Fri, Nov 16, 2018 at 10:52 AM Pavel Hrdina <phrdina@xxxxxxxxxx> wrote:
>
> On Fri, Oct 19, 2018 at 06:54:52PM +0200, Fabiano Fidêncio wrote:
> > Currently osinfo-db has "unknown" entries for fedora, opensuse and
> > asianux. Considering this list may grow even more at some point, let's
> > just make the check more generic and use it for all of them instead of
>
> s/thos/those/
>
> > keeping it for fedora only.
> >
> > Changes have also been done in urldetect and tests_url, as thos also
> > used latest_fedora_version().
> >
> > Signed-off-by: Fabiano Fidêncio <fidencio@xxxxxxxxxx>
> > ---
> > tests/test_urls.py | 4 ++--
> > virtinst/osdict.py | 22 +++++++++++++++-------
> > virtinst/urldetect.py | 2 +-
> > 3 files changed, 18 insertions(+), 10 deletions(-)
> >
> > diff --git a/tests/test_urls.py b/tests/test_urls.py
> > index 098c5996..e7ab7142 100644
> > --- a/tests/test_urls.py
> > +++ b/tests/test_urls.py
> > @@ -130,11 +130,11 @@ def _sanitize_osdict_name(detectdistro):
> > if detectdistro == "testsuite-fedora-rawhide":
> > # Special value we use in the test suite to always return the latest
> > # fedora when checking rawhide URL
> > - return OSDB.latest_fedora_version()
> > + return OSDB.latest_os_version("fedora")
> >
> > if re.match("fedora[0-9]+", detectdistro):
> > if not OSDB.lookup_os(detectdistro):
> > - ret = OSDB.latest_fedora_version()
> > + ret = OSDB.latest_os_version("fedora")
> > print("\nConverting detectdistro=%s to latest value=%s" %
> > (detectdistro, ret))
> > return ret
>
> This 'if' is no longer part of virt-manager code-base.
>
> > diff --git a/virtinst/osdict.py b/virtinst/osdict.py
> > index 28e076da..49085e6d 100644
> > --- a/virtinst/osdict.py
> > +++ b/virtinst/osdict.py
> > @@ -208,11 +208,15 @@ class _OSDB(object):
> > return None
> >
> > osname = ret[0].get_short_id()
> > - if osname == "fedora-unknown":
> > - osname = self.latest_fedora_version()
> > - logging.debug("Detected location=%s as os=fedora-unknown. "
> > - "Converting that to the latest fedora OS version=%s",
> > - location, osname)
> > + if osname.endswith("-unknown"):
> > + # We want to get whatever the name is apart from unknown, instead
> > + # of getting the distro, as some enterprise distros may have more
> > + # than one "unknown" entry and we'd like to match the correct one.
> > + osdistro = osname.split("-unknown")[0]
> > + osname = self.latest_os_version(osdistro)
> > + logging.debug("Detected location=%s as os=%s-unknown. "
> > + "Converting that to the latest %s OS version=%s",
> > + osdistro, osdistro, location, osname)
>
> Doesn't look correct. It should be:
>
> logging.debug("Detected location='%s' as os='%s-unknown'. "
> "Converting that to the latest '%s' OS version='%s'.",
> location, osdistro, osdistro, osname)
>
> > return osname
> >
> > @@ -236,8 +240,12 @@ class _OSDB(object):
> > return None
> > return oses[0]
> >
> > - def latest_fedora_version(self):
> > - return self.latest_regex("fedora[0-9]+")
> > + def latest_os_version(self, osdistro):
> > + regex = osdistro + "[0-9]+"
> > + if osdistro[-1].isdigit():
> > + regex = osdistro + r"\.[0-9]+"
> > +
> > + return self.latest_regex(regex)
>
> How about this:
>
> def latest_os_version(self, osdistro):
> version = r"\.[0-9]+" if osdistro[-1].isdigit() else "[0-9]+"
> return self.latest_regex(osdistro + version)
>
> It's more Pythonic and will create less objects for GC to cleanup.
>
> In C you would write:
>
> version = condition ? exp1 : exp2;
>
> in python it's a bit different:
>
> version = exp1 if condition else exp2
>
> Otherwise looks good, I can fix these minor issues before pushing.
>
> Pavel
>
> >
> >
> > #####################
> > diff --git a/virtinst/urldetect.py b/virtinst/urldetect.py
> > index 5da15d0b..37d5caf1 100644
> > --- a/virtinst/urldetect.py
> > +++ b/virtinst/urldetect.py
> > @@ -412,7 +412,7 @@ class FedoraDistro(RedHatDistro):
> > return cache.treeinfo_family_regex(famregex)
> >
> > def _detect_version(self):
> > - latest_variant = OSDB.latest_fedora_version()
> > + latest_variant = OSDB.latest_os_version("fedora")
> >
> > verstr = self.cache.treeinfo_version
> > if not verstr:
> > --
> > 2.19.1
> >
> > _______________________________________________
> > virt-tools-list mailing list
> > virt-tools-list@xxxxxxxxxx
> > https://www.redhat.com/mailman/listinfo/virt-tools-list
Thanks for the reviews,
I'll do the changes you suggested in the 3 patches, test them locally
to be sure everything will work as expected and submit a v3.
Best Regards,
--
Fabiano Fidêncio
_______________________________________________
virt-tools-list mailing list
virt-tools-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/virt-tools-list
