Merge ~logrotate-charmers/charm-logrotated:bugs_1833095_1833093 into charm-logrotated:master

[Jeremy Lounder]

LP#1833095 LP#1833093 Fixes linting and unit test issues.

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

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

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

Unable to determine commit message from repository - please click "Set commit message" and enter the commit message manually.

[Paul Goins (vultaire)]

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

[Paul Goins (vultaire)]

Adding one more comment

[Paul Goins (vultaire)]

Created bug regarding this code review: https://pad.lv/1864572

As this code is already merged into the current released charm, am merging this as-is.

review: Approve

diff --git a/actions/__init__.py b/actions/__init__.py
deleted file mode 100755
index 9b088de..0000000
--- a/actions/__init__.py
+++ /dev/null
@@ -1,13 +0,0 @@
-# Copyright 2016 Canonical Ltd
-#
-# Licensed under the Apache License, Version 2.0 (the "License");
-# you may not use this file except in compliance with the License.
-# You may obtain a copy of the License at
-#
-#  http://www.apache.org/licenses/LICENSE-2.0
-#
-# Unless required by applicable law or agreed to in writing, software
-# distributed under the License is distributed on an "AS IS" BASIS,
-# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-# See the License for the specific language governing permissions and
-# limitations under the License.
diff --git a/actions/actions.py b/actions/actions.py
index aee6bb7..8178850 100755
--- a/actions/actions.py
+++ b/actions/actions.py
@@ -1,45 +1,36 @@
 #!/usr/bin/env python3
-#
-# Copyright 2016 Canonical Ltd
-#
-# Licensed under the Apache License, Version 2.0 (the "License");
-# you may not use this file except in compliance with the License.
-# You may obtain a copy of the License at
-#
-#  http://www.apache.org/licenses/LICENSE-2.0
-#
-# Unless required by applicable law or agreed to in writing, software
-# distributed under the License is distributed on an "AS IS" BASIS,
-# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-# See the License for the specific language governing permissions and
-# limitations under the License.
+"""Actions module."""
 
 import os
 import sys
 
-sys.path.insert(0, os.path.join(os.environ['CHARM_DIR'], 'lib'))
-from charmhelpers.core import (
-    hookenv,
-    host,
-)
+from charmhelpers.core import hookenv
 
-from lib_logrotate import LogrotateHelper
 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()
 
+
 @hooks.hook("update-logrotate-files")
 def update_logrotate_files():
+    """Update the logrotate files."""
     logrotate.read_config()
     logrotate.modify_configs()
 
+
 @hooks.hook("update-cronjob")
 def update_cronjob():
+    """Update the cronjob file."""
     cron.read_config()
     cron.install_cronjob()
 
+
 if __name__ == "__main__":
+    """Main function."""
     hooks.execute(sys.argv)
-
diff --git a/lib/lib_cron.py b/lib/lib_cron.py
index 15bcdaf..f560084 100644
--- a/lib/lib_cron.py
+++ b/lib/lib_cron.py
@@ -1,24 +1,27 @@
+"""Cron helper module."""
 import os
-import re
+
 from lib_logrotate import LogrotateHelper
 
+
 class CronHelper:
     """Helper class for logrotate charm."""
 
-    @classmethod
     def __init__(self):
-        """Init function"""
-        self.cronjob_check_paths = [ "hourly", "daily", "weekly", "monthly" ]
+        """Init function."""
+        self.cronjob_base_path = "/etc/cron."
+        self.cronjob_etc_config = "/etc/logrotate_cronjob_config"
+        self.cronjob_check_paths = ["hourly", "daily", "weekly", "monthly"]
         self.cronjob_logrotate_cron_file = "charm-logrotate"
 
-
-    @classmethod
     def read_config(self):
-        """Config changed/install hooks dumps config out to disk,
-        Here we read that config to update the cronjob"""
+        """Read the configuration from the file.
 
-        config_file = open("/etc/logrotate_cronjob_config", "r")
-        lines = config_file.read() 
+        Config changed/install hooks dumps config out to disk,
+        Here we read that config to update the cronjob.
+        """
+        config_file = open(self.cronjob_etc_config, "r")
+        lines = config_file.read()
         lines = lines.split('\n')
 
         if lines[0] == 'True':
@@ -28,51 +31,54 @@ class CronHelper:
 
         self.cronjob_frequency = int(self.cronjob_check_paths.index(lines[1]))
 
-
-    @classmethod
     def install_cronjob(self):
