Merge ~mertkirpici/charm-logrotated:lp/2017795 into charm-logrotated:master

[Mert Kirpici]

LP #2017795

fix: cleanup old cronjobs upon config change

    A change in cronjob frequency used to leave the old cronjobs behind,
    causing unnecessary execution. This patch is mitigating that behavior
    and also adds some tests.

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

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

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

PASSED: Continuous integration, rev:3875d18e6a5773449369d2c8c2d6af7135c6978a
https://jenkins.canonical.com/bootstack/job/lp-charm-logrotated-ci/19/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/805/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/257/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-logrotated-ci/19//rebuild

review: Approve (continuous-integration)

[Erhan Sunar (esunar)]

LGTM

review: Approve

[Chi Wai CHAN (raychan96)]

LGTM

review: Approve

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

Failed to merge change (unable to merge source repository due to conflicts), setting status to needs review.

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

FAILED: Continuous integration, rev:e436221e699d5dd85221096a0bed16d973d6529b
https://jenkins.canonical.com/bootstack/job/lp-charm-logrotated-ci/20/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/806/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/259/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-logrotated-ci/20//rebuild

review: Needs Fixing (continuous-integration)

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

FAILED: Continuous integration, rev:e436221e699d5dd85221096a0bed16d973d6529b
https://jenkins.canonical.com/bootstack/job/lp-charm-logrotated-ci/21/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/807/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/260/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-logrotated-ci/21//rebuild

review: Needs Fixing (continuous-integration)

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

PASSED: Continuous integration, rev:7316795355629d4ebcfe8a2b76f5faf0f5b07d23
https://jenkins.canonical.com/bootstack/job/lp-charm-logrotated-ci/22/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/808/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/261/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-logrotated-ci/22//rebuild

review: Approve (continuous-integration)

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

Change successfully merged at revision 55f7ed6f91b9a56f0241e68a4ef5c6cd75fd5492

diff --git a/src/lib/lib_cron.py b/src/lib/lib_cron.py
index be3dff9..1f30ddc 100644
--- a/src/lib/lib_cron.py
+++ b/src/lib/lib_cron.py
@@ -48,13 +48,13 @@ class CronHelper:
         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
+        self.cleanup_cronjob_files()
 
         if self.cronjob_enabled is True:
             cronjob_path = os.path.realpath(__file__)
             cron_file_path = (
                 self.cronjob_base_path
-                + self.cronjob_check_paths[clean_up_file]
+                + self.cronjob_check_paths[self.cronjob_frequency]
                 + "/"
                 + self.cronjob_logrotate_cron_file
             )
@@ -73,25 +73,24 @@ class CronHelper:
             os.chmod(cron_file_path, 700)
 
             # update cron.daily schedule if logrotate-cronjob-frequency set to "daily"
-            if self.validate_cron_conf() and self.cronjob_frequency == 1:
+            if self.cronjob_frequency == 1 and self.validate_cron_conf():
                 self.update_cron_daily_schedule()
+        else:
+            self.cleanup_etc_config()
 
-        self.cleanup_cronjob(clean_up_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)
+    def cleanup_cronjob_files(self):
+        """Cleanup previous cronjob files."""
+        for check_path in self.cronjob_check_paths:
+            path = os.path.join(
+                self.cronjob_base_path + check_path, self.cronjob_logrotate_cron_file
+            )
+            if os.path.exists(path):
+                os.remove(path)
+
+    def cleanup_etc_config(self):
+        """Cleanup the saved config in /etc directory."""
+        if os.path.exists(self.cronjob_etc_config):
+            os.remove(self.cronjob_etc_config)
 
     def update_logrotate_etc(self):
         """Run logrotate update config."""
diff --git a/src/tests/functional/test_logrotate.py b/src/tests/functional/test_logrotate.py
index 4aa0aa6..0afbb82 100644
--- a/src/tests/functional/test_logrotate.py
+++ b/src/tests/functional/test_logrotate.py
@@ -112,6 +112,27 @@ async def test_configure_cron_daily(deploy_app):
     assert deploy_app.status == "active"
 
 
+async def test_reconfigure_cronjob_frequency(model, deploy_app, unit, jujutools):
+    """Test reconfiguration of cronjob frequency."""
+    await deploy_app.set_config({"logrotate-cronjob-frequency": "weekly"})
+    await model.block_until(lambda: deploy_app.status == "active")
+    config = await deploy_app.get_config()
+
+    result = await jujutools.run_command(
+        "test -f /etc/cron.weekly/charm-logrotate", unit
+    )
+    weekly_cronjob_exists = result["return-code"] == 0
+
+    result = await jujutools.run_command(
+        "test -f /etc/cron.daily/charm-logrotate", unit
+    )
+    daily_cronjob_exists = result["return-code"] == 0
+
+    assert config["logrotate-cronjob-frequency"]["value"] == "weekly"
+    assert not daily_cronjob_exists
+    assert weekly_cronjob_exists
+
+
 async def test_configure_override_01(model, deploy_app, jujutools, unit):
     """Test configuring override for the deployment (interval)."""
     test_path = "/etc/logrotate.d/apt"
