Merge lp:~raharper/curtin/trunk-lp1731709-curthooks-write-files into lp:~curtin-dev/curtin/trunk

[Ryan Harper]

Re-add curthooks.write_files method for backwards compat

In revno 512, curthooks.write_files() was re-implemented under
curtin.futil.write_files and supports the original dictionary
format as well as other modes. This broke legacy images which used
hooks that invoked curthooks.write_files. This branch restores
the function and behavior while utilizing the change in implementation.

- Add warning when legacy path is called pointing to new module location
- Add unittests to exercise the code path.

LP: #1731709

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

PASSED: Continuous integration, rev:543
https://jenkins.ubuntu.com/server/job/curtin-ci/674/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/674
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/674
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/674
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/674

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/674/rebuild

review: Approve (continuous-integration)

[Scott Moser (smoser)]

To my knowledge this did not occur in any officially supported image.
I believe it was a "custom image".

While we do not want to willy-nilly break things, we also have been
very un-careful about curtin usage as a library. We need to clean that
up, basically setting __all__ and only exposing methods we promise to keep.

If we do want to make this backwards compatible, I'd prefer to add a
warning also.

[Scott Moser (smoser)]

I'm not entirely opposed to fixing this.

However, I suggest we should put the actual definition of the backwards compatibility either:
a.) inside futil.write_files entirely and then in util:
    from futil import write_files

b.) inside futil._legacy_write_files and then in util, do
    from futil import _legacy_write_files as write_files

Then also, I'd prefer that we just unit test this.
We dont' need an integration test for it.

Lastly any way its called, we should warn with a deprecated warning.
http://www.arruda.blog.br/programacao/python-use-warnings/

[Ryan Harper (raharper)]

On Fri, Nov 17, 2017 at 1:25 PM, Scott Moser <email address hidden>
wrote:

> I'm not entirely opposed to fixing this.
>
> However, I suggest we should put the actual definition of the backwards
> compatibility either:
> a.) inside futil.write_files entirely and then in util:
> from futil import write_files
>
> b.) inside futil._legacy_write_files and then in util, do
> from futil import _legacy_write_files as write_files
>

This was a little confusing; what I think is reasonable
is to move curthooks.write_files (which is the legacy impl)
and move that to futil._legacy_write_files (and have it call write_files
directly)

And in curthooks, change the import as you show in (b).

in certain places where curthooks uses futil.write_files; those stay
but curthooks will have a 'write_files' attr which will point to
futil._legacy_write_files.

Do you still want to LOG a message/warning that users should instead use
futil.write_files instead of curthooks ?

>
> Then also, I'd prefer that we just unit test this.
>

We do.

> We dont' need an integration test for it.
>

I think it's a reasonable check.

>
> Lastly any way its called, we should warn with a deprecated warning.
> http://www.arruda.blog.br/programacao/python-use-warnings/
> --
> https://code.launchpad.net/~raharper/curtin/trunk-
> lp1731709-curthooks-write-files/+merge/333714
> You are the owner of lp:~raharper/curtin/trunk-lp1731709-curthooks-write-
> files.
>

[Scott Moser (smoser)]

https://code.launchpad.net/~smoser/curtin/trunk-lp1731709-curthooks-write-files/+merge/333920

[Scott Moser (smoser)]

Ryan and I talked about my MP linked above, and I think he was generally ok witih the suggested changes.

I think this is ready to go, but will wait for an "OK" from Ryan, or him pulling my branch into his.

If that is done, then I approve.
Just update the commit message appropriately (currently mentions vmtest tests)

[lp:~raharper/curtin/trunk-lp1731709-curthooks-write-files]

544. By Ryan Harper on 2017-11-17

Move curthooks.write_files into futil._legacy_write_files

545. By Ryan Harper on 2017-11-27

merge from smoser write-files

- fixed up style check
- modified docstring in ftutil._legacy_write_files

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

PASSED: Continuous integration, rev:545
https://jenkins.ubuntu.com/server/job/curtin-ci/679/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/679
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/679
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/679
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/679

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/679/rebuild

review: Approve (continuous-integration)

[Scott Moser (smoser)]

review: Approve

=== modified file 'curtin/commands/curthooks.py'
--- curtin/commands/curthooks.py	2017-10-05 18:59:05 +0000
+++ curtin/commands/curthooks.py	2017-11-27 18:50:41 +0000
@@ -36,6 +36,8 @@
 
 from . import populate_one_subcmd
 
