Merge ~tiago.pasqualini/layer-snap:snap_list into layer-snap:master

[Tiago Pasqualini da Silva]

Change 'snap info' to 'snap list'

Currently snap info is being used to retrieve the version and
tracking channel of the installed snaps, which will fail if the
system does not have access to the snapstore. This patch changes
the layer to use 'snap list', which is an offline command.

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

Unable to determine commit message from repository - please click "Set commit message" and enter the commit message manually.

[Tom Haddon (mthaddon)]

One comment inline about docstring.

The way this is set up currently we're going to have a different exception from previous. With the previous code we'd raise an IndexError if the snap existed but wasn't installed, or a subprocess.CalledProcessError if the snap didn't exist. This code will now raise a KeyError in both cases.

I don't think that's terrible, but I think it might be nice to raise a custom exception so that charm writers can have some stability there by inspecting that.

[Tiago Pasqualini da Silva (tiago.pasqualini)]

Hi Tom,

Fixed the docstring and rebased on top of master.

Regarding the exception, the code already checks if the snap is installed and returns nothing in that case, so it is not expected to fail if used correctly.

Please let me know if changed are enough.

Thanks,
Tiago

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

[Tiago Pasqualini da Silva (tiago.pasqualini)]

Added the same treatment from get_installed_channel on get_installed_version. Now the exception will be swallowed by the functions and no exceptions will be raised to the caller.

[Tiago Pasqualini da Silva (tiago.pasqualini)]

Also fixed small indexing issue, which was causing it to ignore the last line from snap list.

I'm currently testing this with the graylog charm and it is working.

Thanks,
Tiago

[Tom Haddon (mthaddon)]

Some docstrings now need updating based on changed behaviour.

[Tom Haddon (mthaddon)]

LGTM, thx

review: Approve

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

Change successfully merged at revision c37f9333bdf85563439244d1aed87dc733a8f085

diff --git a/lib/charms/layer/snap.py b/lib/charms/layer/snap.py
index 06cc4b1..ae9be45 100644
--- a/lib/charms/layer/snap.py
+++ b/lib/charms/layer/snap.py
@@ -273,11 +273,30 @@ def get(snapname, key):
     return subprocess.check_output(["snap", "get", snapname, key]).strip()
 
 
+def _snap_list():
+    """Constructs a dict with all installed snaps.
+    
+    Queries all the snaps installed and returns a dict containing their
+    versions and tracking channels, indexed by the snap name.
+    """
+    cmd = ["snap", "list"]
+    out = subprocess.check_output(cmd).decode("utf-8", errors="replace").split()
+    snaps = {}
+    for i in range(6, len(out) - 5, 6): # Skip first six, which are the titles
+        # Snap list has 6 columns:
+        # name, version, revision, tracking channel, publisher and notes
+        # We only care about name (0), version (1) and tracking channel (3)
+        snaps[out[i]] = {
+            'version': out[i + 1],
+            'channel': out[i + 3],
+        }
+    return snaps
+
+
 def get_installed_version(snapname):
     """Gets the installed version of a snapname.
-    This function will fail if snapname is not an installed snap.
+    This function will return nothing if snapname is not an installed snap.
     """
-    cmd = ["snap", "info", snapname]
     hookenv.log("Get installed key for snap {}".format(snapname))
     if not reactive.is_flag_set(get_installed_flag(snapname)):
         hookenv.log(
@@ -285,14 +304,21 @@ def get_installed_version(snapname):
             hookenv.WARNING,
         )
         return
-    return subprocess.check_output(cmd).decode("utf-8", errors="replace").partition("installed:")[-1].split()[0]
+    try:
+        return _snap_list()[snapname]['version']
+    except Exception as e:
+        # If it fails to get the version information(ex. installed via resource), return nothing.
+        hookenv.log(
+            "Cannot get snap version: {}".format(e),
+            hookenv.WARNING,
+        )
+        return
 
 
 def get_installed_channel(snapname):
     """Gets the tracking (channel) of a snapname.
-    This function will fail if snapname is not an installed snap.
+    This function will return nothing if snapname is not an installed snap.
     """
-    cmd = ["snap", "info", snapname]
     hookenv.log("Get channel for snap {}".format(snapname))
     if not reactive.is_flag_set(get_installed_flag(snapname)):
         hookenv.log(
@@ -301,7 +327,7 @@ def get_installed_channel(snapname):
         )
         return
     try:
-        return subprocess.check_output(cmd).decode("utf-8", errors="replace").partition("tracking:")[-1].split()[0]
+        return _snap_list()[snapname]['channel']
     except Exception as e:
         # If it fails to get the channel information(ex. installed via resource), return nothing.
         hookenv.log(