Merge ~woutervb/charm-sudo-pair:focal into ~sudo-pair-charmers/charm-sudo-pair:master

[Wouter van Bommel]

[Xav Paice (xavpaice)]

Tests work fine for me. This is a big looking change, but mostly it's reformatting (Black?). The actual guts of the change is to add testing and metadata.yaml for focal - the rest is lint.

review: Approve

[Paul Goins (vultaire)]

Suggestion for next time to make it easier to review: try to do 2 separate commits, one for black and one for the actual updates. It sounds like black was called automatically in this case, but at least where we can in the future, that'd help ease reviewing. :)

[Paul Goins (vultaire)]

"2 separate commits" - not literally meant, could be 2 or more.

[Paul Goins (vultaire)]

I don't see an update to metadata.yaml here; looks like it was done immediately prior before.

Looks good; approved.

review: Approve

[Paul Goins (vultaire)]

...This does not apply cleanly to the current master branch of the top-level repo.

review: Needs Fixing

[Paul Goins (vultaire)]

I checked out the previous commit, ran black on the file, then diffed against this code.

The only change here is:

diff --git a/tests/functional/test_deploy.py b/tests/functional/test_deploy.py
index 27a3f9d..73971dd 100644
--- a/tests/functional/test_deploy.py
+++ b/tests/functional/test_deploy.py
@@ -7,7 +7,7 @@ pytestmark = pytest.mark.asyncio

 def get_series():
- series = os.getenv("test_series", "xenial bionic focal").strip()
+ series = os.getenv("test_series", "xenial bionic").strip()
     return series.split()

I will submit a new merge proposal.

[Paul Goins (vultaire)]

New MR: https://code.launchpad.net/~vultaire/charm-sudo-pair/+git/charm-sudo-pair/+merge/384398

Unmerged commits

62b8c98... by Wouter van Bommel on 2020-02-10

Added focal as testing target

By updating series focal is also a test target. It does require an juju
environment capable of running focal (test) images

diff --git a/tests/functional/test_deploy.py b/tests/functional/test_deploy.py
index 5c8e690..27a3f9d 100644
--- a/tests/functional/test_deploy.py
+++ b/tests/functional/test_deploy.py
@@ -7,9 +7,10 @@ pytestmark = pytest.mark.asyncio
 
 
 def get_series():
-    series = os.getenv('test_series', 'xenial bionic').strip()
+    series = os.getenv("test_series", "xenial bionic focal").strip()
     return series.split()
 
+
 ############
 # FIXTURES #
 ############
@@ -17,130 +18,113 @@ def get_series():
 
 # This fixture shouldn't really be in conftest.py since it's specific to this
 # charm
[e-mail](scope='module',
-                params=get_series())
[e-mail](scope="module", params=get_series())
 async def deploy_app(request, model):
-    '''Deploys the sudo_pair app as a subordinate of ubuntu'''
+    """Deploys the sudo_pair app as a subordinate of ubuntu"""
     release = request.param
 
     await model.deploy(
-        'ubuntu',
-        application_name='ubuntu-' + release,
-        series=release,
-        channel='stable'
+        "ubuntu", application_name="ubuntu-" + release, series=release, channel="stable"
     )
     sudo_pair_app = await model.deploy(
-        '{}/builds/sudo-pair'.format(os.getenv('JUJU_REPOSITORY')),
-        application_name='sudo-pair-' + release,
+        "{}/builds/sudo-pair".format(os.getenv("JUJU_REPOSITORY")),
+        application_name="sudo-pair-" + release,
         series=release,
         num_units=0,
         config={
-            'bypass_cmds': '/bin/ls',
-            'groups_enforced': 'ubuntu',
-            'bypass_group': 'warthogs',
-        }
+            "bypass_cmds": "/bin/ls",
+            "groups_enforced": "ubuntu",
+            "bypass_group": "warthogs",
+        },
     )
     await model.add_relation(
-        'ubuntu-{}:juju-info'.format(release),
-        'sudo-pair-{}:juju-info'.format(release))
+        "ubuntu-{}:juju-info".format(release), "sudo-pair-{}:juju-info".format(release)
+    )
 
-    await model.block_until(lambda: sudo_pair_app.status == 'active')
+    await model.block_until(lambda: sudo_pair_app.status == "active")
     yield sudo_pair_app
     # no need to cleanup since the model will be be torn down at the end of the
     # testing
 
 
[e-mail](scope='module')
[e-mail](scope="module")
 async def unit(deploy_app):
-    '''Returns the sudo_pair unit we've deployed'''
+    """Returns the sudo_pair unit we've deployed"""
     return deploy_app.units.pop()
 
+
 #########
 # TESTS #
 #########
 
 
 async def test_deploy(deploy_app):
-    assert deploy_app.status == 'active'
-
-
[e-mail]("path,expected_stat", [
-    ('/usr/lib/sudo/sudo_pair.so', {
-        'gid': 0,
-        'uid': 0,
-        'mode': '0o100644'}),
-    ('/usr/bin/sudo_approve', {
-        'gid': 0,
-        'uid': 0,
-        'mode': '0o100755'}),
-    ('/etc/sudo_pair.prompt.user', {
-        'gid': 0,
-        'uid': 0,
-        'mode': '0o100644'}),
-    ('/etc/sudo_pair.prompt.pair', {
-        'gid': 0,
-        'uid': 0,
-        'mode': '0o100644'}),
-    ('/var/run/sudo_pair', {
-        'gid': 0,
-        'uid': 0,
-        'mode': '0o40644'})])
+    assert deploy_app.status == "active"
+
+
[e-mail](
+    "path,expected_stat",
+    [
+        ("/usr/lib/sudo/sudo_pair.so", {"gid": 0, "uid": 0, "mode": "0o100644"}),
+        ("/usr/bin/sudo_approve", {"gid": 0, "uid": 0, "mode": "0o100755"}),
+        ("/etc/sudo_pair.prompt.user", {"gid": 0, "uid": 0, "mode": "0o100644"}),
+        ("/etc/sudo_pair.prompt.pair", {"gid": 0, "uid": 0, "mode": "0o100644"}),
+        ("/var/run/sudo_pair", {"gid": 0, "uid": 0, "mode": "0o40644"}),
+    ],
+)
 async def test_stats(path, expected_stat, unit, file_stat):
     test_stat = await file_stat(path, unit)
-    assert test_stat['size'] > 0
-    assert test_stat['gid'] == expected_stat['gid']
-    assert test_stat['uid'] == expected_stat['uid']
-    assert test_stat['mode'] == expected_stat['mode']
+    assert test_stat["size"] > 0
+    assert test_stat["gid"] == expected_stat["gid"]
+    assert test_stat["uid"] == expected_stat["uid"]
+    assert test_stat["mode"] == expected_stat["mode"]
 
 
 async def test_sudoers(file_contents, unit):
     sudoers_content = await file_contents("/etc/sudoers", unit)
-    assert 'Defaults	log_output' in sudoers_content
+    assert "Defaults	log_output" in sudoers_content
 
 
 async def test_sudoers_bypass_conf(file_contents, unit):
     path = "/etc/sudoers.d/91-bypass-sudopair-cmds"
-    sudoers_bypass_content = await file_contents(path=path,
-                                                 target=unit)
-    content = '%warthogs ALL = (ALL) NOLOG_OUTPUT: /bin/ls'
+    sudoers_bypass_content = await file_contents(path=path, target=unit)
+    content = "%warthogs ALL = (ALL) NOLOG_OUTPUT: /bin/ls"
     assert content in sudoers_bypass_content
 
 
 async def test_reconfigure(reconfigure_app, file_contents, unit, deploy_app):
-    '''Change a charm config parameter and verify that it has been propagated to
-    the unit'''
-    sudo_approve_path = '/usr/bin/sudo_approve'
-    await reconfigure_app(cfg={'auto_approve': 'false'},
-                          target=deploy_app)
-    sudo_approve_content = await file_contents(path=sudo_approve_path,
-                                               target=unit)
+    """Change a charm config parameter and verify that it has been propagated to
+    the unit"""
+    sudo_approve_path = "/usr/bin/sudo_approve"
+    await reconfigure_app(cfg={"auto_approve": "false"}, target=deploy_app)
+    sudo_approve_content = await file_contents(path=sudo_approve_path, 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)
+    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))
+        "{}: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'
+    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))
+        "{}: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)
+    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'
+    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"