-        """If logrotate-cronjob config option is set to True 
-           install cronjob.  Otherwise cleanup"""
+        """Install the cron job task.
 
+        If logrotate-cronjob config option is set to True install cronjob,
+        otherwise cleanup.
+        """
         clean_up_file = self.cronjob_frequency if self.cronjob_enabled else -1
 
         if self.cronjob_enabled is True:
             path_to_lib = os.path.realpath(__file__)
-            cron_file_path = "/etc/cron." + self.cronjob_check_paths[clean_up_file] \
-                             + "/" + self.cronjob_logrotate_cron_file
+            cron_file_path = self.cronjob_base_path\
+                + self.cronjob_check_paths[clean_up_file]\
+                + "/" + self.cronjob_logrotate_cron_file
 
             # upgrade to template if logic increases
             cron_file = open(cron_file_path, 'w')
             cron_file.write("#!/bin/sh\n/usr/bin/python3 " + path_to_lib + "\n\n")
             cron_file.close()
-            os.chmod(cron_file_path,700)
-            
-        self.cleanup_cronjob(clean_up_file)
+            os.chmod(cron_file_path, 700)
 
+        self.cleanup_cronjob(clean_up_file)
 
-    @classmethod
-    def cleanup_cronjob(self, frequency = -1):
-        """Cleanup previous config"""
-        for i in range(4):
-            if frequency != i:
-                path = "/etc/cron." + self.cronjob_check_paths[i] + "/" +\
-                       self.cronjob_logrotate_cron_file
+    def cleanup_cronjob(self, frequency=-1):
+        """Cleanup previous config."""
+        if frequency == -1:
+            for check_path in self.cronjob_check_paths:
+                path = self.cronjob_base_path \
+                       + check_path \
+                       + "/" \
+                       + self.cronjob_logrotate_cron_file
                 if os.path.exists(path):
                     os.remove(path)
+            if os.path.exists(self.cronjob_etc_config):
+                os.remove(self.cronjob_etc_config)
 
-    @classmethod
     def update_logrotate_etc(self):
-        """Run logrotate update config"""
+        """Run logrotate update config."""
         logrotate = LogrotateHelper()
         logrotate.read_config()
         logrotate.modify_configs()
 
 
 def main():
-    cronHelper = CronHelper()
-    cronHelper.read_config()
-    cronHelper.update_logrotate_etc()
-    cronHelper.install_cronjob()
+    """Ran by cron."""
+    cronhelper = CronHelper()
+    cronhelper.read_config()
+    cronhelper.update_logrotate_etc()
+    cronhelper.install_cronjob()
 
 
 if __name__ == '__main__':
diff --git a/lib/lib_logrotate.py b/lib/lib_logrotate.py
index 145a41b..67245cb 100644
--- a/lib/lib_logrotate.py
+++ b/lib/lib_logrotate.py
@@ -1,6 +1,8 @@
+"""Logrotate module."""
 import os
 import re
 
+from charmhelpers.core import hookenv
 
 LOGROTATE_DIR = "/etc/logrotate.d/"
 
@@ -8,27 +10,24 @@ LOGROTATE_DIR = "/etc/logrotate.d/"
 class LogrotateHelper:
     """Helper class for logrotate charm."""
 
-    @classmethod
     def __init__(self):
-        """Init function"""
-        pass
+        """Init function."""
+        self.retention = hookenv.config('logrotate-retention')
 
-    @classmethod
     def read_config(self):
-        """Config changed/install hooks dumps config out to disk,
-        Here we read that config to update the cronjob"""
+        """Read changes from disk.
 
+        Config changed/install hooks dumps config out to disk,
+        Here we read that config to update the cronjob
+        """
         config_file = open("/etc/logrotate_cronjob_config", "r")
         lines = config_file.read()
         lines = lines.split('\n')
 
         self.retention = int(lines[2])
 
-
-    @classmethod
     def modify_configs(self):
         """Modify the logrotate config files."""
-
         for config_file in os.listdir(LOGROTATE_DIR):
             file_path = LOGROTATE_DIR + config_file
 
@@ -44,11 +43,8 @@ class LogrotateHelper:
             logrotate_file.write(mod_contents)
             logrotate_file.close()
 
-
-    @classmethod
     def modify_content(self, content):
-        """Helper function to edit the content of a logrotate file."""
-
+        """Edit the content of a logrotate file."""
         # Split the contents in a logrotate file in separate entries (if
         # multiple are found in the file) and put in a list for further
         # processing
@@ -66,7 +62,7 @@ class LogrotateHelper:
         # the rotate option to the appropriate value
         results = []
         for item in items:
-            count = self.calculate_count(item)
+            count = self.calculate_count(item, self.retention)
             rotate = 'rotate {}'.format(count)
             result = re.sub(r'rotate \d+\.?[0-9]*', rotate, item)
             results.append(result)
