Merge lp:~free.ekanayaka/charm-helpers/relation-set-file into lp:charm-helpers

[Free Ekanayaka]

This branch re-introduces the code that was added in:

https://code.launchpad.net/~bjornt/charm-helpers/relation-set-file/+merge/259090

but backed out in r379 due to the bug linked to this MP.

The solution is to the describe by Bjorn in comment #5 of the bug linked to this branch (Bug #1459175). Essentially we do the same conversion to string as we do with the command-line-args version.

[Björn Tillenius (bjornt)]

Thanks for readding this feature in a working state! You have one bug in there. I fixed it in lp:~bjornt/charm-helpers/relation-set-file, so you can merge that one and things should be good to go.

review: Approve

[lp:~free.ekanayaka/charm-helpers/relation-set-file]

382. By Free Ekanayaka on 2015-05-29

Properly handle None

[Free Ekanayaka (free.ekanayaka)]

> Thanks for readding this feature in a working state! You have one bug in
> there. I fixed it in lp:~bjornt/charm-helpers/relation-set-file, so you can
> merge that one and things should be good to go.

Good catch. Merged and pushed.

[Chris Glass (tribaal)]

This looks good, and should address the linked bug.

Note: this branch incorporates the changes in https://code.launchpad.net/~stub/charm-helpers/bug-1458546-fix-relation-set-py3/+merge/260048

Merging.

review: Approve

=== modified file 'charmhelpers/core/hookenv.py'
--- charmhelpers/core/hookenv.py	2015-05-27 09:46:36 +0000
+++ charmhelpers/core/hookenv.py	2015-05-29 15:19:31 +0000
@@ -28,6 +28,7 @@
 import subprocess
 import sys
 import errno
+import tempfile
 from subprocess import CalledProcessError
 
 import six
@@ -362,14 +363,34 @@
     """Set relation information for the current unit"""
     relation_settings = relation_settings if relation_settings else {}
     relation_cmd_line = ['relation-set']
+    accepts_file = "--file" in subprocess.check_output(
+        relation_cmd_line + ["--help"], universal_newlines=True)
     if relation_id is not None:
         relation_cmd_line.extend(('-r', relation_id))
-    for k, v in (list(relation_settings.items()) + list(kwargs.items())):
-        if v is None:
-            relation_cmd_line.append('{}='.format(k))
-        else:
-            relation_cmd_line.append('{}={}'.format(k, v))
-    subprocess.check_call(relation_cmd_line)
+    settings = relation_settings.copy()
+    settings.update(kwargs)
+    for key, value in settings.items():
+        # Force value to be a string: it always should, but some call
+        # sites pass in things like dicts or numbers.
+        if value is not None:
+            settings[key] = "{}".format(value)
+    if accepts_file:
+        # --file was introduced in Juju 1.23.2. Use it by default if
+        # available, since otherwise we'll break if the relation data is
+        # too big. Ideally we should tell relation-set to read the data from
+        # stdin, but that feature is broken in 1.23.2: Bug #1454678.
+        with tempfile.NamedTemporaryFile(delete=False) as settings_file:
+            settings_file.write(yaml.safe_dump(settings).encode("utf-8"))
+        subprocess.check_call(
+            relation_cmd_line + ["--file", settings_file.name])
+        os.remove(settings_file.name)
+    else:
+        for key, value in settings.items():
+            if value is None:
+                relation_cmd_line.append('{}='.format(key))
+            else:
+                relation_cmd_line.append('{}={}'.format(key, value))
+        subprocess.check_call(relation_cmd_line)
     # Flush cache of any relation-gets for local unit
     flush(local_unit())
 

=== modified file 'tests/core/test_hookenv.py'
--- tests/core/test_hookenv.py	2015-05-27 09:46:36 +0000
+++ tests/core/test_hookenv.py	2015-05-29 15:19:31 +0000
@@ -915,6 +915,7 @@
         hookenv.relation_get(attribute='baz_scope', unit='baz_unit')
         hookenv.relation_get(attribute='bar_scope')
         self.assertTrue(len(hookenv.cache) == 2)
+        check_output.return_value = ""
         hookenv.relation_set(baz_scope='hello')
         # relation_set should flush any entries for local_unit
         self.assertTrue(len(hookenv.cache) == 1)
