[Merge] ~marcustomlinson/ubuntu-release-upgrader:ubuntu/master into ubuntu-release-upgrader:ubuntu/master

[Iain Lane]

Review: Needs Fixing

Thanks for this! As mentioned I've not tested it yet (man, hard to get this into place), but ...

A few comments (mostly nitpicks) inline.

aaaaaand

I get some pycodestyle errors when trying to build the package with gbp buildpackage -S (these are from the pre-build.sh script):

.........../home/laney/dev/canonical/upstream/build-area/ubuntu-release-upgrader-20.04.13/tests/../tests/test_quirks.py:434:80: E501 line too long (81 > 79 characters)
/home/laney/dev/canonical/upstream/build-area/ubuntu-release-upgrader-20.04.13/tests/../tests/test_quirks.py:453:80: E501 line too long (97 > 79 characters)
/home/laney/dev/canonical/upstream/build-area/ubuntu-release-upgrader-20.04.13/tests/../tests/test_quirks.py:493:80: E501 line too long (88 > 79 characters)
/home/laney/dev/canonical/upstream/build-area/ubuntu-release-upgrader-20.04.13/tests/../tests/test_quirks.py:495:80: E501 line too long (108 > 79 characters)
/home/laney/dev/canonical/upstream/build-area/ubuntu-release-upgrader-20.04.13/tests/../tests/test_quirks.py:497:80: E501 line too long (92 > 79 characters)
/home/laney/dev/canonical/upstream/build-area/ubuntu-release-upgrader-20.04.13/tests/../DistUpgrade/DistUpgradeQuirks.py:548:80: E501 line too long (88 > 79 characters)
/home/laney/dev/canonical/upstream/build-area/ubuntu-release-upgrader-20.04.13/tests/../DistUpgrade/DistUpgradeQuirks.py:554:80: E501 line too long (88 > 79 characters)
/home/laney/dev/canonical/upstream/build-area/ubuntu-release-upgrader-20.04.13/tests/../DistUpgrade/DistUpgradeQuirks.py:863:80: E501 line too long (92 > 79 characters)
/home/laney/dev/canonical/upstream/build-area/ubuntu-release-upgrader-20.04.13/tests/../DistUpgrade/DistUpgradeQuirks.py:864:80: E501 line too long (84 > 79 characters)
/home/laney/dev/canonical/upstream/build-area/ubuntu-release-upgrader-20.04.13/tests/../DistUpgrade/DistUpgradeQuirks.py:869:80: E501 line too long (96 > 79 characters)
/home/laney/dev/canonical/upstream/build-area/ubuntu-release-upgrader-20.04.13/tests/../DistUpgrade/DistUpgradeQuirks.py:898:80: E501 line too long (82 > 79 characters)

could you fix those please?

Diff comments:

