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

[Mert Kirpici]

Removed duplicated header in logrotate config file

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

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

[Eric Chen (eric-chen)]

Could we add some unit test for this function? because the behavior is changed.

And please attach the log of unit/func test by pastebin. Currently , CI bot is not integrated, we need to run it manually and attach the result.

I also leave some comment below. Thanks!

review: Needs Information

[🤖 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:546540d30293592a2b01b37b883421930b999808
https://jenkins.canonical.com/bootstack/job/lp-charm-logrotated-ci/7/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/725/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/228/

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

review: Approve (continuous-integration)

[Robert Gildein (rgildein)]

Suggestion to make it simpler.

[Mert Kirpici (mertkirpici)]

> Suggestion to make it simpler.

Hey, thanks for the suggestion. I do agree it does make it simpler. I will apply it. Marking this in progress since I want to test it thoroughly.

[JamesLin (jneo8)]

+1 for Eric's comment.

review: Needs Fixing

[JamesLin (jneo8)]

review: Needs Information

[JamesLin (jneo8)]

LGTM

review: Approve

[Robert Gildein (rgildein)]

LGTM, Thanks

review: Approve

[Mert Kirpici (mertkirpici)]

Applied the suggestion, also changed the os.linesep into "\n" since we are writing to the file in text mode, upon reading this[0] in the python docs.

[0] https://docs.python.org/3/library/os.html#os.linesep
...
Do not use os.linesep as a line terminator when writing files opened in text mode (the default); use a single '\n' instead, on all platforms
...

Thanks for the reviews. If CI comes clean, I think we can land this.

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

PASSED: Continuous integration, rev:fecd39f8e9feac48e708e8141816a8e4acef3c6a
https://jenkins.canonical.com/bootstack/job/lp-charm-logrotated-ci/11/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/736/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/238/

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

review: Approve (continuous-integration)

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

Change successfully merged at revision e0b3d4381624e3e48f5a56cefd51fd24af4b9f1b

diff --git a/src/lib/lib_logrotate.py b/src/lib/lib_logrotate.py
index 7b60c25..c413b8b 100644
--- a/src/lib/lib_logrotate.py
+++ b/src/lib/lib_logrotate.py
@@ -114,14 +114,10 @@ class LogrotateHelper:
         header = (
             "# Configuration file maintained by Juju. Local changes may be overwritten"
         )
-
-        split = content.split("\n")
-        if split[0].startswith(header):
-            result = content
-        else:
-            result = header + "\n" + content
-
-        return result
+        content = [
+            row for row in content.splitlines() if row and not row.startswith(header)
+        ]
+        return "\n".join([header, *content]) + "\n"
 
     @classmethod
     def calculate_count(cls, item, retention):
diff --git a/src/tests/unit/test_logrotate.py b/src/tests/unit/test_logrotate.py
index c0c5cdb..08797ce 100644
--- a/src/tests/unit/test_logrotate.py
+++ b/src/tests/unit/test_logrotate.py
@@ -138,6 +138,22 @@ class TestLogrotateHelper:
         )
         assert mod_contents == expected_contents
 
+    @pytest.mark.parametrize("header_count", [0, 1, 2, 10])
+    def test_modify_header(self, logrotate, header_count):
+        """Test the modify_header method works and is idempotent."""
+        header = "# Configuration file maintained by Juju. Local changes may be overwritten\n"  # noqa
+        content_body = (
+            "\n/log/some.log {\n  rotate 42\n  daily\n}\n\n"
+            "/log/other.log {\n  rotate 6\n  weekly\n}\n"
+        )
+        content = (header * header_count) + content_body
+        expected_content = (
+            header + "/log/some.log {\n  rotate 42\n  daily\n}\n"
+            "/log/other.log {\n  rotate 6\n  weekly\n}\n"
+        )
+        modified_content = logrotate.modify_header(logrotate, content)
+        assert modified_content == expected_content
+
 
 class TestCronHelper:
     """Main cron test class."""