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

[Diko Parvanov]

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.

Unmerged commits

f3fab36... by Diko Parvanov on 2020-06-01

Re-worked actions.py

Closes-Bug: #1833722

diff --git a/actions/actions.py b/actions/actions.py
index 8178850..0cc7b99 100755
--- a/actions/actions.py
+++ b/actions/actions.py
@@ -1,36 +1,59 @@
-#!/usr/bin/env python3
+#!/usr/bin/env ../.venv/bin/python3
 """Actions module."""
 
 import os
 import sys
 
-from charmhelpers.core import hookenv
+
+_path = os.path.dirname(os.path.realpath(__file__))
+_hooks = os.path.abspath(os.path.join(_path, '../hooks'))
+_lib = os.path.abspath(os.path.join(_path, '../lib'))
+
+
+def _add_path(path):
+    if path not in sys.path:
+        sys.path.insert(1, path)
+
+_add_path(_hooks)
+_add_path(_lib)
+
+from charmhelpers.core.hookenv import action_fail
 
 from lib_cron import CronHelper
 
 from lib_logrotate import LogrotateHelper
 
-sys.path.insert(0, os.path.join(os.environ['CHARM_DIR'], 'lib'))
-
-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):
+    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))
+