Merge ~peter-sabaini/charm-sudo-pair:add-charm-destructor into ~sudo-pair-charmers/charm-sudo-pair:master

[Peter Sabaini]

[Joel Sing (jsing)]

LGTM for readability/standards - some minor comments inline.

Please also get a peer/domain expert review/approval before landing.

review: Approve (+1)

[Giuseppe Petralia (peppepetra)]

Deletion of sudo.conf and lib works fine for me.

In order to run the tests I had to do a couple fix to Makefile and test prep.

I had to add modify Makefile as following:
- remove tabs from line 3 and 4
- remove line 38

I had to add the following line to tox.ini at functional section:
setenv = PYTHONPATH={toxinidir}/lib

I had to add the following lines to tests/functional/requirements.txt:
charmhelpers
charms.reactive
mock

Then everything worked as expected.
Maybe I am missing something. In that case I would recommend to update Readme file properly.

review: Needs Information

[Giuseppe Petralia (peppepetra)]

Looks good to me.

review: Approve

diff --git a/Makefile b/Makefile
index 460970a..935db60 100644
--- a/Makefile
+++ b/Makefile
@@ -1,7 +1,7 @@
 PROJECTPATH = $(dir $(realpath $(firstword $(MAKEFILE_LIST))))
 ifndef JUJU_REPOSITORY
-	JUJU_REPOSITORY := $(shell pwd)
-	$(warning Warning JUJU_REPOSITORY was not set, defaulting to $(JUJU_REPOSITORY))
+  JUJU_REPOSITORY := $(shell pwd)
+  $(warning Warning JUJU_REPOSITORY was not set, defaulting to $(JUJU_REPOSITORY))
 endif
 
 help:
diff --git a/lib/libsudopair.py b/lib/libsudopair.py
index 32cb6d4..3422521 100644
--- a/lib/libsudopair.py
+++ b/lib/libsudopair.py
@@ -58,12 +58,12 @@ class SudoPairHelper(object):
 
     def get_config(self):
         config = {
-            'binary_path' : self.binary_path,
-            'user_prompt_path' : self.user_prompt_path,
-            'pair_prompt_path' :  self.pair_prompt_path,
+            'binary_path': self.binary_path,
+            'user_prompt_path': self.user_prompt_path,
+            'pair_prompt_path': self.pair_prompt_path,
             'socket_dir': self.socket_dir,
             'gids_enforced': group_names_to_group_ids(self.charm_config['groups_enforced']),
-            'gids_exempted' : group_names_to_group_ids(self.charm_config['groups_exempted']),
+            'gids_exempted': group_names_to_group_ids(self.charm_config['groups_exempted']),
         }
 
         config.update(self.charm_config)
@@ -96,11 +96,19 @@ class SudoPairHelper(object):
         copy_file(sudoers_file, self.sudoers_path, self.owner, self.group, self.sudoers_perms)
 
     def render_sudo_approve(self):
+        hookenv.log("Rendering sudo_approve.tmpl to {}".format(self.binary_path))
         return templating.render('sudo_approve.tmpl', self.binary_path, self.get_config(),
                                  perms=self.sudo_approve_perms, owner=self.owner, group=self.group)
 
     def render_bypass_cmds(self):
         if self.get_config()['bypass_cmds'] != "" and self.get_config()['bypass_group'] != "":
+            hookenv.log("Render bypass cmds to {}".format(self.sudoers_bypass_path))
             return templating.render('91-bypass-sudopair-cmds.tmpl', self.sudoers_bypass_path,
                                      self.get_config(), perms=0o440, owner=self.owner, group=self.group)
         return None
+
+    def deconfigure(self):
+        hookenv.log("Delete {}".format(self.sudo_conf_path))
+        os.unlink(self.sudo_conf_path)
+        sudo_pair_lib = os.path.join(hookenv.charm_dir(), 'files', 'sudo_pair.so')
+        hookenv.log("Delete lib {}".format(sudo_pair_lib))
diff --git a/reactive/sudo_pair.py b/reactive/sudo_pair.py
index a5de1e5..13db3d9 100644
--- a/reactive/sudo_pair.py
+++ b/reactive/sudo_pair.py
@@ -39,3 +39,9 @@ def install_sudo_pair():
 def reconfigure_sudo_pair_charm():
     sph.set_charm_config(hookenv.config())
     remove_state('sudo-pair.configured')
