Merge ~j.w.baxter/cloud-init:append_files into cloud-init:master

[James Baxter]

write_files: add support for appending to files.

Add 'append: true' to write_files entries to append 'content' to file
specified by 'path' key. This modifies the file open mode to append.

Adds the ability to append to files

[Ryan Harper (raharper)]

James,

Thanks for the patch here. I've an inline comment/suggestion to the code. Could you also add a unittest here:

tests/unittests/test_handler/test_handler_write_files.py

review: Needs Fixing

21e0df9... by James Baxter on 2018-12-04

config: simplify write_files append and add unit test

[James Baxter (j.w.baxter)]

Thanks, have made the suggested changes!

[Ryan Harper (raharper)]

One more suggestion for the unittest. Thanks for adding that.

[James Baxter (j.w.baxter)]

Hmmmm, so I tried to make those changes, but I get a file permission issue due to an attempted chown to the default owner (root). Maybe this is why 'test_simple' uses patchUtils?

ERROR: tests.unittests.test_handler.test_handler_write_files.TestWriteFiles.test_append
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/james/projects/cloud-init/tests/unittests/test_handler/test_handler_write_files.py", line 63, in test_append
    [{"content": added, "path": filename, "append": "true"}])
  File "/home/james/projects/cloud-init/cloudinit/config/cc_write_files.py", line 122, in write_files
    util.chownbyname(path, u, g)
  File "/home/james/projects/cloud-init/cloudinit/util.py", line 1424, in chownbyname
    chownbyid(fname, uid, gid)
  File "/home/james/projects/cloud-init/cloudinit/util.py", line 1411, in chownbyid
    os.chown(fname, uid, gid)
PermissionError: [Errno 1] Operation not permitted: '/tmp/ci-TestWriteFiles.rozoygw8/append.file'
-------------------- >> begin captured logging << --------------------
cloudinit.util: DEBUG: Writing to /tmp/ci-TestWriteFiles.rozoygw8/append.file - wb: [644] 6 bytes
cloudinit.util: DEBUG: Writing to /tmp/ci-TestWriteFiles.rozoygw8/append.file - ab: [644] 6 bytes
cloudinit.util: DEBUG: Changing the ownership of /tmp/ci-TestWriteFiles.rozoygw8/append.file to 0:0
--------------------- >> end captured logging << ---------------------

[Ryan Harper (raharper)]

I'm sorry, I made you run in circles. The patchUtils() mocks the calls to cloudinit.util() which helps prevent it attempting to run operations like chmod/chown etc, it also "re-roots" /tmp so it's fine to put a /tmp/XXXX.

Sorry for the trouble.

[Ryan Harper (raharper)]

review: Approve

[Scott Moser (smoser)]

I think this is fine.
The only thing I wold say is that 'append: <boolean>' is less
future-safe than 'mode: append' (where 'mode' would default to
'truncate' to retain backwards compatible behavior).

The benefit of 'mode' (or whatever word we want to use there) is
that then ou could later extend that to support "write only if exists"
or something like that.

So i think the other would be more future safe but I think this is fine.

review: Approve

[Server Team CI bot (server-team-bot)]

review: Approve (continuous-integration)

diff --git a/cloudinit/config/cc_write_files.py b/cloudinit/config/cc_write_files.py
index 31d1db6..0b6546e 100644
--- a/cloudinit/config/cc_write_files.py
+++ b/cloudinit/config/cc_write_files.py
@@ -49,6 +49,10 @@ binary gzip data can be specified and will be decoded before being written.
             ...
           path: /bin/arch
           permissions: '0555'
+        - content: |
+            15 * * * * root ship_logs
+          path: /etc/crontab
+          append: true
 """
 
 import base64
@@ -113,7 +117,8 @@ def write_files(name, files):
         contents = extract_contents(f_info.get('content', ''), extractions)
         (u, g) = util.extract_usergroup(f_info.get('owner', DEFAULT_OWNER))
         perms = decode_perms(f_info.get('permissions'), DEFAULT_PERMS)
-        util.write_file(path, contents, mode=perms)
+        omode = 'ab' if util.get_cfg_option_bool(f_info, 'append') else 'wb'
+        util.write_file(path, contents, omode=omode, mode=perms)
         util.chownbyname(path, u, g)
 
 
diff --git a/tests/unittests/test_handler/test_handler_write_files.py b/tests/unittests/test_handler/test_handler_write_files.py
index 7fa8fd2..bc8756c 100644
--- a/tests/unittests/test_handler/test_handler_write_files.py
+++ b/tests/unittests/test_handler/test_handler_write_files.py
@@ -52,6 +52,18 @@ class TestWriteFiles(FilesystemMockingTestCase):
             "test_simple", [{"content": expected, "path": filename}])
         self.assertEqual(util.load_file(filename), expected)
 
+    def test_append(self):
+        self.patchUtils(self.tmp)
+        existing = "hello "
+        added = "world\n"
+        expected = existing + added
+        filename = "/tmp/append.file"
+        util.write_file(filename, existing)
+        write_files(
+            "test_append",
+            [{"content": added, "path": filename, "append": "true"}])
+        self.assertEqual(util.load_file(filename), expected)
+
     def test_yaml_binary(self):
         self.patchUtils(self.tmp)
         data = util.load_yaml(YAML_TEXT)