> diff --git a/DistUpgrade/DistUpgradeQuirks.py b/DistUpgrade/DistUpgradeQuirks.py
> index 511fb58..cea673e 100644
> --- a/DistUpgrade/DistUpgradeQuirks.py
> +++ b/DistUpgrade/DistUpgradeQuirks.py
> @@ -844,18 +847,30 @@ class DistUpgradeQuirks(object):
>              migration: version strings for upgrade (from and to) and the list
>              of snaps (with actions).
>          """
> +        import json
>          self._snap_list = {}
> -        # gtk-common-themes isn't a package name but is this risky?
>          from_channel = "stable/ubuntu-%s" % self._from_version
>          to_channel = "stable/ubuntu-%s" % self._to_version
> -        snaps = {'core18': ('stable', 'stable'),
> -                 'gnome-3-28-1804': (from_channel, to_channel),
> -                 'gtk-common-themes': (from_channel, to_channel),
> -                 'gnome-calculator': (from_channel, to_channel),
> -                 'gnome-characters': (from_channel, to_channel),
> -                 'gnome-logs': (from_channel, to_channel)}
> +        seeded_snaps = {}
> +        unseeded_snaps = {}
> +
> +        current_path = os.path.dirname(os.path.abspath(__file__))
> +        d2s_file = open(current_path + '/deb2snap.json', 'r')

For a system install, I think this should probably be in /usr/share/ubuntu-release-upgrader/. Perhaps try reading from the executable's directory first (to support development) and then fall back to that path?

> +        d2s = json.load(d2s_file)

Please can you handle this file failing to be parsed here (by doing nothing probably)?

> +
> +        for snap in d2s["seeded"]:
> +            seed = d2s["seeded"][snap]
> +            from_chan = from_channel if "from_channel" not in seed else seed["from_channel"]

golf: could be "unseed.get("from_channel", from_channel)"? (and below)

> +            to_chan = to_channel if "to_channel" not in seed else seed["to_channel"]
> +            seeded_snaps[snap] = (seed["deb"], from_chan, to_chan)

Maybe "seed.get("deb", None)"? Then no need for the empty "deb" strings in the json.

> +
> +        for snap in d2s["unseeded"]:
> +            unseed = d2s["unseeded"][snap]
> +            from_chan = from_channel if "from_channel" not in unseed else unseed["from_channel"]
> +            unseeded_snaps[snap] = (unseed["deb"], from_chan)

I think if you make this "(unseed.get("deb", snap), from_chan)" then it would make it possible to omit the "deb" entry if it's the same as the snap, making the json file a bit simpler.

> +
>          self._view.updateStatus(_("Checking for installed snaps"))
> -        for snap, (from_channel, to_channel) in snaps.items():
> +        for snap, (deb, from_channel, to_channel) in seeded_snaps.items():
>              snap_object = {}
>              # check to see if the snap is already installed
>              snap_info = subprocess.Popen(["snap", "info", snap],
> @@ -878,9 +893,9 @@ class DistUpgradeQuirks(object):
>                  # they'll be installed by dependency when the first gnome
>                  # package is installed
>                  cache = self.controller.cache
> -                if (snap not in cache or (snap in cache and
> -                                          not cache[snap].is_installed)):
> -                    logging.debug("Deb package %s is not installed. Skipping "
> +                if (deb not in cache or (deb in cache and
> +                                         not cache[deb].is_installed)):

This isn't your code, but couldn't this be "if deb not in cache or not cache[deb].is_installed"? I mean the "deb in cache" is implied by being in the second branch, no? Perhaps I'm being dense!

> +                    logging.debug("Deb package for %s is not installed. Skipping "
>                                    "snap package installation" % snap)
>                      continue
>  
> diff --git a/DistUpgrade/deb2snap.json b/DistUpgrade/deb2snap.json
> new file mode 100644
> index 0000000..c3d66b0
> --- /dev/null
> +++ b/DistUpgrade/deb2snap.json

am I being dense or doesn't this file need to be installed?

> @@ -0,0 +1,29 @@
> +{
> +    "seeded": {
> +        "core18": {
> +            "deb": "",
> +            "from_channel": "stable",
> +            "to_channel": "stable"
> +        },
> +        "gnome-3-28-1804": {
> +            "deb": ""
> +        },
> +        "gtk-common-themes": {
> +            "deb": ""
> +        },
> +        "snap-store": {
> +            "deb": "gnome-software"
> +        }
> +    },
> +    "unseeded": {
> +        "gnome-calculator": {
> +            "deb": "gnome-calculator"
> +        },
> +        "gnome-characters": {
> +            "deb": "gnome-characters"
> +        },
> +        "gnome-logs": {
> +            "deb": "gnome-logs"
> +        }
> +    }
> +}


-- 
https://code.launchpad.net/~marcustomlinson/ubuntu-release-upgrader/+git/ubuntu-release-upgrader/+merge/379726
Your team Ubuntu Core Development Team is subscribed to branch ubuntu-release-upgrader:ubuntu/master.