Merge ~logrotate-charmers/charm-logrotated:bugs_1833095_1833093 into charm-logrotated:master
- Git
- lp:~logrotate-charmers/charm-logrotated
- bugs_1833095_1833093
- Merge into master
Proposed by
Jeremy Lounder
Status: | Merged |
---|---|
Approved by: | Paul Goins |
Approved revision: | 6d391ef597fad5552bfdee132025c4aa603a21d9 |
Merged at revision: | b2e03efd1888e6644d178af2f5f098ee326e9df5 |
Proposed branch: | ~logrotate-charmers/charm-logrotated:bugs_1833095_1833093 |
Merge into: | charm-logrotated:master |
Diff against target: |
674 lines (+151/-132) 10 files modified
actions/actions.py (+12/-21) dev/null (+0/-13) lib/lib_cron.py (+38/-32) lib/lib_logrotate.py (+20/-25) reactive/logrotate.py (+20/-8) tests/functional/conftest.py (+14/-9) tests/functional/juju_tools.py (+14/-13) tests/functional/test_logrotate.py (+5/-2) tests/unit/conftest.py (+11/-0) tests/unit/test_logrotate.py (+17/-9) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Paul Goins | Approve | ||
Canonical IS Reviewers | Pending | ||
Review via email: mp+378375@code.launchpad.net |
Commit message
LP#1833095 LP#1833093 Fixes linting and unit test issues.
Description of the change
To post a comment you must log in.
Revision history for this message
🤖 Canonical IS Merge Bot (canonical-is-mergebot) wrote : | # |
Revision history for this message
🤖 Canonical IS Merge Bot (canonical-is-mergebot) wrote : | # |
Unable to determine commit message from repository - please click "Set commit message" and enter the commit message manually.
Revision history for this message
Paul Goins (vultaire) wrote : | # |
Looks pretty good. I've suggested a few small improvements, although functionally the code is fine as-is without them. However, removal of the copyright blocks concerns me - I think those need to be restored.
review:
Needs Fixing
Revision history for this message
Paul Goins (vultaire) wrote : | # |
Adding one more comment
Revision history for this message
Paul Goins (vultaire) wrote : | # |
Created bug regarding this code review: https:/
As this code is already merged into the current released charm, am merging this as-is.
review:
Approve
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | diff --git a/actions/__init__.py b/actions/__init__.py | |||
2 | 0 | deleted file mode 100755 | 0 | deleted file mode 100755 |
3 | index 9b088de..0000000 | |||
4 | --- a/actions/__init__.py | |||
5 | +++ /dev/null | |||
6 | @@ -1,13 +0,0 @@ | |||
7 | 1 | # Copyright 2016 Canonical Ltd | ||
8 | 2 | # | ||
9 | 3 | # Licensed under the Apache License, Version 2.0 (the "License"); | ||
10 | 4 | # you may not use this file except in compliance with the License. | ||
11 | 5 | # You may obtain a copy of the License at | ||
12 | 6 | # | ||
13 | 7 | # http://www.apache.org/licenses/LICENSE-2.0 | ||
14 | 8 | # | ||
15 | 9 | # Unless required by applicable law or agreed to in writing, software | ||
16 | 10 | # distributed under the License is distributed on an "AS IS" BASIS, | ||
17 | 11 | # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
18 | 12 | # See the License for the specific language governing permissions and | ||
19 | 13 | # limitations under the License. | ||
20 | diff --git a/actions/actions.py b/actions/actions.py | |||
21 | index aee6bb7..8178850 100755 | |||
22 | --- a/actions/actions.py | |||
23 | +++ b/actions/actions.py | |||
24 | @@ -1,45 +1,36 @@ | |||
25 | 1 | #!/usr/bin/env python3 | 1 | #!/usr/bin/env python3 |
40 | 2 | # | 2 | """Actions module.""" |
27 | 3 | # Copyright 2016 Canonical Ltd | ||
28 | 4 | # | ||
29 | 5 | # Licensed under the Apache License, Version 2.0 (the "License"); | ||
30 | 6 | # you may not use this file except in compliance with the License. | ||
31 | 7 | # You may obtain a copy of the License at | ||
32 | 8 | # | ||
33 | 9 | # http://www.apache.org/licenses/LICENSE-2.0 | ||
34 | 10 | # | ||
35 | 11 | # Unless required by applicable law or agreed to in writing, software | ||
36 | 12 | # distributed under the License is distributed on an "AS IS" BASIS, | ||
37 | 13 | # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
38 | 14 | # See the License for the specific language governing permissions and | ||
39 | 15 | # limitations under the License. | ||
41 | 16 | 3 | ||
42 | 17 | import os | 4 | import os |
43 | 18 | import sys | 5 | import sys |
44 | 19 | 6 | ||
50 | 20 | sys.path.insert(0, os.path.join(os.environ['CHARM_DIR'], 'lib')) | 7 | from charmhelpers.core import hookenv |
46 | 21 | from charmhelpers.core import ( | ||
47 | 22 | hookenv, | ||
48 | 23 | host, | ||
49 | 24 | ) | ||
51 | 25 | 8 | ||
52 | 26 | from lib_logrotate import LogrotateHelper | ||
53 | 27 | from lib_cron import CronHelper | 9 | from lib_cron import CronHelper |
54 | 28 | 10 | ||
55 | 11 | from lib_logrotate import LogrotateHelper | ||
56 | 12 | |||
57 | 13 | sys.path.insert(0, os.path.join(os.environ['CHARM_DIR'], 'lib')) | ||
58 | 14 | |||
59 | 29 | hooks = hookenv.Hooks() | 15 | hooks = hookenv.Hooks() |
60 | 30 | logrotate = LogrotateHelper() | 16 | logrotate = LogrotateHelper() |
61 | 31 | cron = CronHelper() | 17 | cron = CronHelper() |
62 | 32 | 18 | ||
63 | 19 | |||
64 | 33 | @hooks.hook("update-logrotate-files") | 20 | @hooks.hook("update-logrotate-files") |
65 | 34 | def update_logrotate_files(): | 21 | def update_logrotate_files(): |
66 | 22 | """Update the logrotate files.""" | ||
67 | 35 | logrotate.read_config() | 23 | logrotate.read_config() |
68 | 36 | logrotate.modify_configs() | 24 | logrotate.modify_configs() |
69 | 37 | 25 | ||
70 | 26 | |||
71 | 38 | @hooks.hook("update-cronjob") | 27 | @hooks.hook("update-cronjob") |
72 | 39 | def update_cronjob(): | 28 | def update_cronjob(): |
73 | 29 | """Update the cronjob file.""" | ||
74 | 40 | cron.read_config() | 30 | cron.read_config() |
75 | 41 | cron.install_cronjob() | 31 | cron.install_cronjob() |
76 | 42 | 32 | ||
77 | 33 | |||
78 | 43 | if __name__ == "__main__": | 34 | if __name__ == "__main__": |
79 | 35 | """Main function.""" | ||
80 | 44 | hooks.execute(sys.argv) | 36 | hooks.execute(sys.argv) |
81 | 45 | |||
82 | diff --git a/lib/lib_cron.py b/lib/lib_cron.py | |||
83 | index 15bcdaf..f560084 100644 | |||
84 | --- a/lib/lib_cron.py | |||
85 | +++ b/lib/lib_cron.py | |||
86 | @@ -1,24 +1,27 @@ | |||
87 | 1 | """Cron helper module.""" | ||
88 | 1 | import os | 2 | import os |
90 | 2 | import re | 3 | |
91 | 3 | from lib_logrotate import LogrotateHelper | 4 | from lib_logrotate import LogrotateHelper |
92 | 4 | 5 | ||
93 | 6 | |||
94 | 5 | class CronHelper: | 7 | class CronHelper: |
95 | 6 | """Helper class for logrotate charm.""" | 8 | """Helper class for logrotate charm.""" |
96 | 7 | 9 | ||
97 | 8 | @classmethod | ||
98 | 9 | def __init__(self): | 10 | def __init__(self): |
101 | 10 | """Init function""" | 11 | """Init function.""" |
102 | 11 | self.cronjob_check_paths = [ "hourly", "daily", "weekly", "monthly" ] | 12 | self.cronjob_base_path = "/etc/cron." |
103 | 13 | self.cronjob_etc_config = "/etc/logrotate_cronjob_config" | ||
104 | 14 | self.cronjob_check_paths = ["hourly", "daily", "weekly", "monthly"] | ||
105 | 12 | self.cronjob_logrotate_cron_file = "charm-logrotate" | 15 | self.cronjob_logrotate_cron_file = "charm-logrotate" |
106 | 13 | 16 | ||
107 | 14 | |||
108 | 15 | @classmethod | ||
109 | 16 | def read_config(self): | 17 | def read_config(self): |
112 | 17 | """Config changed/install hooks dumps config out to disk, | 18 | """Read the configuration from the file. |
111 | 18 | Here we read that config to update the cronjob""" | ||
113 | 19 | 19 | ||
116 | 20 | config_file = open("/etc/logrotate_cronjob_config", "r") | 20 | Config changed/install hooks dumps config out to disk, |
117 | 21 | lines = config_file.read() | 21 | Here we read that config to update the cronjob. |
118 | 22 | """ | ||
119 | 23 | config_file = open(self.cronjob_etc_config, "r") | ||
120 | 24 | lines = config_file.read() | ||
121 | 22 | lines = lines.split('\n') | 25 | lines = lines.split('\n') |
122 | 23 | 26 | ||
123 | 24 | if lines[0] == 'True': | 27 | if lines[0] == 'True': |
124 | @@ -28,51 +31,54 @@ class CronHelper: | |||
125 | 28 | 31 | ||
126 | 29 | self.cronjob_frequency = int(self.cronjob_check_paths.index(lines[1])) | 32 | self.cronjob_frequency = int(self.cronjob_check_paths.index(lines[1])) |
127 | 30 | 33 | ||
128 | 31 | |||
129 | 32 | @classmethod | ||
130 | 33 | def install_cronjob(self): | 34 | def install_cronjob(self): |
133 | 34 | """If logrotate-cronjob config option is set to True | 35 | """Install the cron job task. |
132 | 35 | install cronjob. Otherwise cleanup""" | ||
134 | 36 | 36 | ||
135 | 37 | If logrotate-cronjob config option is set to True install cronjob, | ||
136 | 38 | otherwise cleanup. | ||
137 | 39 | """ | ||
138 | 37 | clean_up_file = self.cronjob_frequency if self.cronjob_enabled else -1 | 40 | clean_up_file = self.cronjob_frequency if self.cronjob_enabled else -1 |
139 | 38 | 41 | ||
140 | 39 | if self.cronjob_enabled is True: | 42 | if self.cronjob_enabled is True: |
141 | 40 | path_to_lib = os.path.realpath(__file__) | 43 | path_to_lib = os.path.realpath(__file__) |
144 | 41 | cron_file_path = "/etc/cron." + self.cronjob_check_paths[clean_up_file] \ | 44 | cron_file_path = self.cronjob_base_path\ |
145 | 42 | + "/" + self.cronjob_logrotate_cron_file | 45 | + self.cronjob_check_paths[clean_up_file]\ |
146 | 46 | + "/" + self.cronjob_logrotate_cron_file | ||
147 | 43 | 47 | ||
148 | 44 | # upgrade to template if logic increases | 48 | # upgrade to template if logic increases |
149 | 45 | cron_file = open(cron_file_path, 'w') | 49 | cron_file = open(cron_file_path, 'w') |
150 | 46 | cron_file.write("#!/bin/sh\n/usr/bin/python3 " + path_to_lib + "\n\n") | 50 | cron_file.write("#!/bin/sh\n/usr/bin/python3 " + path_to_lib + "\n\n") |
151 | 47 | cron_file.close() | 51 | cron_file.close() |
155 | 48 | os.chmod(cron_file_path,700) | 52 | os.chmod(cron_file_path, 700) |
153 | 49 | |||
154 | 50 | self.cleanup_cronjob(clean_up_file) | ||
156 | 51 | 53 | ||
157 | 54 | self.cleanup_cronjob(clean_up_file) | ||
158 | 52 | 55 | ||
166 | 53 | @classmethod | 56 | def cleanup_cronjob(self, frequency=-1): |
167 | 54 | def cleanup_cronjob(self, frequency = -1): | 57 | """Cleanup previous config.""" |
168 | 55 | """Cleanup previous config""" | 58 | if frequency == -1: |
169 | 56 | for i in range(4): | 59 | for check_path in self.cronjob_check_paths: |
170 | 57 | if frequency != i: | 60 | path = self.cronjob_base_path \ |
171 | 58 | path = "/etc/cron." + self.cronjob_check_paths[i] + "/" +\ | 61 | + check_path \ |
172 | 59 | self.cronjob_logrotate_cron_file | 62 | + "/" \ |
173 | 63 | + self.cronjob_logrotate_cron_file | ||
174 | 60 | if os.path.exists(path): | 64 | if os.path.exists(path): |
175 | 61 | os.remove(path) | 65 | os.remove(path) |
176 | 66 | if os.path.exists(self.cronjob_etc_config): | ||
177 | 67 | os.remove(self.cronjob_etc_config) | ||
178 | 62 | 68 | ||
179 | 63 | @classmethod | ||
180 | 64 | def update_logrotate_etc(self): | 69 | def update_logrotate_etc(self): |
182 | 65 | """Run logrotate update config""" | 70 | """Run logrotate update config.""" |
183 | 66 | logrotate = LogrotateHelper() | 71 | logrotate = LogrotateHelper() |
184 | 67 | logrotate.read_config() | 72 | logrotate.read_config() |
185 | 68 | logrotate.modify_configs() | 73 | logrotate.modify_configs() |
186 | 69 | 74 | ||
187 | 70 | 75 | ||
188 | 71 | def main(): | 76 | def main(): |
193 | 72 | cronHelper = CronHelper() | 77 | """Ran by cron.""" |
194 | 73 | cronHelper.read_config() | 78 | cronhelper = CronHelper() |
195 | 74 | cronHelper.update_logrotate_etc() | 79 | cronhelper.read_config() |
196 | 75 | cronHelper.install_cronjob() | 80 | cronhelper.update_logrotate_etc() |
197 | 81 | cronhelper.install_cronjob() | ||
198 | 76 | 82 | ||
199 | 77 | 83 | ||
200 | 78 | if __name__ == '__main__': | 84 | if __name__ == '__main__': |
201 | diff --git a/lib/lib_logrotate.py b/lib/lib_logrotate.py | |||
202 | index 145a41b..67245cb 100644 | |||
203 | --- a/lib/lib_logrotate.py | |||
204 | +++ b/lib/lib_logrotate.py | |||
205 | @@ -1,6 +1,8 @@ | |||
206 | 1 | """Logrotate module.""" | ||
207 | 1 | import os | 2 | import os |
208 | 2 | import re | 3 | import re |
209 | 3 | 4 | ||
210 | 5 | from charmhelpers.core import hookenv | ||
211 | 4 | 6 | ||
212 | 5 | LOGROTATE_DIR = "/etc/logrotate.d/" | 7 | LOGROTATE_DIR = "/etc/logrotate.d/" |
213 | 6 | 8 | ||
214 | @@ -8,27 +10,24 @@ LOGROTATE_DIR = "/etc/logrotate.d/" | |||
215 | 8 | class LogrotateHelper: | 10 | class LogrotateHelper: |
216 | 9 | """Helper class for logrotate charm.""" | 11 | """Helper class for logrotate charm.""" |
217 | 10 | 12 | ||
218 | 11 | @classmethod | ||
219 | 12 | def __init__(self): | 13 | def __init__(self): |
222 | 13 | """Init function""" | 14 | """Init function.""" |
223 | 14 | pass | 15 | self.retention = hookenv.config('logrotate-retention') |
224 | 15 | 16 | ||
225 | 16 | @classmethod | ||
226 | 17 | def read_config(self): | 17 | def read_config(self): |
229 | 18 | """Config changed/install hooks dumps config out to disk, | 18 | """Read changes from disk. |
228 | 19 | Here we read that config to update the cronjob""" | ||
230 | 20 | 19 | ||
231 | 20 | Config changed/install hooks dumps config out to disk, | ||
232 | 21 | Here we read that config to update the cronjob | ||
233 | 22 | """ | ||
234 | 21 | config_file = open("/etc/logrotate_cronjob_config", "r") | 23 | config_file = open("/etc/logrotate_cronjob_config", "r") |
235 | 22 | lines = config_file.read() | 24 | lines = config_file.read() |
236 | 23 | lines = lines.split('\n') | 25 | lines = lines.split('\n') |
237 | 24 | 26 | ||
238 | 25 | self.retention = int(lines[2]) | 27 | self.retention = int(lines[2]) |
239 | 26 | 28 | ||
240 | 27 | |||
241 | 28 | @classmethod | ||
242 | 29 | def modify_configs(self): | 29 | def modify_configs(self): |
243 | 30 | """Modify the logrotate config files.""" | 30 | """Modify the logrotate config files.""" |
244 | 31 | |||
245 | 32 | for config_file in os.listdir(LOGROTATE_DIR): | 31 | for config_file in os.listdir(LOGROTATE_DIR): |
246 | 33 | file_path = LOGROTATE_DIR + config_file | 32 | file_path = LOGROTATE_DIR + config_file |
247 | 34 | 33 | ||
248 | @@ -44,11 +43,8 @@ class LogrotateHelper: | |||
249 | 44 | logrotate_file.write(mod_contents) | 43 | logrotate_file.write(mod_contents) |
250 | 45 | logrotate_file.close() | 44 | logrotate_file.close() |
251 | 46 | 45 | ||
252 | 47 | |||
253 | 48 | @classmethod | ||
254 | 49 | def modify_content(self, content): | 46 | def modify_content(self, content): |
257 | 50 | """Helper function to edit the content of a logrotate file.""" | 47 | """Edit the content of a logrotate file.""" |
256 | 51 | |||
258 | 52 | # Split the contents in a logrotate file in separate entries (if | 48 | # Split the contents in a logrotate file in separate entries (if |
259 | 53 | # multiple are found in the file) and put in a list for further | 49 | # multiple are found in the file) and put in a list for further |
260 | 54 | # processing | 50 | # processing |
261 | @@ -66,7 +62,7 @@ class LogrotateHelper: | |||
262 | 66 | # the rotate option to the appropriate value | 62 | # the rotate option to the appropriate value |
263 | 67 | results = [] | 63 | results = [] |
264 | 68 | for item in items: | 64 | for item in items: |
266 | 69 | count = self.calculate_count(item) | 65 | count = self.calculate_count(item, self.retention) |
267 | 70 | rotate = 'rotate {}'.format(count) | 66 | rotate = 'rotate {}'.format(count) |
268 | 71 | result = re.sub(r'rotate \d+\.?[0-9]*', rotate, item) | 67 | result = re.sub(r'rotate \d+\.?[0-9]*', rotate, item) |
269 | 72 | results.append(result) | 68 | results.append(result) |
270 | @@ -75,12 +71,9 @@ class LogrotateHelper: | |||
271 | 75 | 71 | ||
272 | 76 | return results | 72 | return results |
273 | 77 | 73 | ||
274 | 78 | @classmethod | ||
275 | 79 | def modify_header(self, content): | 74 | def modify_header(self, content): |
280 | 80 | """Helper function to add Juju headers to the file.""" | 75 | """Add Juju headers to the file.""" |
281 | 81 | 76 | header = "# Configuration file maintained by Juju. Local changes may be overwritten" | |
278 | 82 | header = ("# Configuration file maintained by Juju. " | ||
279 | 83 | "Local changes may be overwritten") | ||
282 | 84 | 77 | ||
283 | 85 | split = content.split('\n') | 78 | split = content.split('\n') |
284 | 86 | if split[0].startswith(header): | 79 | if split[0].startswith(header): |
285 | @@ -91,20 +84,22 @@ class LogrotateHelper: | |||
286 | 91 | return result | 84 | return result |
287 | 92 | 85 | ||
288 | 93 | @classmethod | 86 | @classmethod |
290 | 94 | def calculate_count(self, item): | 87 | def calculate_count(cls, item, retention): |
291 | 95 | """Calculate rotate based on rotation interval. Always round up.""" | 88 | """Calculate rotate based on rotation interval. Always round up.""" |
293 | 96 | 89 | # Fallback to default lowest retention - days | |
294 | 90 | # better to keep the logs than lose them | ||
295 | 91 | count = retention | ||
296 | 97 | # Daily 1:1 to configuration retention period (in days) | 92 | # Daily 1:1 to configuration retention period (in days) |
297 | 98 | if 'daily' in item: | 93 | if 'daily' in item: |
299 | 99 | count = self.retention | 94 | count = retention |
300 | 100 | # Weekly rounding up, as weeks are 7 days | 95 | # Weekly rounding up, as weeks are 7 days |
301 | 101 | if 'weekly' in item: | 96 | if 'weekly' in item: |
303 | 102 | count = int(round(self.retention/7)) | 97 | count = int(round(retention/7)) |
304 | 103 | # Monthly default 30 days and round up because of 28/31 days months | 98 | # Monthly default 30 days and round up because of 28/31 days months |
305 | 104 | if 'monthly' in item: | 99 | if 'monthly' in item: |
307 | 105 | count = int(round(self.retention/30)) | 100 | count = int(round(retention/30)) |
308 | 106 | # For every 360 days - add 1 year | 101 | # For every 360 days - add 1 year |
309 | 107 | if 'yearly' in item: | 102 | if 'yearly' in item: |
311 | 108 | count = self.retention // 360 + 1 if self.retention > 360 else 1 | 103 | count = retention // 360 + 1 if retention > 360 else 1 |
312 | 109 | 104 | ||
313 | 110 | return count | 105 | return count |
314 | diff --git a/reactive/logrotate.py b/reactive/logrotate.py | |||
315 | index 4a8d5c1..1810707 100644 | |||
316 | --- a/reactive/logrotate.py | |||
317 | +++ b/reactive/logrotate.py | |||
318 | @@ -1,36 +1,48 @@ | |||
321 | 1 | from lib_logrotate import LogrotateHelper | 1 | """Reactive charm hooks.""" |
320 | 2 | from lib_cron import CronHelper | ||
322 | 3 | from charmhelpers.core import hookenv | 2 | from charmhelpers.core import hookenv |
323 | 3 | |||
324 | 4 | from charms.reactive import set_flag, when, when_not | 4 | from charms.reactive import set_flag, when, when_not |
325 | 5 | 5 | ||
326 | 6 | from lib_cron import CronHelper | ||
327 | 7 | |||
328 | 8 | from lib_logrotate import LogrotateHelper | ||
329 | 9 | |||
330 | 10 | |||
331 | 11 | hooks = hookenv.Hooks() | ||
332 | 6 | logrotate = LogrotateHelper() | 12 | logrotate = LogrotateHelper() |
333 | 7 | cron = CronHelper() | 13 | cron = CronHelper() |
334 | 8 | 14 | ||
335 | 15 | |||
336 | 9 | @when_not('logrotate.installed') | 16 | @when_not('logrotate.installed') |
337 | 10 | def install_logrotate(): | 17 | def install_logrotate(): |
339 | 11 | dump_config_to_disk(); | 18 | """Install the logrotate charm.""" |
340 | 19 | dump_config_to_disk() | ||
341 | 12 | logrotate.read_config() | 20 | logrotate.read_config() |
342 | 13 | cron.read_config() | 21 | cron.read_config() |
343 | 14 | logrotate.modify_configs() | 22 | logrotate.modify_configs() |
344 | 15 | hookenv.status_set('active', 'Unit is ready.') | 23 | hookenv.status_set('active', 'Unit is ready.') |
345 | 16 | set_flag('logrotate.installed') | 24 | set_flag('logrotate.installed') |
347 | 17 | cron.install_cronjob(); | 25 | cron.install_cronjob() |
348 | 26 | |||
349 | 18 | 27 | ||
350 | 19 | @when('config.changed') | 28 | @when('config.changed') |
351 | 20 | def config_changed(): | 29 | def config_changed(): |
352 | 30 | """Run when configuration changes.""" | ||
353 | 21 | dump_config_to_disk() | 31 | dump_config_to_disk() |
354 | 22 | cron.read_config() | 32 | cron.read_config() |
355 | 23 | logrotate.read_config() | 33 | logrotate.read_config() |
356 | 24 | hookenv.status_set('maintenance', 'Modifying configs.') | 34 | hookenv.status_set('maintenance', 'Modifying configs.') |
357 | 25 | logrotate.modify_configs() | 35 | logrotate.modify_configs() |
358 | 26 | hookenv.status_set('active', 'Unit is ready.') | 36 | hookenv.status_set('active', 'Unit is ready.') |
360 | 27 | cron.install_cronjob(); | 37 | cron.install_cronjob() |
361 | 38 | |||
362 | 28 | 39 | ||
363 | 29 | def dump_config_to_disk(): | 40 | def dump_config_to_disk(): |
364 | 41 | """Dump configurations to disk.""" | ||
365 | 30 | cronjob_enabled = hookenv.config('logrotate-cronjob') | 42 | cronjob_enabled = hookenv.config('logrotate-cronjob') |
366 | 31 | cronjob_frequency = hookenv.config('logrotate-cronjob-frequency') | 43 | cronjob_frequency = hookenv.config('logrotate-cronjob-frequency') |
367 | 32 | logrotate_retention = hookenv.config('logrotate-retention') | 44 | logrotate_retention = hookenv.config('logrotate-retention') |
368 | 33 | with open('/etc/logrotate_cronjob_config', 'w+') as cronjob_config_file: | 45 | with open('/etc/logrotate_cronjob_config', 'w+') as cronjob_config_file: |
372 | 34 | cronjob_config_file.write(str(cronjob_enabled) + '\n') | 46 | cronjob_config_file.write(str(cronjob_enabled) + '\n') |
373 | 35 | cronjob_config_file.write(str(cronjob_frequency) + '\n') | 47 | cronjob_config_file.write(str(cronjob_frequency) + '\n') |
374 | 36 | cronjob_config_file.write(str(logrotate_retention) + '\n') | 48 | cronjob_config_file.write(str(logrotate_retention) + '\n') |
375 | diff --git a/tests/functional/conftest.py b/tests/functional/conftest.py | |||
376 | index 56925ff..1b22cb8 100644 | |||
377 | --- a/tests/functional/conftest.py | |||
378 | +++ b/tests/functional/conftest.py | |||
379 | @@ -1,28 +1,32 @@ | |||
380 | 1 | #!/usr/bin/python3 | 1 | #!/usr/bin/python3 |
383 | 2 | ''' | 2 | """ |
384 | 3 | Reusable pytest fixtures for functional testing | 3 | Reusable pytest fixtures for functional testing. |
385 | 4 | 4 | ||
386 | 5 | Environment variables | 5 | Environment variables |
387 | 6 | --------------------- | 6 | --------------------- |
388 | 7 | 7 | ||
389 | 8 | test_preserve_model: | 8 | test_preserve_model: |
390 | 9 | if set, the testing model won't be torn down at the end of the testing session | 9 | if set, the testing model won't be torn down at the end of the testing session |
392 | 10 | ''' | 10 | """ |
393 | 11 | 11 | ||
394 | 12 | import asyncio | 12 | import asyncio |
395 | 13 | import os | 13 | import os |
396 | 14 | import uuid | ||
397 | 15 | import pytest | ||
398 | 16 | import subprocess | 14 | import subprocess |
399 | 15 | import uuid | ||
400 | 17 | 16 | ||
401 | 18 | from juju.controller import Controller | 17 | from juju.controller import Controller |
402 | 18 | |||
403 | 19 | from juju_tools import JujuTools | 19 | from juju_tools import JujuTools |
404 | 20 | 20 | ||
405 | 21 | import pytest | ||
406 | 22 | |||
407 | 21 | 23 | ||
408 | 22 | @pytest.fixture(scope='module') | 24 | @pytest.fixture(scope='module') |
409 | 23 | def event_loop(): | 25 | def event_loop(): |
412 | 24 | '''Override the default pytest event loop to allow for fixtures using a | 26 | """Override the default pytest event loop. |
413 | 25 | broader scope''' | 27 | |
414 | 28 | Do this too allow for fixtures using a broader scope. | ||
415 | 29 | """ | ||
416 | 26 | loop = asyncio.get_event_loop_policy().new_event_loop() | 30 | loop = asyncio.get_event_loop_policy().new_event_loop() |
417 | 27 | asyncio.set_event_loop(loop) | 31 | asyncio.set_event_loop(loop) |
418 | 28 | loop.set_debug(True) | 32 | loop.set_debug(True) |
419 | @@ -33,7 +37,7 @@ def event_loop(): | |||
420 | 33 | 37 | ||
421 | 34 | @pytest.fixture(scope='module') | 38 | @pytest.fixture(scope='module') |
422 | 35 | async def controller(): | 39 | async def controller(): |
424 | 36 | '''Connect to the current controller''' | 40 | """Connect to the current controller.""" |
425 | 37 | _controller = Controller() | 41 | _controller = Controller() |
426 | 38 | await _controller.connect_current() | 42 | await _controller.connect_current() |
427 | 39 | yield _controller | 43 | yield _controller |
428 | @@ -42,7 +46,7 @@ async def controller(): | |||
429 | 42 | 46 | ||
430 | 43 | @pytest.fixture(scope='module') | 47 | @pytest.fixture(scope='module') |
431 | 44 | async def model(controller): | 48 | async def model(controller): |
433 | 45 | '''This model lives only for the duration of the test''' | 49 | """Live only for the duration of the test.""" |
434 | 46 | model_name = "functest-{}".format(str(uuid.uuid4())[-12:]) | 50 | model_name = "functest-{}".format(str(uuid.uuid4())[-12:]) |
435 | 47 | _model = await controller.add_model(model_name, | 51 | _model = await controller.add_model(model_name, |
436 | 48 | cloud_name=os.getenv('PYTEST_CLOUD_NAME'), | 52 | cloud_name=os.getenv('PYTEST_CLOUD_NAME'), |
437 | @@ -62,5 +66,6 @@ async def model(controller): | |||
438 | 62 | 66 | ||
439 | 63 | @pytest.fixture(scope='module') | 67 | @pytest.fixture(scope='module') |
440 | 64 | async def jujutools(controller, model): | 68 | async def jujutools(controller, model): |
441 | 69 | """Juju tools.""" | ||
442 | 65 | tools = JujuTools(controller, model) | 70 | tools = JujuTools(controller, model) |
443 | 66 | return tools | 71 | return tools |
444 | diff --git a/tests/functional/juju_tools.py b/tests/functional/juju_tools.py | |||
445 | index 2fd501d..5e4a1cc 100644 | |||
446 | --- a/tests/functional/juju_tools.py | |||
447 | +++ b/tests/functional/juju_tools.py | |||
448 | @@ -1,22 +1,26 @@ | |||
449 | 1 | """Juju tools.""" | ||
450 | 2 | import base64 | ||
451 | 1 | import pickle | 3 | import pickle |
452 | 4 | |||
453 | 2 | import juju | 5 | import juju |
454 | 3 | import base64 | ||
455 | 4 | 6 | ||
456 | 5 | # from juju.errors import JujuError | 7 | # from juju.errors import JujuError |
457 | 6 | 8 | ||
458 | 7 | 9 | ||
459 | 8 | class JujuTools: | 10 | class JujuTools: |
460 | 11 | """Juju tools.""" | ||
461 | 12 | |||
462 | 9 | def __init__(self, controller, model): | 13 | def __init__(self, controller, model): |
463 | 14 | """Init.""" | ||
464 | 10 | self.controller = controller | 15 | self.controller = controller |
465 | 11 | self.model = model | 16 | self.model = model |
466 | 12 | 17 | ||
467 | 13 | async def run_command(self, cmd, target): | 18 | async def run_command(self, cmd, target): |
470 | 14 | ''' | 19 | """Run a command on a unit. |
469 | 15 | Runs a command on a unit. | ||
471 | 16 | 20 | ||
472 | 17 | :param cmd: Command to be run | 21 | :param cmd: Command to be run |
473 | 18 | :param unit: Unit object or unit name string | 22 | :param unit: Unit object or unit name string |
475 | 19 | ''' | 23 | """ |
476 | 20 | unit = ( | 24 | unit = ( |
477 | 21 | target | 25 | target |
478 | 22 | if isinstance(target, juju.unit.Unit) | 26 | if isinstance(target, juju.unit.Unit) |
479 | @@ -26,13 +30,12 @@ class JujuTools: | |||
480 | 26 | return action.results | 30 | return action.results |
481 | 27 | 31 | ||
482 | 28 | async def remote_object(self, imports, remote_cmd, target): | 32 | async def remote_object(self, imports, remote_cmd, target): |
485 | 29 | ''' | 33 | """Run command on target machine and returns a python object of the result. |
484 | 30 | Runs command on target machine and returns a python object of the result | ||
486 | 31 | 34 | ||
487 | 32 | :param imports: Imports needed for the command to run | 35 | :param imports: Imports needed for the command to run |
488 | 33 | :param remote_cmd: The python command to execute | 36 | :param remote_cmd: The python command to execute |
489 | 34 | :param target: Unit object or unit name string | 37 | :param target: Unit object or unit name string |
491 | 35 | ''' | 38 | """ |
492 | 36 | python3 = "python3 -c '{}'" | 39 | python3 = "python3 -c '{}'" |
493 | 37 | python_cmd = ('import pickle;' | 40 | python_cmd = ('import pickle;' |
494 | 38 | 'import base64;' | 41 | 'import base64;' |
495 | @@ -44,12 +47,11 @@ class JujuTools: | |||
496 | 44 | return pickle.loads(base64.b64decode(bytes(results['Stdout'][2:-1], 'utf8'))) | 47 | return pickle.loads(base64.b64decode(bytes(results['Stdout'][2:-1], 'utf8'))) |
497 | 45 | 48 | ||
498 | 46 | async def file_stat(self, path, target): | 49 | async def file_stat(self, path, target): |
501 | 47 | ''' | 50 | """Run stat on a file. |
500 | 48 | Runs stat on a file | ||
502 | 49 | 51 | ||
503 | 50 | :param path: File path | 52 | :param path: File path |
504 | 51 | :param target: Unit object or unit name string | 53 | :param target: Unit object or unit name string |
506 | 52 | ''' | 54 | """ |
507 | 53 | imports = 'import os;' | 55 | imports = 'import os;' |
508 | 54 | python_cmd = ('os.stat("{}")' | 56 | python_cmd = ('os.stat("{}")' |
509 | 55 | .format(path)) | 57 | .format(path)) |
510 | @@ -57,12 +59,11 @@ class JujuTools: | |||
511 | 57 | return await self.remote_object(imports, python_cmd, target) | 59 | return await self.remote_object(imports, python_cmd, target) |
512 | 58 | 60 | ||
513 | 59 | async def file_contents(self, path, target): | 61 | async def file_contents(self, path, target): |
516 | 60 | ''' | 62 | """Return the contents of a file. |
515 | 61 | Returns the contents of a file | ||
517 | 62 | 63 | ||
518 | 63 | :param path: File path | 64 | :param path: File path |
519 | 64 | :param target: Unit object or unit name string | 65 | :param target: Unit object or unit name string |
521 | 65 | ''' | 66 | """ |
522 | 66 | cmd = 'cat {}'.format(path) | 67 | cmd = 'cat {}'.format(path) |
523 | 67 | result = await self.run_command(cmd, target) | 68 | result = await self.run_command(cmd, target) |
524 | 68 | return result['Stdout'] | 69 | return result['Stdout'] |
525 | diff --git a/tests/functional/test_logrotate.py b/tests/functional/test_logrotate.py | |||
526 | index b4402be..100690f 100644 | |||
527 | --- a/tests/functional/test_logrotate.py | |||
528 | +++ b/tests/functional/test_logrotate.py | |||
529 | @@ -1,6 +1,8 @@ | |||
530 | 1 | #!/usr/bin/python3.6 | 1 | #!/usr/bin/python3.6 |
531 | 2 | """Main module for functional testing.""" | ||
532 | 2 | 3 | ||
533 | 3 | import os | 4 | import os |
534 | 5 | |||
535 | 4 | import pytest | 6 | import pytest |
536 | 5 | 7 | ||
537 | 6 | pytestmark = pytest.mark.asyncio | 8 | pytestmark = pytest.mark.asyncio |
538 | @@ -16,7 +18,7 @@ SERIES = ['xenial', | |||
539 | 16 | @pytest.fixture(scope='module', | 18 | @pytest.fixture(scope='module', |
540 | 17 | params=SERIES) | 19 | params=SERIES) |
541 | 18 | async def deploy_app(request, model): | 20 | async def deploy_app(request, model): |
543 | 19 | '''Deploys the logrotate charm as a subordinate of ubuntu''' | 21 | """Deploy the logrotate charm as a subordinate of ubuntu.""" |
544 | 20 | release = request.param | 22 | release = request.param |
545 | 21 | 23 | ||
546 | 22 | await model.deploy( | 24 | await model.deploy( |
547 | @@ -42,7 +44,7 @@ async def deploy_app(request, model): | |||
548 | 42 | 44 | ||
549 | 43 | @pytest.fixture(scope='module') | 45 | @pytest.fixture(scope='module') |
550 | 44 | async def unit(deploy_app): | 46 | async def unit(deploy_app): |
552 | 45 | '''Returns the logrotate unit we've deployed''' | 47 | """Return the logrotate unit we've deployed.""" |
553 | 46 | return deploy_app.units.pop() | 48 | return deploy_app.units.pop() |
554 | 47 | 49 | ||
555 | 48 | ######### | 50 | ######### |
556 | @@ -51,4 +53,5 @@ async def unit(deploy_app): | |||
557 | 51 | 53 | ||
558 | 52 | 54 | ||
559 | 53 | async def test_deploy(deploy_app): | 55 | async def test_deploy(deploy_app): |
560 | 56 | """Tst the deployment.""" | ||
561 | 54 | assert deploy_app.status == 'active' | 57 | assert deploy_app.status == 'active' |
562 | diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py | |||
563 | index 534e654..8520709 100644 | |||
564 | --- a/tests/unit/conftest.py | |||
565 | +++ b/tests/unit/conftest.py | |||
566 | @@ -1,11 +1,15 @@ | |||
567 | 1 | #!/usr/bin/python3 | 1 | #!/usr/bin/python3 |
568 | 2 | """Configurations for tests.""" | ||
569 | 3 | |||
570 | 2 | import mock | 4 | import mock |
571 | 5 | |||
572 | 3 | import pytest | 6 | import pytest |
573 | 4 | 7 | ||
574 | 5 | # If layer options are used, add this to ${fixture} | 8 | # If layer options are used, add this to ${fixture} |
575 | 6 | # and import layer in logrotate | 9 | # and import layer in logrotate |
576 | 7 | @pytest.fixture | 10 | @pytest.fixture |
577 | 8 | def mock_layers(monkeypatch): | 11 | def mock_layers(monkeypatch): |
578 | 12 | """Layers mock.""" | ||
579 | 9 | import sys | 13 | import sys |
580 | 10 | sys.modules['charms.layer'] = mock.Mock() | 14 | sys.modules['charms.layer'] = mock.Mock() |
581 | 11 | sys.modules['reactive'] = mock.Mock() | 15 | sys.modules['reactive'] = mock.Mock() |
582 | @@ -21,8 +25,10 @@ def mock_layers(monkeypatch): | |||
583 | 21 | 25 | ||
584 | 22 | monkeypatch.setattr('lib_logrotate.layer.options', options) | 26 | monkeypatch.setattr('lib_logrotate.layer.options', options) |
585 | 23 | 27 | ||
586 | 28 | |||
587 | 24 | @pytest.fixture | 29 | @pytest.fixture |
588 | 25 | def mock_hookenv_config(monkeypatch): | 30 | def mock_hookenv_config(monkeypatch): |
589 | 31 | """Hookenv mock.""" | ||
590 | 26 | import yaml | 32 | import yaml |
591 | 27 | 33 | ||
592 | 28 | def mock_config(): | 34 | def mock_config(): |
593 | @@ -39,17 +45,22 @@ def mock_hookenv_config(monkeypatch): | |||
594 | 39 | 45 | ||
595 | 40 | monkeypatch.setattr('lib_logrotate.hookenv.config', mock_config) | 46 | monkeypatch.setattr('lib_logrotate.hookenv.config', mock_config) |
596 | 41 | 47 | ||
597 | 48 | |||
598 | 42 | @pytest.fixture | 49 | @pytest.fixture |
599 | 43 | def mock_remote_unit(monkeypatch): | 50 | def mock_remote_unit(monkeypatch): |
600 | 51 | """Remote unit mock.""" | ||
601 | 44 | monkeypatch.setattr('lib_logrotate.hookenv.remote_unit', lambda: 'unit-mock/0') | 52 | monkeypatch.setattr('lib_logrotate.hookenv.remote_unit', lambda: 'unit-mock/0') |
602 | 45 | 53 | ||
603 | 46 | 54 | ||
604 | 47 | @pytest.fixture | 55 | @pytest.fixture |
605 | 48 | def mock_charm_dir(monkeypatch): | 56 | def mock_charm_dir(monkeypatch): |
606 | 57 | """Charm dir mock.""" | ||
607 | 49 | monkeypatch.setattr('lib_logrotate.hookenv.charm_dir', lambda: '/mock/charm/dir') | 58 | monkeypatch.setattr('lib_logrotate.hookenv.charm_dir', lambda: '/mock/charm/dir') |
608 | 50 | 59 | ||
609 | 60 | |||
610 | 51 | @pytest.fixture | 61 | @pytest.fixture |
611 | 52 | def logrotate(tmpdir, mock_hookenv_config, mock_charm_dir, monkeypatch): | 62 | def logrotate(tmpdir, mock_hookenv_config, mock_charm_dir, monkeypatch): |
612 | 63 | """Logrotate fixture.""" | ||
613 | 53 | from lib_logrotate import LogrotateHelper | 64 | from lib_logrotate import LogrotateHelper |
614 | 54 | helper = LogrotateHelper | 65 | helper = LogrotateHelper |
615 | 55 | 66 | ||
616 | diff --git a/tests/unit/test_logrotate.py b/tests/unit/test_logrotate.py | |||
617 | index 1f0ed9b..b01c7b5 100644 | |||
618 | --- a/tests/unit/test_logrotate.py | |||
619 | +++ b/tests/unit/test_logrotate.py | |||
620 | @@ -1,37 +1,45 @@ | |||
622 | 1 | from unittest.mock import patch | 1 | """Main unit test module.""" |
623 | 2 | |||
624 | 2 | 3 | ||
625 | 3 | class TestLogrotateHelper(): | 4 | class TestLogrotateHelper(): |
626 | 5 | """Main test class.""" | ||
627 | 6 | |||
628 | 4 | def test_pytest(self): | 7 | def test_pytest(self): |
629 | 8 | """Simple pytest.""" | ||
630 | 5 | assert True | 9 | assert True |
631 | 6 | 10 | ||
632 | 7 | |||
633 | 8 | def test_daily_retention_count(self, logrotate): | 11 | def test_daily_retention_count(self, logrotate): |
634 | 12 | """Test daily retention count.""" | ||
635 | 9 | logrotate.retention = 90 | 13 | logrotate.retention = 90 |
636 | 10 | contents = '/var/log/some.log {\n rotate 123\n daily\n}' | 14 | contents = '/var/log/some.log {\n rotate 123\n daily\n}' |
638 | 11 | count = logrotate.calculate_count(contents) | 15 | count = logrotate.calculate_count(contents, logrotate.retention) |
639 | 12 | assert count == 90 | 16 | assert count == 90 |
640 | 13 | 17 | ||
641 | 14 | def test_weekly_retention_count(self, logrotate): | 18 | def test_weekly_retention_count(self, logrotate): |
642 | 19 | """Test weekly retention count.""" | ||
643 | 15 | logrotate.retention = 21 | 20 | logrotate.retention = 21 |
644 | 16 | contents = '/var/log/some.log {\n rotate 123\n weekly\n}' | 21 | contents = '/var/log/some.log {\n rotate 123\n weekly\n}' |
646 | 17 | count = logrotate.calculate_count(contents) | 22 | count = logrotate.calculate_count(contents, logrotate.retention) |
647 | 18 | assert count == 3 | 23 | assert count == 3 |
648 | 19 | 24 | ||
649 | 20 | def test_monthly_retention_count(self, logrotate): | 25 | def test_monthly_retention_count(self, logrotate): |
650 | 26 | """Test monthly retention count.""" | ||
651 | 21 | logrotate.retention = 60 | 27 | logrotate.retention = 60 |
652 | 22 | contents = '/var/log/some.log {\n rotate 123\n monthly\n}' | 28 | contents = '/var/log/some.log {\n rotate 123\n monthly\n}' |
654 | 23 | count = logrotate.calculate_count(contents) | 29 | count = logrotate.calculate_count(contents, logrotate.retention) |
655 | 24 | assert count == 2 | 30 | assert count == 2 |
656 | 25 | 31 | ||
657 | 26 | def test_yearly_retention_count(self, logrotate): | 32 | def test_yearly_retention_count(self, logrotate): |
658 | 33 | """Test yearly retention count.""" | ||
659 | 27 | logrotate.retention = 180 | 34 | logrotate.retention = 180 |
660 | 28 | contents = '/var/log/some.log {\n rotate 123\n yearly\n}' | 35 | contents = '/var/log/some.log {\n rotate 123\n yearly\n}' |
662 | 29 | count = logrotate.calculate_count(contents) | 36 | count = logrotate.calculate_count(contents, logrotate.retention) |
663 | 30 | assert count == 1 | 37 | assert count == 1 |
664 | 31 | 38 | ||
665 | 32 | def test_modify_content(self, logrotate): | 39 | def test_modify_content(self, logrotate): |
666 | 40 | """Test the modify_content method.""" | ||
667 | 33 | logrotate.retention = 42 | 41 | logrotate.retention = 42 |
671 | 34 | contents = '/var/log/some.log {\n rotate 123\n daily\n}\n/var/log/other.log {\n rotate 456\n weekly\n}' | 42 | contents = '/log/some.log {\n rotate 123\n daily\n}\n/log/other.log {\n rotate 456\n weekly\n}' |
672 | 35 | mod_contents = logrotate.modify_content(contents) | 43 | mod_contents = logrotate.modify_content(logrotate, contents) |
673 | 36 | expected_contents = '/var/log/some.log {\n rotate 42\n daily\n}\n\n/var/log/other.log {\n rotate 6\n weekly\n}\n' | 44 | expected_contents = '/log/some.log {\n rotate 42\n daily\n}\n\n/log/other.log {\n rotate 6\n weekly\n}\n' |
674 | 37 | assert mod_contents == expected_contents | 45 | assert mod_contents == expected_contents |
This merge proposal is being monitored by mergebot. Change the status to Approved to merge.