Code review comment for lp://qastaging/~joeborg/charm-helpers/snap-install

Yep, agree with all of that.

I mainly put the explicit args in the snap_install method for verbosity, but I’ll move these to the docs instead.

In terms of more commands; I would probably be a good idea to allow them all (and then we can make one abstract method to handle them all, rather than one for each snap command). Although I’m not sure many of them are relevant in terms of what need to be done on charm install. There’s then the question of implicit vs explicit if there’s a factory creating all these methods, or if you just have a static method with the snap command as an argument.

> On 24 Feb 2017, at 10:17, Corey Bryant <email address hidden> wrote:
>
> Looking like a good start! One overarching comment and then some inline comments below. I think this code should live in charm-helpers/charmhelpers/fetch/ similar to how apt install etc lives there. Also can you think of any other snap commands we should support?
>
> Diff comments:
>
>> === added directory 'charmhelpers/contrib/snap'
>> === added file 'charmhelpers/contrib/snap/__init__.py'
>> --- charmhelpers/contrib/snap/__init__.py 1970-01-01 00:00:00 +0000
>> +++ charmhelpers/contrib/snap/__init__.py 2017-02-24 14:37:30 +0000
>> @@ -0,0 +1,109 @@
>> +# Copyright 2014-2017 Canonical Limited.
>> +#
>> +# Licensed under the Apache License, Version 2.0 (the "License");
>> +# you may not use this file except in compliance with the License.
>> +# You may obtain a copy of the License at
>> +#
>> +# http://www.apache.org/licenses/LICENSE-2.0 <http://www.apache.org/licenses/LICENSE-2.0>
>> +#
>> +# Unless required by applicable law or agreed to in writing, software
>> +# distributed under the License is distributed on an "AS IS" BASIS,
>> +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
>> +# See the License for the specific language governing permissions and
>> +# limitations under the License.
>> +"""
>> +Charm helpers snap for classic charms.
>> +
>> +If writing reactive charms, use the snap layer:
>> +https://lists.ubuntu.com/archives/snapcraft/2016-September/001114.html <https://lists.ubuntu.com/archives/snapcraft/2016-September/001114.html>
>
> Good idea putting this here. You could alternatively point to the Snap layer at http://interfaces.juju.solutions/ <http://interfaces.juju.solutions/>.
>
>> +"""
>> +import subprocess
>> +from charmhelpers.core.hookenv import log
>> +
>> +__author__ = 'Joseph Borg <<email address hidden> <mailto:<email address hidden>>>'
>> +
>> +
>> +def _snap_exec(commands):
>> + """
>> + Execute snap commands.
>> + :param commands: List commands
>> + :return: Integer exit code
>> + """
>> + assert type(commands) == list
>> + proc = subprocess.Popen(
>
> Is there any reason to not use subprocess.check_call here?
>
>> + ['snap'] + commands
>> + )
>> + proc.wait()
>> +
>> + if proc.returncode > 0:
>> + log('`snap %s` exited with a non-zero status' % ' '.join(commands),
>> + level='FATAL')
>> +
>> + return proc.returncode
>> +
>> +
>> +def snap_install(package, edge=False, beta=False, candidate=False, stable=False, devmode=False, jailmode=False,
>> + classic=False, dangerous=False, channel=None, revision=None):
>
> I think instead of passing all of the flags we should just pass an options list, similar to what install() does in charmhelpers/fetch/ubuntu.py. I think that'll make maintenance a bit easier, in case options change in the future.
>
>> + """
>> + Install a snap package.
>> +
>> + :param package: String or List String package name
>> + :param edge: Boolean install from the edge channel
>> + :param beta: Boolean install from the beta channel
>> + :param candidate: Boolean install from the candidate channel
>> + :param stable: Boolean install from the stable channel
>> + :param devmode: Boolean put snap in development mode and disable security confinement
>> + :param jailmode: Boolean put snap in enforced confinement mode
>> + :param classic: Boolean put snap in classic mode and disable security confinement
>> + :param dangerous: Boolean install the given snap file even if there are no pre-acknowledged signatures for
>> + it, meaning it was not verified and could be dangerous (--devmode implies this)
>> + :param channel: String use this channel instead of stable
>> + :param revision: String install the given revision of a snap, to which you must have developer access
>> + :return: None
>> + """
>> + if type(package) is not list:
>> + package = [package]
>> +
>> + flags = []
>> +
>> + if edge:
>> + flags.append('--edge')
>> + if beta:
>> + flags.append('--beta')
>> + if candidate:
>> + flags.append('--candidate')
>> + if stable:
>> + flags.append('--stable')
>> + if devmode:
>> + flags.append('--devmode')
>> + if jailmode:
>> + flags.append('--jailmode')
>> + if classic:
>> + flags.append('--classic')
>> + if dangerous:
>> + flags.append('--dangerous')
>> + if channel:
>> + flags.append('--channel=%s' % channel)
>> + if revision:
>> + flags.append('--revision=%s' % revision)
>> +
>> + message = 'Installing snap "%s"' % package
>> + if flags:
>> + message += ' with options "%s"' % ' '.join(flags)
>> +
>> + log(message)
>> + _snap_exec(['install'] + flags + package)
>> +
>> +
>> +def snap_remove(package):
>
> probably worth enabling an options parameter here too.
>
>> + """
>> + Remove a snap package.
>> +
>> + :param package: String or List String package name
>> + :return: None
>> + """
>> + if type(package) is not list:
>> + package = [package]
>> +
>> + log('Removing snap "%s"' % package)
>> + _snap_exec(['remove'] + package)
>>
>> === added directory 'tests/contrib/snap'
>> === added file 'tests/contrib/snap/__init__.py'
>> --- tests/contrib/snap/__init__.py 1970-01-01 00:00:00 +0000
>> +++ tests/contrib/snap/__init__.py 2017-02-24 14:37:30 +0000
>> @@ -0,0 +1,54 @@
>> +# Copyright 2014-2017 Canonical Limited.
>> +#
>> +# Licensed under the Apache License, Version 2.0 (the "License");
>> +# you may not use this file except in compliance with the License.
>> +# You may obtain a copy of the License at
>> +#
>> +# http://www.apache.org/licenses/LICENSE-2.0
>> +#
>> +# Unless required by applicable law or agreed to in writing, software
>> +# distributed under the License is distributed on an "AS IS" BASIS,
>> +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
>> +# See the License for the specific language governing permissions and
>> +# limitations under the License.
>> +import subprocess
>> +from mock import call
>> +from unittest import TestCase
>> +from charmhelpers.contrib.snap import snap_install, snap_remove
>> +
>> +__author__ = 'Joseph Borg <email address hidden>'
>> +
>> +
>> +class SnapTest(TestCase):
>
> You'll want to use mocking for unit tests. Otherwise users will get snaps installed/removed on their machine if they run the tests, and if they don't have snapd installed tests would fail.
>
>> + """
>> + Test and install and removal of a snap.
>> + """
>> + def setUp(self):
>> + """
>> + Make sure the hello-world snap isn't installed.
>> + :return: None
>> + """
>> + snap_remove('hello-world')
>> +
>> + def testSnapInstall(self):
>> + """
>> + Test snap install.
>> + :return: None
>> + """
>> + snap_install('hello-world')
>> + proc = subprocess.Popen(
>> + ['/snap/bin/hello-world'],
>> + stdout=subprocess.PIPE
>> + )
>> + proc.wait()
>> +
>> + self.assertEqual(proc.stdout.read(), b'Hello World!\n')
>> +
>> + proc.stdout.close()
>> +
>> + def tearDown(self):
>> + """
>> + Remove test snap.
>> + :return: None
>> + """
>> + snap_remove('hello-world')
>
>
> --
> https://code.launchpad.net/~joeborg/charm-helpers/snap-install/+merge/318233 <https://code.launchpad.net/~joeborg/charm-helpers/snap-install/+merge/318233>
> You are the owner of lp:~joeborg/charm-helpers/snap-install.