[Merge] lp:~brian-murray/ubuntu-release-upgrader/deb-to-snap into lp:ubuntu-release-upgrader

[Steve Langasek]

Review: Needs Fixing



Diff comments:

> 
> === modified file 'DistUpgrade/DistUpgradeQuirks.py'
> --- DistUpgrade/DistUpgradeQuirks.py	2017-07-13 22:49:22 +0000
> +++ DistUpgrade/DistUpgradeQuirks.py	2018-07-03 21:19:53 +0000
> @@ -471,6 +485,65 @@
>          except Exception as e:
>              logging.warning("error during unlink of old crash files (%s)" % e)
>  
> +    def _checkStoreConnectivity(self):
> +        " check for connectivity to the snap store so snaps can be installed "
> +        msg = ""
> +        summary = ""
> +        connected = Popen(["snap", "debug", "connectivity"], stdout=PIPE,
> +                          stderr=PIPE, universal_newlines=True).communicate()
> +        if re.search("^ \* PASS", connected[0], re.MULTILINE):
> +            return
> +        # can't connect
> +        elif re.search("^ \*.*unreachable", connected[0], re.MULTILINE):
> +            logging.error("No snap store connectivity")
> +            summary = _("Connecting to Snap Store Failed")

I'd suggest either "Connection to Snap Store failed" or "Failed to connect to Snap Store"

> +            msg = _("Your system does not have a connection to the Ubuntu "

I think the branding on this is simply "the Snap Store", not "the Ubuntu Snap Store".

> +                    "Snap store. For the best upgrade experience make sure "
> +                    "that your system can connect to api.snapcraft.io.")

We may or may not care to mention it here, but NB that the Snap Enterprise Proxy means that you might resolve this by configuring a different endpoint instead of api.snapcraft.io.  (https://docs.ubuntu.com/snap-enterprise-proxy/en/install; https://docs.ubuntu.com/snap-enterprise-proxy/en/devices)

> +        # debug command not available
> +        elif 'error: unknown command' in connected[1]:
> +            logging.error("snap debug command not available")
> +            summary = _("Outdated Snapd Package")

I recommend 'Outdated snapd package' without the title-case.

> +            msg = _("Your system does not have the latest version of snapd. "
> +                    "Please update the version of snapd on your system to "
> +                    "improve the upgrade experience.")
> +        # not running as root
> +        elif 'error: access denied' in connected[1]:
> +            logging.error("Not running as root!")
> +        if summary and msg:
> +            self._view.error(summary, msg)
> +        self.controller.abort()

What is the implication of this abort()?  Does it prevent the upgrade from proceeding?  My recollection is that we want to encourage the user to get connected to the snap store before upgrading, but that they should have the option to bypass this.

> +
> +    def _replaceDebsWithSnaps(self):
> +        """ install a snap and mark its corresponding package for removal """
> +        # gtk-common-themes isn't a package name but is this risky?
> +        snaps = ['gtk-common-themes', 'gnome-calculator', 'gnome-characters',
> +                 'gnome-logs', 'gnome-system-monitor']
> +        installed_snaps = subprocess.Popen(["snap", "list"], stdout=PIPE,
> +                                           universal_newlines=True).communicate()
> +        for snap in snaps:
> +            installed = False
> +            # check to see if the snap is already installed
> +            if re.search("^%s " % snap, installed_snaps[0], re.MULTILINE):
> +                logging.debug("Snap %s is already installed" % snap)
> +                installed = True
> +            else:
> +                installed = False

redundant else case since we initialize inside the loop

> +            if not installed:
> +                proc = subprocess.Popen(["snap", "install", "--channel",
> +                                         "stable/ubuntu-18.04", snap],
> +                                        stdout=subprocess.PIPE)
> +                try:
> +                    proc.wait(timeout=180)

Do we expect 3 minutes is long enough for users on slow connections?  Would we be better off waiting indefinitely but installing a ^C handler?

> +                except TimeoutExpired:
> +                    proc.kill()
> +                    logging.debug("Install of snap %s failed" % snap)
> +                if proc.returncode == 0:
> +                    logging.debug("Install of snap %s succeeded" % snap)
> +                    installed = True
> +            if installed:
> +                self.controller.forced_obsoletes.append(snap)
> +
>      def _checkPae(self):
>          " check PAE in /proc/cpuinfo "
>          # upgrade from Precise will fail if PAE is not in cpu flags


-- 
https://code.launchpad.net/~brian-murray/ubuntu-release-upgrader/deb-to-snap/+merge/348916
Your team Ubuntu Core Development Team is subscribed to branch lp:ubuntu-release-upgrader.