+
+
+@hook('stop')
+def stop():
+    sph.deconfigure()
+    remove_state('sudo-pair.installed')
diff --git a/tests/functional/conftest.py b/tests/functional/conftest.py
index d5de8f4..7279748 100644
--- a/tests/functional/conftest.py
+++ b/tests/functional/conftest.py
@@ -1,13 +1,14 @@
 #!/usr/bin/python3
 
-import pytest
-import json
-import uuid
 import asyncio
+import json
 import juju
+import os
+import pytest
+import uuid
 from juju.controller import Controller
-from juju.model import Model
 from juju.errors import JujuError
+from juju.model import Model
 
 STAT_FILE = "python3 -c \"import json; import os; s=os.stat('%s'); print(json.dumps({'uid': s.st_uid, 'gid': s.st_gid, 'mode': oct(s.st_mode), 'size': s.st_size}))\""  # noqa: E501
 
@@ -36,10 +37,12 @@ async def controller():
 @pytest.fixture(scope='module')
 async def model(controller):
     '''This model lives only for the duration of the test'''
-    model_name = str(uuid.uuid4())
+    model_name = "functest-{}".format(uuid.uuid4())
     model = await controller.add_model(model_name)
     yield model
     await model.disconnect()
+    if os.getenv('test_preserve_model'):
+        return
     await controller.destroy_model(model_name)
     while model_name in await controller.list_models():
         await asyncio.sleep(1)
@@ -86,7 +89,7 @@ async def get_entity(model, get_unit, get_app):
         except JujuError:
             try:
                 return await get_app(name)
-            except JujuError as e:
+            except JujuError:
                 raise JujuError("Cannot find entity {}".format(name))
     return _get_entity
 
diff --git a/tests/functional/test_deploy.py b/tests/functional/test_deploy.py
index 9b27ba2..5c8e690 100644
--- a/tests/functional/test_deploy.py
+++ b/tests/functional/test_deploy.py
@@ -1,10 +1,14 @@
 #!/usr/bin/python3.6
-
+import asyncio
 import os
 import pytest
 
 pytestmark = pytest.mark.asyncio
-SERIES = ['xenial', 'bionic']
+
+
+def get_series():
+    series = os.getenv('test_series', 'xenial bionic').strip()
+    return series.split()
 
 ############
 # FIXTURES #
@@ -14,7 +18,7 @@ SERIES = ['xenial', 'bionic']
 # This fixture shouldn't really be in conftest.py since it's specific to this
 # charm
 @pytest.fixture(scope='module',
-                params=SERIES)
+                params=get_series())
 async def deploy_app(request, model):
     '''Deploys the sudo_pair app as a subordinate of ubuntu'''
     release = request.param
@@ -37,9 +41,8 @@ async def deploy_app(request, model):
         }
     )
     await model.add_relation(
-        'ubuntu-' + release,
-        'sudo-pair-' + release
-    )
+        'ubuntu-{}:juju-info'.format(release),
+        'sudo-pair-{}:juju-info'.format(release))
 
     await model.block_until(lambda: sudo_pair_app.status == 'active')
     yield sudo_pair_app
@@ -81,8 +84,7 @@ async def test_deploy(deploy_app):
     ('/var/run/sudo_pair', {
         'gid': 0,
         'uid': 0,
-        'mode': '0o40644'})
-    ])
+        'mode': '0o40644'})])
 async def test_stats(path, expected_stat, unit, file_stat):
     test_stat = await file_stat(path, unit)
     assert test_stat['size'] > 0
@@ -114,3 +116,31 @@ async def test_reconfigure(reconfigure_app, file_contents, unit, deploy_app):
                                                target=unit)
     new_content = 'echo "You can\'t approve your own session."'
     assert new_content in sudo_approve_content
