[virt-tools-list] [virt-manager PATCH v2 1/3] osdict: expand the "unknown" check for any distro

[Fabiano Fidêncio]

Pavel,

On Fri, Nov 16, 2018 at 10:52 AM Pavel Hrdina <phrdina at redhat.com> 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 at redhat.com>
> > ---
> >  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 at redhat.com
> > 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