@@ -930,27 +931,86 @@
         check_output.assert_called_with(['relation-get', '--format=json', '-r',
                                          123, 'baz-scope', 'baz-unit'])
 
+    @patch('subprocess.check_output')
     @patch('subprocess.check_call')
-    def test_sets_relation_with_kwargs(self, check_call_):
+    def test_sets_relation_with_kwargs(self, check_call_, check_output):
         hookenv.relation_set(foo="bar")
         check_call_.assert_called_with(['relation-set', 'foo=bar'])
 
+    @patch('subprocess.check_output')
     @patch('subprocess.check_call')
-    def test_sets_relation_with_dict(self, check_call_):
+    def test_sets_relation_with_dict(self, check_call_, check_output):
         hookenv.relation_set(relation_settings={"foo": "bar"})
         check_call_.assert_called_with(['relation-set', 'foo=bar'])
 
+    @patch('subprocess.check_output')
     @patch('subprocess.check_call')
-    def test_sets_relation_with_relation_id(self, check_call_):
+    def test_sets_relation_with_relation_id(self, check_call_, check_output):
         hookenv.relation_set(relation_id="foo", bar="baz")
         check_call_.assert_called_with(['relation-set', '-r', 'foo',
                                         'bar=baz'])
 
+    @patch('subprocess.check_output')
     @patch('subprocess.check_call')
-    def test_sets_relation_with_missing_value(self, check_call_):
+    def test_sets_relation_with_missing_value(self, check_call_, check_output):
         hookenv.relation_set(foo=None)
         check_call_.assert_called_with(['relation-set', 'foo='])
 
+    @patch('os.remove')
+    @patch('subprocess.check_output')
+    @patch('subprocess.check_call')
+    def test_relation_set_file(self, check_call, check_output, remove):
+        """If relation-set accepts a --file parameter, it's used.
+
+        Juju 1.23.2 introduced a --file parameter, which means you can
+        pass the data through a file. Not using --file would make
+        relation_set break if the relation data is too big.
+        """
+        # check_output(["relation-set", "--help"]) is used to determine
+        # whether we can pass --file to it.
+        check_output.return_value = "--file"
+        hookenv.relation_set(foo="bar")
+        check_output.assert_called_with(
+            ["relation-set", "--help"], universal_newlines=True)
+        # relation-set is called with relation-set --file <temp_file>
+        # with data as YAML and the temp_file is then removed.
+        self.assertEqual(1, len(check_call.call_args[0]))
+        command = check_call.call_args[0][0]
+        self.assertEqual(3, len(command))
+        self.assertEqual("relation-set", command[0])
+        self.assertEqual("--file", command[1])
+        temp_file = command[2]
+        with open(temp_file, "r") as f:
+            self.assertEqual("{foo: bar}", f.read().strip())
+        remove.assert_called_with(temp_file)
+
+    @patch('os.remove')
+    @patch('subprocess.check_output')
+    @patch('subprocess.check_call')
+    def test_relation_set_file_non_str(self, check_call, check_output, remove):
+        """If relation-set accepts a --file parameter, it's used.
+
+        Any value that is not a string is converted to a string before encoding
+        the settings to YAML.
+        """
+        # check_output(["relation-set", "--help"]) is used to determine
+        # whether we can pass --file to it.
+        check_output.return_value = "--file"
+        hookenv.relation_set(foo={"bar": 1})
+        check_output.assert_called_with(
+            ["relation-set", "--help"], universal_newlines=True)
+        # relation-set is called with relation-set --file <temp_file>
+        # with data as YAML and the temp_file is then removed.
+        self.assertEqual(1, len(check_call.call_args[0]))
+        command = check_call.call_args[0][0]
+        self.assertEqual(3, len(command))
+        self.assertEqual("relation-set", command[0])
+        self.assertEqual("--file", command[1])
+        temp_file = command[2]
+        with open(temp_file, "r") as f:
+            self.assertEqual("{foo: '{''bar'': 1}'}", f.read().strip())
+        remove.assert_called_with(temp_file)
+
     def test_lists_relation_types(self):
         open_ = mock_open()
         open_.return_value = io.BytesIO(CHARM_METADATA)