@@ -75,12 +71,9 @@ class LogrotateHelper:
 
         return results
 
-    @classmethod
     def modify_header(self, content):
-        """Helper function to add Juju headers to the file."""
-
-	header = ("# Configuration file maintained by Juju. "
-		 "Local changes may be overwritten")
+        """Add Juju headers to the file."""
+        header = "# Configuration file maintained by Juju. Local changes may be overwritten"
 
         split = content.split('\n')
         if split[0].startswith(header):
@@ -91,20 +84,22 @@ class LogrotateHelper:
         return result
 
     @classmethod
-    def calculate_count(self, item):
+    def calculate_count(cls, item, retention):
         """Calculate rotate based on rotation interval. Always round up."""
-
+        # Fallback to default lowest retention - days
+        # better to keep the logs than lose them
+        count = retention
         # Daily 1:1 to configuration retention period (in days)
         if 'daily' in item:
-            count = self.retention
+            count = retention
         # Weekly rounding up, as weeks are 7 days
         if 'weekly' in item:
-            count = int(round(self.retention/7))
+            count = int(round(retention/7))
         # Monthly default 30 days and round up because of 28/31 days months
         if 'monthly' in item:
-            count = int(round(self.retention/30))
+            count = int(round(retention/30))
         # For every 360 days - add 1 year
         if 'yearly' in item:
-            count = self.retention // 360 + 1 if self.retention > 360 else 1
+            count = retention // 360 + 1 if retention > 360 else 1
 
         return count
diff --git a/reactive/logrotate.py b/reactive/logrotate.py
index 4a8d5c1..1810707 100644
--- a/reactive/logrotate.py
+++ b/reactive/logrotate.py
@@ -1,36 +1,48 @@
-from lib_logrotate import LogrotateHelper
-from lib_cron import CronHelper
+"""Reactive charm hooks."""
 from charmhelpers.core import hookenv
+
 from charms.reactive import set_flag, when, when_not
 
+from lib_cron import CronHelper
+
+from lib_logrotate import LogrotateHelper
+
+
+hooks = hookenv.Hooks()
 logrotate = LogrotateHelper()
 cron = CronHelper()
 
+
 @when_not('logrotate.installed')
 def install_logrotate():
-    dump_config_to_disk();
+    """Install the logrotate charm."""
+    dump_config_to_disk()
     logrotate.read_config()
     cron.read_config()
     logrotate.modify_configs()
     hookenv.status_set('active', 'Unit is ready.')
     set_flag('logrotate.installed')
-    cron.install_cronjob();
+    cron.install_cronjob()
+
 
 @when('config.changed')
 def config_changed():
+    """Run when configuration changes."""
     dump_config_to_disk()
     cron.read_config()
     logrotate.read_config()
     hookenv.status_set('maintenance', 'Modifying configs.')
     logrotate.modify_configs()
     hookenv.status_set('active', 'Unit is ready.')
-    cron.install_cronjob();
+    cron.install_cronjob()
+
 
 def dump_config_to_disk():
+    """Dump configurations to disk."""
     cronjob_enabled = hookenv.config('logrotate-cronjob')
     cronjob_frequency = hookenv.config('logrotate-cronjob-frequency')
     logrotate_retention = hookenv.config('logrotate-retention')
     with open('/etc/logrotate_cronjob_config', 'w+') as cronjob_config_file:
-       cronjob_config_file.write(str(cronjob_enabled) + '\n')
-       cronjob_config_file.write(str(cronjob_frequency) + '\n')
-       cronjob_config_file.write(str(logrotate_retention) + '\n')
+        cronjob_config_file.write(str(cronjob_enabled) + '\n')
+        cronjob_config_file.write(str(cronjob_frequency) + '\n')
+        cronjob_config_file.write(str(logrotate_retention) + '\n')
diff --git a/tests/functional/conftest.py b/tests/functional/conftest.py
index 56925ff..1b22cb8 100644
--- a/tests/functional/conftest.py
+++ b/tests/functional/conftest.py
@@ -1,28 +1,32 @@
 #!/usr/bin/python3
