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

Proposed by Stuart Bishop
Status: Merged
Merged at revision: 47
Proposed branch: lp:~stub/charm-helpers/bug-1195649-fix-write-file
Merge into: lp:charm-helpers
Diff against target: 203 lines (+68/-67)
4 files modified
charmhelpers/contrib/templating/pyformat.py (+13/-0)
charmhelpers/core/host.py (+6/-19)
tests/contrib/templating/test_pyformat.py (+36/-0)
tests/core/test_host.py (+13/-48)
To merge this branch: bzr merge lp:~stub/charm-helpers/bug-1195649-fix-write-file
Reviewer Review Type Date Requested Status
Matthew Wedgwood (community) Approve
Review via email: mp+173467@code.launchpad.net

Description of the change

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

To post a comment you must log in.
Revision history for this message
Matthew Wedgwood (mew) wrote :

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
36. By Stuart Bishop

Move python templating to contrib per review feedback

37. By Stuart Bishop

Add missing test

Revision history for this message
Stuart Bishop (stub) wrote :

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.

Revision history for this message
Matthew Wedgwood (mew) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== added directory 'charmhelpers/contrib/templating'
2=== added file 'charmhelpers/contrib/templating/__init__.py'
3=== added file 'charmhelpers/contrib/templating/pyformat.py'
4--- charmhelpers/contrib/templating/pyformat.py 1970-01-01 00:00:00 +0000
5+++ charmhelpers/contrib/templating/pyformat.py 2013-07-09 10:50:35 +0000
6@@ -0,0 +1,13 @@
7+'''
8+Templating using standard Python str.format() method.
9+'''
10+
11+from charmhelpers.core import hookenv
12+
13+
14+def render(template, extra={}, **kwargs):
15+ """Return the template rendered using Python's str.format()."""
16+ context = hookenv.execution_environment()
17+ context.update(extra)
18+ context.update(kwargs)
19+ return template.format(**context)
20
21=== modified file 'charmhelpers/core/host.py'
22--- charmhelpers/core/host.py 2013-07-03 11:42:37 +0000
23+++ charmhelpers/core/host.py 2013-07-09 10:50:35 +0000
24@@ -114,28 +114,15 @@
25 os.chown(realpath, uid, gid)
26
27
28-def write_file(path, fmtstr, owner='root', group='root', perms=0444, **kwargs):
29+def write_file(path, content, owner='root', group='root', perms=0444):
30 """Create or overwrite a file with the contents of a string"""
31- context = execution_environment()
32- context.update(kwargs)
33- log("Writing file {} {}:{} {:o}".format(path, owner, group,
34- perms))
35- uid = pwd.getpwnam(owner.format(**context)).pw_uid
36- gid = grp.getgrnam(group.format(**context)).gr_gid
37- with open(path.format(**context), 'w') as target:
38+ log("Writing file {} {}:{} {:o}".format(path, owner, group, perms))
39+ uid = pwd.getpwnam(owner).pw_uid
40+ gid = grp.getgrnam(group).gr_gid
41+ with open(path, 'w') as target:
42 os.fchown(target.fileno(), uid, gid)
43 os.fchmod(target.fileno(), perms)
44- target.write(fmtstr.format(**context))
45-
46-
47-def render_template_file(source, destination, **kwargs):
48- """Create or overwrite a file using a template"""
49- log("Rendering template {} for {}".format(source,
50- destination))
51- context = execution_environment()
52- with open(source.format(**context), 'r') as template:
53- write_file(destination.format(**context), template.read(),
54- **kwargs)
55+ target.write(content)
56
57
58 def filter_installed_packages(packages):
59
60=== added directory 'tests/contrib/templating'
61=== added file 'tests/contrib/templating/__init__.py'
62=== added file 'tests/contrib/templating/test_pyformat.py'
63--- tests/contrib/templating/test_pyformat.py 1970-01-01 00:00:00 +0000
64+++ tests/contrib/templating/test_pyformat.py 2013-07-09 10:50:35 +0000
65@@ -0,0 +1,36 @@
66+from mock import patch
67+from testtools import TestCase
68+
69+from charmhelpers.contrib.templating.pyformat import render
70+from charmhelpers.core import hookenv
71+
72+class PyFormatTest(TestCase):
73+ @patch.object(hookenv, 'execution_environment')
74+ def test_renders_using_environment(self, execution_environment):
75+ execution_environment.return_value = {
76+ 'foo': 'FOO',
77+ }
78+
79+ self.assertEqual(render('foo is {foo}'), 'foo is FOO')
80+
81+ @patch.object(hookenv, 'execution_environment')
82+ def test_extra_overrides(self, execution_environment):
83+ execution_environment.return_value = {
84+ 'foo': 'FOO',
85+ }
86+
87+ extra = {'foo': 'BAR'}
88+
89+ self.assertEqual(render('foo is {foo}', extra=extra), 'foo is BAR')
90+
91+ @patch.object(hookenv, 'execution_environment')
92+ def test_kwargs_overrides(self, execution_environment):
93+ execution_environment.return_value = {
94+ 'foo': 'FOO',
95+ }
96+
97+ extra = {'foo': 'BAR'}
98+
99+ self.assertEqual(
100+ render('foo is {foo}', extra=extra, foo='BAZ'), 'foo is BAZ')
101+
102
103=== modified file 'tests/core/test_host.py'
104--- tests/core/test_host.py 2013-07-03 11:42:37 +0000
105+++ tests/core/test_host.py 2013-07-09 10:50:35 +0000
106@@ -390,23 +390,18 @@
107
108 @patch('pwd.getpwnam')
109 @patch('grp.getgrnam')
110- @patch.object(host, 'execution_environment')
111 @patch.object(host, 'log')
112 @patch.object(host, 'os')
113- def test_writes_content_to_a_file(self, os_, log, execution_environment,
114- getgrnam, getpwnam):
115- execution_environment.return_value = {
116- 'foo': 'FOO',
117- 'bar': 'BAR',
118- 'baz': 'BAZ',
119- 'juju': 'DevOps Distilled',
120- }
121+ def test_writes_content_to_a_file(self, os_, log, getgrnam, getpwnam):
122+ # Curly brackets here demonstrate that we are *not* rendering
123+ # these strings with Python's string formatting. This is a
124+ # change from the original behavior per Bug #1195634.
125 uid = 123
126 gid = 234
127 owner = 'some-user-{foo}'
128 group = 'some-group-{bar}'
129 path = '/some/path/{baz}'
130- fmtstr = 'what is {juju}'
131+ contents = 'what is {juju}'
132 perms = 0644
133 fileno = 'some-fileno'
134
135@@ -416,27 +411,19 @@
136 with patch_open() as (mock_open, mock_file):
137 mock_file.fileno.return_value = fileno
138
139- host.write_file(path, fmtstr, owner=owner, group=group,
140+ host.write_file(path, contents, owner=owner, group=group,
141 perms=perms)
142
143- getpwnam.assert_called_with('some-user-FOO')
144- getgrnam.assert_called_with('some-group-BAR')
145- mock_open.assert_called_with('/some/path/BAZ', 'w')
146+ getpwnam.assert_called_with('some-user-{foo}')
147+ getgrnam.assert_called_with('some-group-{bar}')
148+ mock_open.assert_called_with('/some/path/{baz}', 'w')
149 os_.fchown.assert_called_with(fileno, uid, gid)
150 os_.fchmod.assert_called_with(fileno, perms)
151- mock_file.write.assert_called_with('what is DevOps Distilled')
152+ mock_file.write.assert_called_with('what is {juju}')
153
154- @patch.object(host, 'execution_environment')
155 @patch.object(host, 'log')
156 @patch.object(host, 'os')
157- def test_writes_content_with_default(self, os_, log,
158- execution_environment):
159- execution_environment.return_value = {
160- 'foo': 'FOO',
161- 'bar': 'BAR',
162- 'baz': 'BAZ',
163- 'juju': 'DevOps Distilled',
164- }
165+ def test_writes_content_with_default(self, os_, log):
166 uid = 0
167 gid = 0
168 path = '/some/path/{baz}'
169@@ -449,32 +436,10 @@
170
171 host.write_file(path, fmtstr)
172
173- mock_open.assert_called_with('/some/path/BAZ', 'w')
174+ mock_open.assert_called_with('/some/path/{baz}', 'w')
175 os_.fchown.assert_called_with(fileno, uid, gid)
176 os_.fchmod.assert_called_with(fileno, perms)
177- mock_file.write.assert_called_with('what is DevOps Distilled')
178-
179- @patch.object(host, 'execution_environment')
180- @patch.object(host, 'log')
181- @patch.object(host, 'os')
182- @patch.object(host, 'write_file')
183- def test_renders_a_template_file(self, write_file, os_, log,
184- execution_environment):
185- execution_environment.return_value = {
186- 'foo': 'FOO',
187- 'bar': 'BAR',
188- }
189- source = '/some/path/{foo}'
190- destination = '/some/path/{bar}'
191- content = 'some-content'
192-
193- with patch_open() as (mock_open, mock_file):
194- mock_file.read.return_value = content
195-
196- host.render_template_file(source, destination, foo2='2')
197-
198- mock_open.assert_called_with('/some/path/FOO', 'r')
199- write_file.assert_called_with('/some/path/BAR', content, foo2='2')
200+ mock_file.write.assert_called_with('what is {juju}')
201
202 @patch('subprocess.call')
203 @patch.object(host, 'log')

Subscribers

People subscribed via source and target branches