Merge lp:~julian-edwards/maas/touch-maasmeta into lp:~maas-committers/maas/trunk

[Julian Edwards]

When checking maas.meta, update its utime so we can determine elsewhere the last time the import ran

[Jeroen T. Vermeulen (jtv)]

Doesn't seem right to hide "and oh yes, modify its timestamp" in a function called meta_contains. Why not do this one level up, in import_images?

    if meta_contains(storage, meta_file_content):
        # No changes. Update the meta file's timestamp so that future
        # import runs can see that we checked, but otherwise we're done.
        update_meta_timestamp(storage)
        return

Jeroen

review: Needs Fixing

[Jeroen T. Vermeulen (jtv)]

By the way, we have a testing helper called age_file that might save you some work.

[Raphaël Badin (rvb)]

Nice! Two tiny remarks (see inline).

review: Approve

[Jeroen T. Vermeulen (jtv)]

You're cramming three tests into a single test method whose name says it will only do one of them. Probably just a sign that your setup is too expensive to do in-line.

[Julian Edwards (julian-edwards)]

On Friday 25 Jul 2014 13:31:55 you wrote:
> Review: Needs Fixing
>
> Doesn't seem right to hide "and oh yes, modify its timestamp" in a function
> called meta_contains. Why not do this one level up, in import_images?

I disagree, I want the timestamp changed *every single time* someone even
looks at this file.

[Julian Edwards (julian-edwards)]

On Friday 25 Jul 2014 13:32:58 you wrote:
> By the way, we have a testing helper called age_file that might save you
> some work.

Ah didn't know about that, thanks.

[Julian Edwards (julian-edwards)]

On Friday 25 Jul 2014 13:34:48 you wrote:
> You're cramming three tests into a single test method whose name says it
> will only do one of them. Probably just a sign that your setup is too
> expensive to do in-line.

I'm doing expensive set up because there were no existing tests AT ALL for
this function.

Just for you, I've reworked the tests!

[Julian Edwards (julian-edwards)]

Jeroen, I wanted you to re-check but you're not on IRC so since rvb approved it I'm going to land it. If you want more changes done I'm happy to oblige later.

[Jeroen T. Vermeulen (jtv)]

Much better, thanks. If you really have to keep the side effect, rename the function to something that clearly expresses its new meaning: "update the meta file's modification timestamp, and return whether it contains exactly this data." Just don't blame me if that's messy. :-)

In the test, just to get rid of spurious failures at midnight UTC on those pesky filesystems that have a timestamp resolution of a full second, consider subtracting timestamps and checking that the difference is less than a timedelta of one day. Also, if getmtime is awkward, consider using the get_write_time helper that's defined just below age_file. I don't think the cast to int is needed.

review: Approve

[Julian Edwards (julian-edwards)]

On Friday 25 Jul 2014 14:23:53 you wrote:
> Review: Approve
>
> Much better, thanks. If you really have to keep the side effect, rename the
> function to something that clearly expresses its new meaning: "update the
> meta file's modification timestamp, and return whether it contains exactly
> this data." Just don't blame me if that's messy. :-)

Yeah I did think of doing that and pushed the fsckit button too quickly.

> In the test, just to get rid of spurious failures at midnight UTC on those
> pesky filesystems that have a timestamp resolution of a full second,
> consider subtracting timestamps and checking that the difference is less
> than a timedelta of one day. Also, if getmtime is awkward, consider using
> the get_write_time helper that's defined just below age_file. I don't
> think the cast to int is needed.

I must admit after I landed it I wondered about the failures around midnight.
There's an assertAlmostEquals which I'll drop in on the next iteration!

Cheers.

=== modified file 'src/provisioningserver/import_images/boot_resources.py'
--- src/provisioningserver/import_images/boot_resources.py	2014-07-14 14:31:14 +0000
+++ src/provisioningserver/import_images/boot_resources.py	2014-07-25 13:51:33 +0000
@@ -143,12 +143,17 @@
     """Does the `maas.meta` file match `content`?
 
     If the file's contents match the latest data, there is no need to update.
+
+    The file's timestamp is also updated to now to reflect the last time
+    that this import was run.
     """
     current_meta = os.path.join(storage, 'current', 'maas.meta')
-    return (
-        os.path.isfile(current_meta) and
-        content == read_text_file(current_meta)
-        )
+    exists = os.path.isfile(current_meta)
+    if exists:
+        # Touch file to the current timestamp so that the last time this
+        # import ran can be determined.
+        os.utime(current_meta, None)
+    return exists and content == read_text_file(current_meta)
 
 
 def update_current_symlink(storage, latest_snapshot):

=== modified file 'src/provisioningserver/import_images/tests/test_boot_resources.py'
--- src/provisioningserver/import_images/tests/test_boot_resources.py	2014-07-14 14:31:14 +0000
+++ src/provisioningserver/import_images/tests/test_boot_resources.py	2014-07-25 13:51:33 +0000
@@ -15,6 +15,10 @@
 __all__ = []
 
 import errno
+from datetime import (
+    datetime,
+    timedelta,
+    )
 import hashlib
 import json
 import os
@@ -30,6 +34,7 @@
     MockCalledWith,
     )
 from maastesting.testcase import MAASTestCase
+from maastesting.utils import age_file
 import mock
 from provisioningserver.boot import BootMethodRegistry
 import provisioningserver.config
@@ -388,6 +393,43 @@
             boot_resources.main, self.make_args(sources="", sources_file=""))
 
 
+class TestMetaContains(MAASTestCase):
+    """Tests for the `meta_contains` function."""
+
+    def make_meta_file(self, content=None):
+        if content is None:
+            content = factory.make_string()
+        storage = self.make_dir()
+        current = os.path.join(storage, 'current')
+        os.mkdir(current)
+        return storage, factory.make_file(current, 'maas.meta', content)
+
+    def test_matching_content_is_compared_True(self):
+        content = factory.make_string()
+        storage, meta_file = self.make_meta_file(content)
+        self.assertTrue(boot_resources.meta_contains(storage, content))
+
+    def test_mismatching_content_is_compared_False(self):
+        content = factory.make_string()
+        storage, meta_file = self.make_meta_file()
+        self.assertFalse(boot_resources.meta_contains(storage, content))
+
+    def test_meta_contains_updates_file_timestamp(self):
+        content = factory.make_string()
+        storage, meta_file = self.make_meta_file(content)
+
+        # Change the file's timestamp to a week ago.
+        one_week_ago = timedelta(weeks=1).total_seconds()
+        age_file(meta_file, one_week_ago)
+
+        boot_resources.meta_contains(storage, content)
+
+        # Check the timestamp was updated.
+        expected_date = datetime.now()
+        actual_date = datetime.fromtimestamp(int(os.path.getmtime(meta_file)))
+        self.assertEqual(expected_date.day, actual_date.day)
+
+
 class TestParseSources(MAASTestCase):
     """Tests for the `parse_sources` function."""