-'''
-Reusable pytest fixtures for functional testing
+"""
+Reusable pytest fixtures for functional testing.
 
 Environment variables
 ---------------------
 
 test_preserve_model:
 if set, the testing model won't be torn down at the end of the testing session
-'''
+"""
 
 import asyncio
 import os
-import uuid
-import pytest
 import subprocess
+import uuid
 
 from juju.controller import Controller
+
 from juju_tools import JujuTools
 
+import pytest
+
 
 @pytest.fixture(scope='module')
 def event_loop():
-    '''Override the default pytest event loop to allow for fixtures using a
-    broader scope'''
+    """Override the default pytest event loop.
+
+    Do this too allow for fixtures using a broader scope.
+    """
     loop = asyncio.get_event_loop_policy().new_event_loop()
     asyncio.set_event_loop(loop)
     loop.set_debug(True)
@@ -33,7 +37,7 @@ def event_loop():
 
 @pytest.fixture(scope='module')
 async def controller():
-    '''Connect to the current controller'''
+    """Connect to the current controller."""
     _controller = Controller()
     await _controller.connect_current()
     yield _controller
@@ -42,7 +46,7 @@ async def controller():
 
 @pytest.fixture(scope='module')
 async def model(controller):
-    '''This model lives only for the duration of the test'''
+    """Live only for the duration of the test."""
     model_name = "functest-{}".format(str(uuid.uuid4())[-12:])
     _model = await controller.add_model(model_name,
                                         cloud_name=os.getenv('PYTEST_CLOUD_NAME'),
@@ -62,5 +66,6 @@ async def model(controller):
 
 @pytest.fixture(scope='module')
 async def jujutools(controller, model):
+    """Juju tools."""
     tools = JujuTools(controller, model)
     return tools
diff --git a/tests/functional/juju_tools.py b/tests/functional/juju_tools.py
index 2fd501d..5e4a1cc 100644
--- a/tests/functional/juju_tools.py
+++ b/tests/functional/juju_tools.py
@@ -1,22 +1,26 @@
+"""Juju tools."""
+import base64
 import pickle
+
 import juju
-import base64
 
 # from juju.errors import JujuError
 
 
 class JujuTools:
+    """Juju tools."""
+
     def __init__(self, controller, model):
+        """Init."""
         self.controller = controller
         self.model = model
 
     async def run_command(self, cmd, target):
-        '''
-        Runs a command on a unit.
+        """Run a command on a unit.
 
         :param cmd: Command to be run
         :param unit: Unit object or unit name string
