Merge lp:~stub/charm-helpers/bug-1195649-fix-write-file into lp:charm-helpers

[Stuart Bishop]

Move implicit rendering out of write_file and into a helper, per Bug #1195634.

[Matthew Wedgwood (mew)]

Stuart,

Thanks for cleaning this up. I agree that template rendering should be a separate concern from writing a file. I'm +1 on this change with one caveat:

As py_render() isn't host-related (nor is the existing render_template_file() function), it should live somewhere outside this module. charmhelpers.contrib.template might be appropriate, as most any other template renderers will have external dependencies.

-Matthew

review: Needs Fixing

[lp:~stub/charm-helpers/bug-1195649-fix-write-file]

36. By Stuart Bishop on 2013-07-09

Move python templating to contrib per review feedback

37. By Stuart Bishop on 2013-07-09

Add missing test

[Stuart Bishop (stub)]

I've moved the helper to contrib.

I removed render_template_file() entirely for now. Perhaps write_file should do implicit rendering of usernames, groupnames & paths? It is magic, but at least with python formatting using curly brackets it is safe(ish) magic in these cases.

[Matthew Wedgwood (mew)]

I don't see a lot of value in having write_file do anything magic so long as there's an easy way to do it elsewhere. host.py, as I see it, should simply make it easy to interact with the host and shouldn't care much about the fact that it's running in a juju environment.

That means we should probably clean up rsync(), symlink(), and mkdir() as well. I'll file bugs for those.

This looks good!

review: Approve

=== added directory 'charmhelpers/contrib/templating'
=== added file 'charmhelpers/contrib/templating/__init__.py'
=== added file 'charmhelpers/contrib/templating/pyformat.py'
--- charmhelpers/contrib/templating/pyformat.py	1970-01-01 00:00:00 +0000
+++ charmhelpers/contrib/templating/pyformat.py	2013-07-09 10:50:35 +0000
@@ -0,0 +1,13 @@
+'''
+Templating using standard Python str.format() method.
+'''
+
+from charmhelpers.core import hookenv
+
+
+def render(template, extra={}, **kwargs):
+    """Return the template rendered using Python's str.format()."""
+    context = hookenv.execution_environment()
+    context.update(extra)
+    context.update(kwargs)
+    return template.format(**context)

=== modified file 'charmhelpers/core/host.py'
--- charmhelpers/core/host.py	2013-07-03 11:42:37 +0000
+++ charmhelpers/core/host.py	2013-07-09 10:50:35 +0000
@@ -114,28 +114,15 @@
     os.chown(realpath, uid, gid)
 
 
-def write_file(path, fmtstr, owner='root', group='root', perms=0444, **kwargs):
+def write_file(path, content, owner='root', group='root', perms=0444):
     """Create or overwrite a file with the contents of a string"""
-    context = execution_environment()
-    context.update(kwargs)
-    log("Writing file {} {}:{} {:o}".format(path, owner, group,
-        perms))
-    uid = pwd.getpwnam(owner.format(**context)).pw_uid
-    gid = grp.getgrnam(group.format(**context)).gr_gid
-    with open(path.format(**context), 'w') as target:
+    log("Writing file {} {}:{} {:o}".format(path, owner, group, perms))
+    uid = pwd.getpwnam(owner).pw_uid
+    gid = grp.getgrnam(group).gr_gid
+    with open(path, 'w') as target:
         os.fchown(target.fileno(), uid, gid)
         os.fchmod(target.fileno(), perms)
-        target.write(fmtstr.format(**context))
-
-
-def render_template_file(source, destination, **kwargs):
-    """Create or overwrite a file using a template"""
-    log("Rendering template {} for {}".format(source,
-        destination))
-    context = execution_environment()
-    with open(source.format(**context), 'r') as template:
-        write_file(destination.format(**context), template.read(),
-                   **kwargs)
+        target.write(content)
 
 
 def filter_installed_packages(packages):

=== added directory 'tests/contrib/templating'
=== added file 'tests/contrib/templating/__init__.py'
=== added file 'tests/contrib/templating/test_pyformat.py'
--- tests/contrib/templating/test_pyformat.py	1970-01-01 00:00:00 +0000
+++ tests/contrib/templating/test_pyformat.py	2013-07-09 10:50:35 +0000
@@ -0,0 +1,36 @@
+from mock import patch
+from testtools import TestCase
+
+from charmhelpers.contrib.templating.pyformat import render
+from charmhelpers.core import hookenv
+
+class PyFormatTest(TestCase):
+    @patch.object(hookenv, 'execution_environment')
+    def test_renders_using_environment(self, execution_environment):
+        execution_environment.return_value = {
+            'foo': 'FOO',
+        }
+
+        self.assertEqual(render('foo is {foo}'), 'foo is FOO')
+
+    @patch.object(hookenv, 'execution_environment')
+    def test_extra_overrides(self, execution_environment):
+        execution_environment.return_value = {
+            'foo': 'FOO',
+        }
+
+        extra = {'foo': 'BAR'}
+
+        self.assertEqual(render('foo is {foo}', extra=extra), 'foo is BAR')
+
+    @patch.object(hookenv, 'execution_environment')
+    def test_kwargs_overrides(self, execution_environment):
+        execution_environment.return_value = {
+            'foo': 'FOO',
+        }
+
+        extra = {'foo': 'BAR'}
+
+        self.assertEqual(
+            render('foo is {foo}', extra=extra, foo='BAZ'), 'foo is BAZ')
+

