Snap Layer

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

  1. Git
  2. lp:~tiago.pasqualini/layer-snap
  3. snap_list
  4. Merge into master
Proposed by Tiago Pasqualini da Silva
Status: Superseded
Proposed branch: ~tiago.pasqualini/layer-snap:snap_list
Merge into: layer-snap:master
Diff against target: 61 lines (+22/-4)
1 file modified
lib/charms/layer/snap.py (+22/-4)
Reviewer Review Type Date Requested Status
Canonical IS Reviewers Pending
layer-snap-charmers Pending
Review via email: mp+406039@code.staging.launchpad.net

This proposal has been superseded by a proposal from 2021-10-15.

Commit message

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.

To post a comment you must log in.
Revision history for this message
🤖 Canonical IS Merge Bot (canonical-is-mergebot) wrote :

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

Revision history for this message
🤖 Canonical IS Merge Bot (canonical-is-mergebot) wrote :

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

Revision history for this message
Tom Haddon (mthaddon) wrote :

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.

Revision history for this message
Tiago Pasqualini da Silva (tiago.pasqualini) wrote :

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

There was an error fetching revisions from git servers. Please try again in a few minutes. If the problem persists, contact Launchpad support.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches