Merge ~xavpaice/charm-logrotated:bug_1833722 into charm-logrotated:master

[Xav Paice]

Re-worked actions.py

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

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

[Paul Goins (vultaire)]

I would prefer if we leverage charms.layer.basic.activate_venv(), as described in one of the diff comments here, to better ensure consistency of behavior between actions and hooks. (In conjunction with reverting the sshebang line.)

Other than that, the other changes seem okay, although I don't think they really change behavior.

review: Needs Fixing

[Drew Freiberger (afreiberger)]

There's nothing in this change discussing why any of these actions must run in the venv. the only thing setup in the venv is layer basic's charmhelpers wheelhouse. This should be imported with the snippet suggested by Paul.

Also, there's some iterative leftovers in the code like args that aren't passed or used. linting should have caught that these references are never used.

review: Needs Fixing

[Tom Haddon (mthaddon)]

Leaving for bootstack-reviewers

review: Abstain

[Drew Freiberger (afreiberger)]

taking another look, this looks like a pretty standard actions.py update other than the venv reference in the #! line.

review: Abstain

[Paul Goins (vultaire)]

Minor comment in reply to one of Drew's. My other feedback still stands.

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

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

[Peter Sabaini (peter-sabaini)]

lgtm

review: Approve

[Alvaro Uria (aluria)]

The change looks good. However, I think actions.py should call #!/usr/local/sbin/charm-env python3 so the active_venv() part is automatically done and "lib" is also included by default.

Other than that, +1

[Paul Goins (vultaire)]

I agree with aluria's comment, but that can be fixed another time; I don't think it's important enough to block on. I'm +1'ing this.

review: Approve

[Paul Goins (vultaire)]

> The change looks good. However, I think actions.py should call
> #!/usr/local/sbin/charm-env python3 so the active_venv() part is automatically
> done and "lib" is also included by default.
>
> Other than that, +1

Good point btw; I wasn't aware of that methodology.

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

Change successfully merged at revision 9f83e6177ea72883b973c50c97307b1d8e3f7baf

diff --git a/src/actions/actions.py b/src/actions/actions.py
index dd311d4..65ee216 100755
--- a/src/actions/actions.py
+++ b/src/actions/actions.py
@@ -4,33 +4,51 @@
 import os
 import sys
 
-from charmhelpers.core import hookenv
+sys.path.append("lib")
 
-from lib_cron import CronHelper
+from charms.layer.basic import activate_venv  # NOQA E402
 
-from lib_logrotate import LogrotateHelper
+activate_venv()
 
-sys.path.insert(0, os.path.join(os.environ["CHARM_DIR"], "lib"))
+from charmhelpers.core.hookenv import action_fail  # NOQA E402
+from lib_cron import CronHelper  # NOQA E402
+from lib_logrotate import LogrotateHelper  # NOQA E402
 
-hooks = hookenv.Hooks()
 logrotate = LogrotateHelper()
 cron = CronHelper()
 
 
[e-mail]("update-logrotate-files")
-def update_logrotate_files():
+def update_logrotate_files(args):
     """Update the logrotate files."""
     logrotate.read_config()
     logrotate.modify_configs()
 
 
[e-mail]("update-cronjob")
-def update_cronjob():
+def update_cronjob(args):
     """Update the cronjob file."""
     cron.read_config()
     cron.install_cronjob()
 
 
+ACTIONS = {
+    "update-cronjob": update_cronjob,
+    "update-logrotate-files": update_logrotate_files,
+}
+
+
+def main(args):
+    """Run assigned action."""
+    action_name = os.path.basename(args[0])
+    try:
+        action = ACTIONS[action_name]
+    except KeyError:
+        return "Action {} undefined".format(action_name)
+    else:
+        try:
+            action(args)
+        except Exception as e:
+            action_fail(str(e))
+
+
 if __name__ == "__main__":
-    """Main function."""
-    hooks.execute(sys.argv)
+    sys.exit(main(sys.argv))