=== modified file 'tests/core/test_host.py'
--- tests/core/test_host.py	2013-07-03 11:42:37 +0000
+++ tests/core/test_host.py	2013-07-09 10:50:35 +0000
@@ -390,23 +390,18 @@
 
     @patch('pwd.getpwnam')
     @patch('grp.getgrnam')
-    @patch.object(host, 'execution_environment')
     @patch.object(host, 'log')
     @patch.object(host, 'os')
-    def test_writes_content_to_a_file(self, os_, log, execution_environment,
-                                      getgrnam, getpwnam):
-        execution_environment.return_value = {
-            'foo': 'FOO',
-            'bar': 'BAR',
-            'baz': 'BAZ',
-            'juju': 'DevOps Distilled',
-        }
+    def test_writes_content_to_a_file(self, os_, log, getgrnam, getpwnam):
+        # Curly brackets here demonstrate that we are *not* rendering
+        # these strings with Python's string formatting. This is a
+        # change from the original behavior per Bug #1195634.
         uid = 123
         gid = 234
         owner = 'some-user-{foo}'
         group = 'some-group-{bar}'
         path = '/some/path/{baz}'
-        fmtstr = 'what is {juju}'
+        contents = 'what is {juju}'
         perms = 0644
         fileno = 'some-fileno'
 
@@ -416,27 +411,19 @@
         with patch_open() as (mock_open, mock_file):
             mock_file.fileno.return_value = fileno
 
-            host.write_file(path, fmtstr, owner=owner, group=group,
+            host.write_file(path, contents, owner=owner, group=group,
                             perms=perms)
 
-            getpwnam.assert_called_with('some-user-FOO')
-            getgrnam.assert_called_with('some-group-BAR')
-            mock_open.assert_called_with('/some/path/BAZ', 'w')
+            getpwnam.assert_called_with('some-user-{foo}')
+            getgrnam.assert_called_with('some-group-{bar}')
+            mock_open.assert_called_with('/some/path/{baz}', 'w')
             os_.fchown.assert_called_with(fileno, uid, gid)
             os_.fchmod.assert_called_with(fileno, perms)
-            mock_file.write.assert_called_with('what is DevOps Distilled')
+            mock_file.write.assert_called_with('what is {juju}')
 
-    @patch.object(host, 'execution_environment')
     @patch.object(host, 'log')
     @patch.object(host, 'os')
-    def test_writes_content_with_default(self, os_, log,
-                                         execution_environment):
-        execution_environment.return_value = {
-            'foo': 'FOO',
-            'bar': 'BAR',
-            'baz': 'BAZ',
-            'juju': 'DevOps Distilled',
-        }
+    def test_writes_content_with_default(self, os_, log):
         uid = 0
         gid = 0
         path = '/some/path/{baz}'
@@ -449,32 +436,10 @@
 
             host.write_file(path, fmtstr)
 
-            mock_open.assert_called_with('/some/path/BAZ', 'w')
+            mock_open.assert_called_with('/some/path/{baz}', 'w')
             os_.fchown.assert_called_with(fileno, uid, gid)
             os_.fchmod.assert_called_with(fileno, perms)
-            mock_file.write.assert_called_with('what is DevOps Distilled')
-
-    @patch.object(host, 'execution_environment')
-    @patch.object(host, 'log')
-    @patch.object(host, 'os')
-    @patch.object(host, 'write_file')
-    def test_renders_a_template_file(self, write_file, os_, log,
-                                     execution_environment):
-        execution_environment.return_value = {
-            'foo': 'FOO',
-            'bar': 'BAR',
-        }
-        source = '/some/path/{foo}'
-        destination = '/some/path/{bar}'
-        content = 'some-content'
-
-        with patch_open() as (mock_open, mock_file):
-            mock_file.read.return_value = content
-
-            host.render_template_file(source, destination, foo2='2')
-
-            mock_open.assert_called_with('/some/path/FOO', 'r')
-            write_file.assert_called_with('/some/path/BAR', content, foo2='2')
+            mock_file.write.assert_called_with('what is {juju}')
 
     @patch('subprocess.call')
     @patch.object(host, 'log')