-        '''
+        """
         unit = (
             target
             if isinstance(target, juju.unit.Unit)
@@ -26,13 +30,12 @@ class JujuTools:
         return action.results
 
     async def remote_object(self, imports, remote_cmd, target):
-        '''
-        Runs command on target machine and returns a python object of the result
+        """Run command on target machine and returns a python object of the result.
 
         :param imports: Imports needed for the command to run
         :param remote_cmd: The python command to execute
         :param target: Unit object or unit name string
-        '''
+        """
         python3 = "python3 -c '{}'"
         python_cmd = ('import pickle;'
                       'import base64;'
@@ -44,12 +47,11 @@ class JujuTools:
         return pickle.loads(base64.b64decode(bytes(results['Stdout'][2:-1], 'utf8')))
 
     async def file_stat(self, path, target):
-        '''
-        Runs stat on a file
+        """Run stat on a file.
 
         :param path: File path
         :param target: Unit object or unit name string
-        '''
+        """
         imports = 'import os;'
         python_cmd = ('os.stat("{}")'
                       .format(path))
@@ -57,12 +59,11 @@ class JujuTools:
         return await self.remote_object(imports, python_cmd, target)
 
     async def file_contents(self, path, target):
-        '''
-        Returns the contents of a file
+        """Return the contents of a file.
 
         :param path: File path
         :param target: Unit object or unit name string
-        '''
+        """
         cmd = 'cat {}'.format(path)
         result = await self.run_command(cmd, target)
         return result['Stdout']
diff --git a/tests/functional/test_logrotate.py b/tests/functional/test_logrotate.py
index b4402be..100690f 100644
--- a/tests/functional/test_logrotate.py
+++ b/tests/functional/test_logrotate.py
@@ -1,6 +1,8 @@
 #!/usr/bin/python3.6
+"""Main module for functional testing."""
 
 import os
+
 import pytest
 
 pytestmark = pytest.mark.asyncio
@@ -16,7 +18,7 @@ SERIES = ['xenial',
 @pytest.fixture(scope='module',
                 params=SERIES)
 async def deploy_app(request, model):
-    '''Deploys the logrotate charm as a subordinate of ubuntu'''
+    """Deploy the logrotate charm as a subordinate of ubuntu."""
     release = request.param
 
     await model.deploy(
@@ -42,7 +44,7 @@ async def deploy_app(request, model):
 
 @pytest.fixture(scope='module')
 async def unit(deploy_app):
-    '''Returns the logrotate unit we've deployed'''
+    """Return the logrotate unit we've deployed."""
     return deploy_app.units.pop()
 
 #########
@@ -51,4 +53,5 @@ async def unit(deploy_app):
 
 
 async def test_deploy(deploy_app):
+    """Tst the deployment."""
     assert deploy_app.status == 'active'
diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py
index 534e654..8520709 100644
--- a/tests/unit/conftest.py
+++ b/tests/unit/conftest.py
@@ -1,11 +1,15 @@
 #!/usr/bin/python3
+"""Configurations for tests."""
+
 import mock
+
 import pytest
 
 # If layer options are used, add this to ${fixture}
 # and import layer in logrotate
 @pytest.fixture
 def mock_layers(monkeypatch):
+    """Layers mock."""
     import sys
     sys.modules['charms.layer'] = mock.Mock()
     sys.modules['reactive'] = mock.Mock()
@@ -21,8 +25,10 @@ def mock_layers(monkeypatch):
 
     monkeypatch.setattr('lib_logrotate.layer.options', options)
 
+
 @pytest.fixture
 def mock_hookenv_config(monkeypatch):
+    """Hookenv mock."""
     import yaml
 
     def mock_config():
@@ -39,17 +45,22 @@ def mock_hookenv_config(monkeypatch):
 
     monkeypatch.setattr('lib_logrotate.hookenv.config', mock_config)
 
+
 @pytest.fixture
 def mock_remote_unit(monkeypatch):
+    """Remote unit mock."""
     monkeypatch.setattr('lib_logrotate.hookenv.remote_unit', lambda: 'unit-mock/0')
 
 
 @pytest.fixture
 def mock_charm_dir(monkeypatch):
+    """Charm dir mock."""
     monkeypatch.setattr('lib_logrotate.hookenv.charm_dir', lambda: '/mock/charm/dir')
 
+
 @pytest.fixture
 def logrotate(tmpdir, mock_hookenv_config, mock_charm_dir, monkeypatch):
+    """Logrotate fixture."""
     from lib_logrotate import LogrotateHelper
     helper = LogrotateHelper
 
diff --git a/tests/unit/test_logrotate.py b/tests/unit/test_logrotate.py
index 1f0ed9b..b01c7b5 100644
--- a/tests/unit/test_logrotate.py
+++ b/tests/unit/test_logrotate.py
@@ -1,37 +1,45 @@
-from unittest.mock import patch
+"""Main unit test module."""
+
 
 class TestLogrotateHelper():
+    """Main test class."""
+
     def test_pytest(self):
+        """Simple pytest."""
         assert True
 
-
     def test_daily_retention_count(self, logrotate):
+        """Test daily retention count."""
         logrotate.retention = 90
         contents = '/var/log/some.log {\n  rotate 123\n  daily\n}'
-        count = logrotate.calculate_count(contents)
+        count = logrotate.calculate_count(contents, logrotate.retention)
         assert count == 90
 
     def test_weekly_retention_count(self, logrotate):
+        """Test weekly retention count."""
         logrotate.retention = 21
         contents = '/var/log/some.log {\n  rotate 123\n  weekly\n}'
-        count = logrotate.calculate_count(contents)
+        count = logrotate.calculate_count(contents, logrotate.retention)
         assert count == 3
 
     def test_monthly_retention_count(self, logrotate):
+        """Test monthly retention count."""
         logrotate.retention = 60
         contents = '/var/log/some.log {\n  rotate 123\n  monthly\n}'
-        count = logrotate.calculate_count(contents)
+        count = logrotate.calculate_count(contents, logrotate.retention)
         assert count == 2
 
     def test_yearly_retention_count(self, logrotate):
+        """Test yearly retention count."""
         logrotate.retention = 180
         contents = '/var/log/some.log {\n  rotate 123\n  yearly\n}'
-        count = logrotate.calculate_count(contents)
+        count = logrotate.calculate_count(contents, logrotate.retention)
         assert count == 1
 
     def test_modify_content(self, logrotate):
+        """Test the modify_content method."""
         logrotate.retention = 42
-        contents = '/var/log/some.log {\n  rotate 123\n  daily\n}\n/var/log/other.log {\n  rotate 456\n  weekly\n}'
-        mod_contents = logrotate.modify_content(contents)
-        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'
+        contents = '/log/some.log {\n  rotate 123\n  daily\n}\n/log/other.log {\n  rotate 456\n  weekly\n}'
+        mod_contents = logrotate.modify_content(logrotate, contents)
+        expected_contents = '/log/some.log {\n  rotate 42\n  daily\n}\n\n/log/other.log {\n  rotate 6\n  weekly\n}\n'
         assert mod_contents == expected_contents