+write_files = futil._legacy_write_files  # LP: #1731709
+
 CMD_ARGUMENTS = (
     ((('-t', '--target'),
       {'help': 'operate on target. default is env[TARGET_MOUNT_POINT]',

=== modified file 'curtin/futil.py'
--- curtin/futil.py	2017-07-28 19:01:38 +0000
+++ curtin/futil.py	2017-11-27 18:50:41 +0000
@@ -18,6 +18,7 @@
 import grp
 import pwd
 import os
+import warnings
 
 from .util import write_file, target_path
 from .log import LOG
@@ -101,3 +102,15 @@
                     content=info.get('content', ''),
                     owner=info.get('owner', "-1:-1"),
                     perms=info.get('permissions', info.get('perms', "0644")))
+
+
+def _legacy_write_files(cfg, base_dir=None):
+    """Backwards compatibility for curthooks.write_files (LP: #1731709)
+    It needs to work like:
+        curthooks.write_files(cfg, target)
+    cfg is a 'cfg' dictionary with a 'write_files' entry in it.
+    """
+    warnings.warn(
+        "write_files use from curtin.util is deprecated. "
+        "Please use curtin.futil.write_files.", DeprecationWarning)
+    return write_files(cfg.get('write_files', {}), base_dir=base_dir)

=== modified file 'tests/unittests/helpers.py'
--- tests/unittests/helpers.py	2017-08-03 19:31:49 +0000
+++ tests/unittests/helpers.py	2017-11-27 18:50:41 +0000
@@ -79,3 +79,16 @@
         if _dir is None:
             _dir = self.tmp_dir()
         return os.path.normpath(os.path.abspath(os.path.join(_dir, path)))
+
+
+def dir2dict(startdir, prefix=None):
+    flist = {}
+    if prefix is None:
+        prefix = startdir
+    for root, dirs, files in os.walk(startdir):
+        for fname in files:
+            fpath = os.path.join(root, fname)
+            key = fpath[len(prefix):]
+            with open(fpath, "r") as fp:
+                flist[key] = fp.read()
+    return flist

=== modified file 'tests/unittests/test_curthooks.py'
--- tests/unittests/test_curthooks.py	2017-08-03 18:14:51 +0000
+++ tests/unittests/test_curthooks.py	2017-11-27 18:50:41 +0000
@@ -5,7 +5,7 @@
 from curtin import util
 from curtin import config
 from curtin.reporter import events
-from .helpers import CiTestCase
+from .helpers import CiTestCase, dir2dict
 
 
 class TestGetFlashKernelPkgs(CiTestCase):
@@ -761,4 +761,50 @@
             curthooks.detect_required_packages({'network': {'version': 3}})
 
 
+class TestCurthooksWriteFiles(CiTestCase):
+    def test_handle_write_files_empty(self):
+        """ Test curthooks.write_files returns for empty config """
+        tmpd = self.tmp_dir()
+        ret = curthooks.write_files({}, tmpd)
+        self.assertEqual({}, dir2dict(tmpd, prefix=tmpd))
+        self.assertIsNone(ret)
+
+    def test_handle_write_files(self):
+        """ Test curthooks.write_files works as it used to """
+        tmpd = self.tmp_dir()
+        cfg = {'file1': {'path': '/etc/default/hello.txt',
+                         'content': "Hello World!\n"},
+               'foobar': {'path': '/sys/wark', 'content': "Engauge!\n"}}
+        curthooks.write_files({'write_files': cfg}, tmpd)
+        self.assertEqual(
+            dict((cfg[i]['path'], cfg[i]['content']) for i in cfg.keys()),
+            dir2dict(tmpd, prefix=tmpd))
+
+    @patch('curtin.commands.curthooks.futil.target_path')
+    @patch('curtin.commands.curthooks.futil.write_finfo')
+    def test_handle_write_files_finfo(self, mock_write_finfo, mock_tp):
+        """ Validate that futils.write_files handles target_path correctly """
+        cc_target = "/tmpXXXX/random/dir/used/by/maas"
+        cfg = {
+            'file1': {
+                'path': '/etc/default/hello.txt',
+                'content': "Hello World!\n",
+            },
+        }
+        mock_tp.side_effect = [
+            cc_target + cfg['file1']['path'],
+        ]
+
+        expected_cfg = {
+            'file1': {
+                'path': '/etc/default/hello.txt',
+                'content': cfg['file1']['content']},
+        }
+        curthooks.write_files({'write_files': cfg}, cc_target)
+        mock_write_finfo.assert_called_with(
+            content=expected_cfg['file1']['content'], owner='-1:-1',
+            path=cc_target + expected_cfg['file1']['path'],
+            perms='0644')
+
+
 # vi: ts=4 expandtab syntax=python