diff --git a/src/tests/unit/conftest.py b/src/tests/unit/conftest.py
index 1f15b42..9eff063 100644
--- a/src/tests/unit/conftest.py
+++ b/src/tests/unit/conftest.py
@@ -1,7 +1,7 @@
 #!/usr/bin/python3
 """Configurations for tests."""
 
-import mock
+from unittest import mock
 
 import pytest
 
@@ -54,6 +54,12 @@ def mock_remote_unit(monkeypatch):
     monkeypatch.setattr("lib_logrotate.hookenv.remote_unit", lambda: "unit-mock/0")
 
 
[e-mail]()
+def mock_local_unit(monkeypatch):
+    """Local unit mock."""
+    monkeypatch.setattr("lib_logrotate.hookenv.local_unit", lambda: "unit-logrotated/0")
+
+
 @pytest.fixture
 def mock_charm_dir(monkeypatch):
     """Charm dir mock."""
diff --git a/src/tests/unit/requirements.txt b/src/tests/unit/requirements.txt
index 9c685e5..3d4047b 100644
--- a/src/tests/unit/requirements.txt
+++ b/src/tests/unit/requirements.txt
@@ -1,5 +1,5 @@
 charmhelpers
 charms.reactive
-mock
 pytest
 pytest-cov
+pytest-mock
\ No newline at end of file
diff --git a/src/tests/unit/test_logrotate.py b/src/tests/unit/test_logrotate.py
index 0c89bbc..8d329e5 100644
--- a/src/tests/unit/test_logrotate.py
+++ b/src/tests/unit/test_logrotate.py
@@ -1,6 +1,7 @@
 """Main unit test module."""
-
 import json
+import os
+from textwrap import dedent
 from unittest import mock
 
 from lib_logrotate import LogrotateHelper
@@ -279,3 +280,72 @@ class TestCronHelper:
             cron_config.validate_cron_conf()
 
         assert err.type == cron_config.InvalidCronConfig
+
+    def test_install_cronjob(self, cron, mock_local_unit, mocker):
+        """Test install cronjob method."""
+        mock_charm_dir = "/mock/unit-logrotated-0/charm"
+        mock_exists = mocker.patch("lib_cron.os.path.exists", return_value=True)
+        mock_remove = mocker.patch("lib_cron.os.remove")
+        mock_chmod = mocker.patch("lib_cron.os.chmod")
+        mocker.patch(
+            "lib_cron.os.path.realpath",
+            return_value=os.path.join(mock_charm_dir, "lib/lib_cron.py"),
+        )
+        mocker.patch("lib_cron.os.getcwd", return_value=mock_charm_dir)
+        mock_open = mocker.patch("lib_cron.open")
+        mock_handle = mock_open.return_value
+
+        expected_files_to_be_removed = [
+            "/etc/cron.hourly/charm-logrotate",
+            "/etc/cron.daily/charm-logrotate",
+            "/etc/cron.weekly/charm-logrotate",
+            "/etc/cron.monthly/charm-logrotate",
+        ]
+
+        cron_config = cron()
+        cron_config.cronjob_enabled = True
+        cron_config.cronjob_frequency = 2
+        cron_config.install_cronjob()
+
+        mock_exists.assert_has_calls(
+            [mock.call(file) for file in expected_files_to_be_removed], any_order=True
+        )
+        mock_remove.assert_has_calls(
+            [mock.call(file) for file in expected_files_to_be_removed], any_order=True
+        )
+        mock_open.assert_called_once_with("/etc/cron.weekly/charm-logrotate", "w")
+        mock_handle.write.assert_called_once_with(
+            dedent(
+                """\
+                #!/bin/bash
+                /usr/bin/sudo /usr/bin/juju-run unit-logrotated/0 "/mock/unit-logrotated-0/.venv/bin/python3 /mock/unit-logrotated-0/charm/lib/lib_cron.py"
+                """  # noqa
+            )
+        )
+        mock_handle.close.assert_called_once()
+        mock_chmod.assert_called_once_with("/etc/cron.weekly/charm-logrotate", 700)
+
+    def test_install_cronjob_removes_etc_config_when_cronjob_disabled(
+        self, cron, mocker
+    ):
+        """Test that all cronjob related files created upon cronjobs being disabled."""
+        mock_exists = mocker.patch("lib_cron.os.path.exists", return_value=True)
+        mock_remove = mocker.patch("lib_cron.os.remove")
+
+        expected_files_to_be_removed = [
+            "/etc/cron.hourly/charm-logrotate",
+            "/etc/cron.daily/charm-logrotate",
+            "/etc/cron.weekly/charm-logrotate",
+            "/etc/cron.monthly/charm-logrotate",
+            "/etc/logrotate_cronjob_config",
+        ]
+        cron_config = cron()
+        cron_config.cronjob_enabled = False
+        cron_config.install_cronjob()
+
+        mock_exists.assert_has_calls(
+            [mock.call(file) for file in expected_files_to_be_removed], any_order=True
+        )
+        mock_remove.assert_has_calls(
+            [mock.call(file) for file in expected_files_to_be_removed], any_order=True
+        )