Merge lp:~mew/charm-helpers/no-host-magic into lp:charm-helpers

[Matthew Wedgwood]

Remove magic value interpolation from host.py. Similar functionality exists
in contrib.templating.

[Stuart Bishop (stub)]

Is host.symlink() useful at all now? I guess os.symlink() doesn't force creation and no logging.

Looks good to me.

review: Approve

=== modified file 'charmhelpers/core/host.py'
--- charmhelpers/core/host.py	2013-07-09 10:43:41 +0000
+++ charmhelpers/core/host.py	2013-07-10 20:02:32 +0000
@@ -14,7 +14,7 @@
 
 from collections import OrderedDict
 
-from hookenv import log, execution_environment
+from hookenv import log
 
 
 def service_start(service_name):
@@ -74,36 +74,33 @@
 
 def rsync(from_path, to_path, flags='-r', options=None):
     """Replicate the contents of a path"""
-    context = execution_environment()
     options = options or ['--delete', '--executability']
     cmd = ['/usr/bin/rsync', flags]
     cmd.extend(options)
-    cmd.append(from_path.format(**context))
-    cmd.append(to_path.format(**context))
+    cmd.append(from_path)
+    cmd.append(to_path)
     log(" ".join(cmd))
     return subprocess.check_output(cmd).strip()
 
 
 def symlink(source, destination):
     """Create a symbolic link"""
-    context = execution_environment()
     log("Symlinking {} as {}".format(source, destination))
     cmd = [
         'ln',
         '-sf',
-        source.format(**context),
-        destination.format(**context)
+        source,
+        destination,
     ]
     subprocess.check_call(cmd)
 
 
 def mkdir(path, owner='root', group='root', perms=0555, force=False):
     """Create a directory"""
-    context = execution_environment()
     log("Making dir {} {}:{} {:o}".format(path, owner, group,
                                           perms))
-    uid = pwd.getpwnam(owner.format(**context)).pw_uid
-    gid = grp.getgrnam(group.format(**context)).gr_gid
+    uid = pwd.getpwnam(owner).pw_uid
+    gid = grp.getgrnam(group).gr_gid
     realpath = os.path.abspath(path)
     if os.path.exists(realpath):
         if force and not os.path.isdir(realpath):

=== modified file 'tests/core/test_host.py'
--- tests/core/test_host.py	2013-07-09 10:43:41 +0000
+++ tests/core/test_host.py	2013-07-10 20:02:32 +0000
@@ -252,15 +252,10 @@
         ])
 
     @patch('subprocess.check_output')
-    @patch.object(host, 'execution_environment')
     @patch.object(host, 'log')
-    def test_rsyncs_a_path(self, log, execution_environment, check_output):
-        execution_environment.return_value = {
-            'foo': 'FOO',
-            'bar': 'BAR',
-        }
-        from_path = '/from/this/path/{foo}'
-        to_path = '/to/this/path/{bar}'
+    def test_rsyncs_a_path(self, log, check_output):
+        from_path = '/from/this/path/foo'
+        to_path = '/to/this/path/bar'
         check_output.return_value = ' some output '
 
         result = host.rsync(from_path, to_path)
@@ -268,42 +263,31 @@
         self.assertEqual(result, 'some output')
         check_output.assert_called_with(['/usr/bin/rsync', '-r', '--delete',
                                          '--executability',
-                                         '/from/this/path/FOO',
-                                         '/to/this/path/BAR'])
+                                         '/from/this/path/foo',
+                                         '/to/this/path/bar'])
 
     @patch('subprocess.check_call')
-    @patch.object(host, 'execution_environment')
     @patch.object(host, 'log')
-    def test_creates_a_symlink(self, log, execution_environment, check_call):
-        execution_environment.return_value = {
-            'foo': 'FOO',
-            'bar': 'BAR',
-        }
-        source = '/from/this/path/{foo}'
-        destination = '/to/this/path/{bar}'
+    def test_creates_a_symlink(self, log, check_call):
+        source = '/from/this/path/foo'
+        destination = '/to/this/path/bar'
 
         host.symlink(source, destination)
 
         check_call.assert_called_with(['ln', '-sf',
-                                       '/from/this/path/FOO',
-                                       '/to/this/path/BAR'])
+                                       '/from/this/path/foo',
+                                       '/to/this/path/bar'])
 
     @patch('pwd.getpwnam')
     @patch('grp.getgrnam')
-    @patch.object(host, 'execution_environment')
     @patch.object(host, 'log')
     @patch.object(host, 'os')
     def test_creates_a_directory_if_it_doesnt_exist(self, os_, log,
-                                                    execution_environment,
                                                     getgrnam, getpwnam):
-        execution_environment.return_value = {
-            'foo': 'FOO',
-            'bar': 'BAR',
-        }
         uid = 123
         gid = 234
-        owner = 'some-user-{foo}'
-        group = 'some-group-{bar}'
+        owner = 'some-user'
+        group = 'some-group'
         path = '/some/other/path/from/link'
         realpath = '/some/path'
         path_exists = False
@@ -316,22 +300,16 @@
 
         host.mkdir(path, owner=owner, group=group, perms=perms)
 
-        getpwnam.assert_called_with('some-user-FOO')
-        getgrnam.assert_called_with('some-group-BAR')
+        getpwnam.assert_called_with('some-user')
+        getgrnam.assert_called_with('some-group')
         os_.path.abspath.assert_called_with(path)
         os_.path.exists.assert_called_with(realpath)
         os_.makedirs.assert_called_with(realpath, perms)
         os_.chown.assert_called_with(realpath, uid, gid)
 
-    @patch.object(host, 'execution_environment')
     @patch.object(host, 'log')
     @patch.object(host, 'os')
-    def test_creates_a_directory_with_defaults(self, os_, log,
-                                               execution_environment):
-        execution_environment.return_value = {
-            'foo': 'FOO',
-            'bar': 'BAR',
-        }
+    def test_creates_a_directory_with_defaults(self, os_, log):
         uid = 0
         gid = 0
         path = '/some/other/path/from/link'
@@ -351,20 +329,14 @@
 
     @patch('pwd.getpwnam')
     @patch('grp.getgrnam')
-    @patch.object(host, 'execution_environment')
     @patch.object(host, 'log')
     @patch.object(host, 'os')
     def test_removes_file_with_same_path_before_mkdir(self, os_, log,
-                                                      execution_environment,
                                                       getgrnam, getpwnam):
-        execution_environment.return_value = {
-            'foo': 'FOO',
-            'bar': 'BAR',
-        }
         uid = 123
         gid = 234
-        owner = 'some-user-{foo}'
-        group = 'some-group-{bar}'
+        owner = 'some-user'
+        group = 'some-group'
         path = '/some/other/path/from/link'
         realpath = '/some/path'
         path_exists = True
@@ -380,8 +352,8 @@
 
         host.mkdir(path, owner=owner, group=group, perms=perms, force=force)
 
-        getpwnam.assert_called_with('some-user-FOO')
-        getgrnam.assert_called_with('some-group-BAR')
+        getpwnam.assert_called_with('some-user')
+        getgrnam.assert_called_with('some-group')
         os_.path.abspath.assert_called_with(path)
         os_.path.exists.assert_called_with(realpath)
         os_.unlink.assert_called_with(realpath)