Repository: canonical/operator
Pull request #369 (2020-08-10 14:42:06)

use yaml instead of json for pod-spec-set call
fixes: #293

chipaca: > We should test that this works on a K8s charm

@darkalia was hoping to get back to me with that sometime tomorrow (if I got that right :slightly_smiling_face: )
jameinel: On the plus side, reactive already is using yaml for the interaction:
https://github.com/juju-solutions/layer-caas-base/blob/master/lib/charms/layer/caas_base.py

Interestingly they don't make use of the multiple arguments but just strictly concatenate the two and pass it on stdin.
That feels a bit dirty as content goes, but I can see an advantage of not having temp files.

jameinel: Actually rather surprising as yaml.dump() doesn't create a document boundary (`---\n` in yaml), so it means it all comes through as a single document.

chipaca: > Actually rather surprising as yaml.dump() doesn't create a document boundary (`---\n` in yaml), so it means it all comes through as a single document.

suspect it's used with filenames, in which case it would do the right thing except for the space. The usage where both things are not strings makes no sense to me.
chipaca: so, looking at the code, pod-spec-set just munges the two things together anyway. Which is why it doesn't work with json.

I can't even.

Patch:
diff --git a/ops/model.py b/ops/model.py
index 1e82847..405396f 100644
--- a/ops/model.py
+++ b/ops/model.py
@@ -28,11 +28,18 @@ from abc import ABC, abstractmethod
 from collections.abc import Mapping, MutableMapping
 from pathlib import Path
 from subprocess import run, PIPE, CalledProcessError
+import yaml
 
 import ops
 from ops.jujuversion import JujuVersion
 
 
+if yaml.__with_libyaml__:
+    _DefaultDumper = yaml.CSafeDumper
+else:
+    _DefaultDumper = yaml.SafeDumper
+
+
 class Model:
     """Represents the Juju Model as seen from this unit.
 
@@ -1070,12 +1077,14 @@ class _ModelBackend:
     def pod_spec_set(self, spec, k8s_resources):
         tmpdir = Path(tempfile.mkdtemp('-pod-spec-set'))
         try:
-            spec_path = tmpdir / 'spec.json'
-            spec_path.write_text(json.dumps(spec))
+            spec_path = tmpdir / 'spec.yaml'
+            with spec_path.open("wt", encoding="utf8") as f:
+                yaml.dump(spec, stream=f, Dumper=_DefaultDumper)
             args = ['--file', str(spec_path)]
             if k8s_resources:
-                k8s_res_path = tmpdir / 'k8s-resources.json'
-                k8s_res_path.write_text(json.dumps(k8s_resources))
+                k8s_res_path = tmpdir / 'k8s-resources.yaml'
+                with k8s_res_path.open("wt", encoding="utf8") as f:
+                    yaml.dump(k8s_resources, stream=f, Dumper=_DefaultDumper)
                 args.extend(['--k8s-resources', str(k8s_res_path)])
             self._run('pod-spec-set', *args)
         finally:


Test patch:
diff --git a/test/test_model.py b/test/test_model.py
index 8415efa..044ad6e 100755
--- a/test/test_model.py
+++ b/test/test_model.py
@@ -452,15 +452,15 @@ class TestModel(unittest.TestCase):
             self.assertRegex(pod_spec_call[2], '.*/tmp[A-Za-z0-9._-]{8}-pod-spec-set')
 
         model.pod.set_spec({'foo': 'bar'})
-        self.assertEqual(spec_path.read_text(), '{"foo": "bar"}')
+        self.assertEqual(spec_path.read_text(), 'foo: bar\n')
         self.assertFalse(k8s_res_path.exists())
 
         fake_calls = fake_script_calls(self, clear=True)
         check_calls(fake_calls)
 
         model.pod.set_spec({'bar': 'foo'}, {'qux': 'baz'})
-        self.assertEqual(spec_path.read_text(), '{"bar": "foo"}')
-        self.assertEqual(k8s_res_path.read_text(), '{"qux": "baz"}')
+        self.assertEqual(spec_path.read_text(), 'bar: foo\n')
+        self.assertEqual(k8s_res_path.read_text(), 'qux: baz\n')
 
         fake_calls = fake_script_calls(self, clear=True)
         check_calls(fake_calls)