Merge lp://qastaging/~joeborg/charm-helpers/snap-install into lp://qastaging/charm-helpers

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?

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,
>> + ...

Let's try to land snap_install/snap_remove/snap_refresh for now and if we find that we need any other commands later we can add them then. I'd say follow the lead of the apt functions in terms of parameters and code structure.

Updates pushed

Looking good Joe. Couple of comments inline below.

Hey Joe, The unit tests are failing. I think you just need to mock out os.environ(). I gave the new functions a test run via the keystone charm and they worked as expected, no problems. I didn't test out the doc updates yet.

> Hey Joe, The unit tests are failing. I think you just need to mock out
> os.environ(). I gave the new functions a test run via the keystone charm and
> they worked as expected, no problems. I didn't test out the doc updates yet.

Fixed! Was running tests with `python -m unittest` which didn't flag the issue.

LGTM thanks!

Looks good. Just a couple of requests.

Please create a specific Exception to raise in line 66.

And please explicitly add log severity to to lines 91,112 and 133

log("MESSAGE", level='ERROR')

Added!

Thanks for the work. Let me know if you need me to do the merge for this.

> Thanks for the work. Let me know if you need me to do the merge for this.

Yes please, I don't have the permissions.