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

Proposed by Xav Paice
Status: Merged
Approved by: Paul Goins
Approved revision: e7592deed7d181b683b62f15f4b97d927fa4a4ab
Merged at revision: 9f83e6177ea72883b973c50c97307b1d8e3f7baf
Proposed branch: ~xavpaice/charm-logrotated:bug_1833722
Merge into: charm-logrotated:master
Diff against target: 67 lines (+29/-11)
1 file modified
src/actions/actions.py (+29/-11)
Reviewer Review Type Date Requested Status
Paul Goins Approve
Peter Sabaini Approve
Canonical IS Reviewers Pending
Tom Haddon Pending
BootStack Reviewers Pending
Drew Freiberger Pending
Review via email: mp+396711@code.launchpad.net

This proposal supersedes a proposal from 2020-06-01.

Commit message

Re-worked actions.py

To post a comment you must log in.
Canonical IS Mergebot (canonical-is-mergebot) wrote : Posted in a previous version of this proposal

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

Paul Goins (vultaire) wrote : Posted in a previous version of this proposal

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
Paul Goins (vultaire) : Posted in a previous version of this proposal
Drew Freiberger (afreiberger) wrote : Posted in a previous version of this proposal

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) wrote : Posted in a previous version of this proposal

Leaving for bootstack-reviewers

review: Abstain
Drew Freiberger (afreiberger) wrote : Posted in a previous version of this proposal

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) wrote : Posted in a previous version of this proposal

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

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

Peter Sabaini (peter-sabaini) wrote :

lgtm

review: Approve
Alvaro Uria (aluria) wrote :

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) wrote :

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) wrote :

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

Change successfully merged at revision 9f83e6177ea72883b973c50c97307b1d8e3f7baf

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/src/actions/actions.py b/src/actions/actions.py
2index dd311d4..65ee216 100755
3--- a/src/actions/actions.py
4+++ b/src/actions/actions.py
5@@ -4,33 +4,51 @@
6 import os
7 import sys
8
9-from charmhelpers.core import hookenv
10+sys.path.append("lib")
11
12-from lib_cron import CronHelper
13+from charms.layer.basic import activate_venv # NOQA E402
14
15-from lib_logrotate import LogrotateHelper
16+activate_venv()
17
18-sys.path.insert(0, os.path.join(os.environ["CHARM_DIR"], "lib"))
19+from charmhelpers.core.hookenv import action_fail # NOQA E402
20+from lib_cron import CronHelper # NOQA E402
21+from lib_logrotate import LogrotateHelper # NOQA E402
22
23-hooks = hookenv.Hooks()
24 logrotate = LogrotateHelper()
25 cron = CronHelper()
26
27
28-@hooks.hook("update-logrotate-files")
29-def update_logrotate_files():
30+def update_logrotate_files(args):
31 """Update the logrotate files."""
32 logrotate.read_config()
33 logrotate.modify_configs()
34
35
36-@hooks.hook("update-cronjob")
37-def update_cronjob():
38+def update_cronjob(args):
39 """Update the cronjob file."""
40 cron.read_config()
41 cron.install_cronjob()
42
43
44+ACTIONS = {
45+ "update-cronjob": update_cronjob,
46+ "update-logrotate-files": update_logrotate_files,
47+}
48+
49+
50+def main(args):
51+ """Run assigned action."""
52+ action_name = os.path.basename(args[0])
53+ try:
54+ action = ACTIONS[action_name]
55+ except KeyError:
56+ return "Action {} undefined".format(action_name)
57+ else:
58+ try:
59+ action(args)
60+ except Exception as e:
61+ action_fail(str(e))
62+
63+
64 if __name__ == "__main__":
65- """Main function."""
66- hooks.execute(sys.argv)
67+ sys.exit(main(sys.argv))

Subscribers

People subscribed via source and target branches