+
+
+async def test_remove_relation(deploy_app, model, run_command):
+    series = deploy_app.units[0].data['series']
+    app_name = 'sudo-pair-{}'.format(series)
+    principalname = 'ubuntu-{}'.format(series)
+    await deploy_app.remove_relation(
+        '{}:juju-info'.format(app_name),
+        '{}:juju-info'.format(principalname))
+    await model.block_until(lambda: not deploy_app.relations)
+    principal = model.applications[principalname].units[0]
+    res = await run_command('test -f /etc/sudo.conf || echo gone', target=principal)
+    assert res['Stdout'].strip() == 'gone'
+    await model.add_relation(
+        '{}:juju-info'.format(principalname),
+        '{}:juju-info'.format(app_name))
+    await model.block_until(lambda: deploy_app.relations)
+
+
+async def test_remove_unit(deploy_app, model, run_command):
+    series = deploy_app.units[0].data['series']
+    app_name = 'sudo-pair-{}'.format(series)
+    await deploy_app.destroy()
+    while app_name in model.applications:
+        await asyncio.sleep(2)
+    principal = model.applications['ubuntu-{}'.format(series)].units[0]
+    res = await run_command('test -f /etc/sudo.conf || echo gone', target=principal)
+    assert res['Stdout'].strip() == 'gone'
diff --git a/tests/unit/test_libsudopair.py b/tests/unit/test_libsudopair.py
index f6482f8..ecd1e41 100644
--- a/tests/unit/test_libsudopair.py
+++ b/tests/unit/test_libsudopair.py
@@ -57,15 +57,15 @@ class TestSudoPairHelper():
         # Default config
         content = sph.render_sudo_conf()
         expected_content = 'Plugin sudo_pair sudo_pair.so binary_path={} ' \
-                       'user_prompt_path={} ' \
-                       'pair_prompt_path={} socket_dir={} gids_enforced={}'.format(tmpdir.join('/usr/bin/sudo_approve'),
-                                                                                   tmpdir.join('/etc/sudo_pair.prompt.user'),
-                                                                                   tmpdir.join('/etc/sudo_pair.prompt.pair'),
-                                                                                   tmpdir.join('/var/run/sudo_pair'),
-                                                                                   '0')
+                           'user_prompt_path={} ' \
+                           'pair_prompt_path={} socket_dir={} gids_enforced={}'.format(
+                               tmpdir.join('/usr/bin/sudo_approve'),
+                               tmpdir.join('/etc/sudo_pair.prompt.user'),
+                               tmpdir.join('/etc/sudo_pair.prompt.pair'),
+                               tmpdir.join('/var/run/sudo_pair'),
+                               '0')
         assert expected_content in content
 
-
         # Gid exempted
         groups_exempted = grp.getgrgid(os.getgid()).gr_name
         charm_config = {
@@ -100,10 +100,10 @@ class TestSudoPairHelper():
         sph.set_charm_config(charm_config)
         expected_content = 'Plugin sudo_pair sudo_pair.so binary_path={} user_prompt_path={} ' \
                            'pair_prompt_path={} socket_dir={} gids_enforced={}'.format(
-            tmpdir.join('/usr/bin/sudo_approve'),
-            tmpdir.join('/etc/sudo_pair.prompt.user'),
-            tmpdir.join('/etc/sudo_pair.prompt.pair'),
-            tmpdir.join('/var/run/sudo_pair'), '0,' + str(os.getgid()))
+                               tmpdir.join('/usr/bin/sudo_approve'),
+                               tmpdir.join('/etc/sudo_pair.prompt.user'),
+                               tmpdir.join('/etc/sudo_pair.prompt.pair'),
+                               tmpdir.join('/var/run/sudo_pair'), '0,{}'.format(os.getgid()))
         content = sph.render_sudo_conf()
         assert expected_content in content
 
diff --git a/tox.ini b/tox.ini
index 123e618..7d3565b 100644
--- a/tox.ini
+++ b/tox.ini
@@ -1,6 +1,6 @@
 [tox]
 skipsdist=True
-envlist = unit, functional
+envlist = unit, functional, func_xenial
 skip_missing_interpreters = True
 
 [testenv]
@@ -21,11 +21,25 @@ passenv =
 commands = pytest -v --ignore {toxinidir}/tests/unit --ignore {toxinidir}/tests/amulet
 deps = -r{toxinidir}/tests/functional/requirements.txt
 
+
+[testenv:func_xenial]
+passenv =
+  HOME
+  JUJU_REPOSITORY
+  PATH
+  test_preserve_model
+setenv = test_series=xenial
+commands = pytest -v --ignore {toxinidir}/tests/unit --ignore {toxinidir}/tests/amulet 
+deps = -r{toxinidir}/tests/functional/requirements.txt
+
 [testenv:lint]
 commands = flake8
 deps = flake8
 
 [flake8]
+max-line-length = 120
+max-complexity=10
+ignore = W503
 exclude =
     .git,
     __pycache__,