Merge lp:~niedbalski/charm-helpers/fix-1699565 into lp:charm-helpers

[Jorge Niedbalski]

Partial fix required for LP: #1699565.

[Alex Kavanagh (ajkavanagh)]

Does this really need to be in charm-helpers? It seems to be something that could just be done in the glance charm. The function 'update_policy(...)' would have to also be mocked out in the glance charm to test the code that it will be using.

Are you seeing other use cases that means that it will be used across many charms? Thanks.

[Felipe Reyes (freyes)]

I agree with having this in charm-helpers, because I'm aware of at least 1 other charm that could benefit from this (keystone charm). We update the policy.json when v3 is enabled.

review: Approve

[Alex Kavanagh (ajkavanagh)]

Okay, if it is needed, then it should have a more generic name. It's essentially updating a json file, not particularly a policy file, so:

    def update_json_file(filename, items):
        ....

would make it more usable from a re-use perspective. i.e. there's nothing in the function that dictates that it's a policy file only. And this is a utils library module.

[Jorge Niedbalski (niedbalski)]

Addressed ajkavanagh comments in the latest commit, please review.

[Felipe Reyes (freyes)]

review: Approve

[Alex Kavanagh (ajkavanagh)]

review: Approve

=== modified file 'charmhelpers/contrib/openstack/utils.py'
--- charmhelpers/contrib/openstack/utils.py	2017-05-25 13:46:14 +0000
+++ charmhelpers/contrib/openstack/utils.py	2017-06-22 14:59:18 +0000
@@ -1982,3 +1982,15 @@
     if enable_memcache(source=source, release=release):
         packages.extend(['memcached', 'python-memcache'])
     return packages
+
+
+def update_json_file(filename, items):
+    """Updates the json `filename` with a given dict.
+    :param filename: json filename (i.e.: /etc/glance/policy.json)
+    :param items: dict of items to update
+    """
+    with open(filename) as fd:
+        policy = json.load(fd)
+    policy.update(items)
+    with open(filename, "w") as fd:
+        fd.write(json.dumps(policy, indent=4))

=== modified file 'tests/contrib/openstack/test_os_utils.py'
--- tests/contrib/openstack/test_os_utils.py	2017-04-10 18:10:37 +0000
+++ tests/contrib/openstack/test_os_utils.py	2017-06-22 14:59:18 +0000
@@ -1,9 +1,15 @@
 import json
 import mock
 import unittest
+import six
 
 from charmhelpers.contrib.openstack import utils
 
+if not six.PY3:
+    builtin_open = '__builtin__.open'
+else:
+    builtin_open = 'builtins.open'
+
 
 class UtilsTests(unittest.TestCase):
     def setUp(self):
@@ -188,3 +194,31 @@
         _enable_memcache.return_value = True
         self.assertEqual(utils.token_cache_pkgs(source='distro'),
                          ['memcached', 'python-memcache'])
+
+    def test_update_json_file(self):
+        TEST_POLICY = """{
+        "delete_image_location": "",
+        "get_image_location": "",
+        "set_image_location": "",
+        "extra_property": "False"
+        }"""
+
+        TEST_POLICY_FILE = "/etc/glance/policy.json"
+
+        item_to_update = {
+            "get_image_location": "role:admin",
+            "extra_policy": "extra",
+        }
+
+        mock_open = mock.mock_open(read_data=TEST_POLICY)
+        with mock.patch(builtin_open, mock_open) as mock_file:
+            utils.update_json_file(TEST_POLICY_FILE, item_to_update)
+            mock_file.assert_has_calls([
+                mock.call(TEST_POLICY_FILE),
+                mock.call(TEST_POLICY_FILE, 'w'),
+            ], any_order=True)
+
+        modified_policy = json.loads(TEST_POLICY)
+        modified_policy.update(item_to_update)
+        mock_open().write.assert_called_with(
+            json.dumps(modified_policy, indent=4))