Repository: canonical/operator
Pull request #324 (2020-06-09 02:24:24)

model: don't use --app for relation-get/-set before 2.7.0
Juju before 2.7.0 didn't support the `--app` parameter for
`relation-get` and `relation-set`, meaning that the current code
breaks in 2.6.9.

This fixes that by avoiding `--app` entirely when not needed,
raising an explicit `RuntimeError` when an attempt is made to get
or set app data in an old Juju (where we know adding `--app` will
fail).

fixes: #223

chipaca: I've changed it as discussed, such that it only adds `--app` if needed and supported, and raises an exception when the feature is requested but the Juju does not support it.

IMHO we can look into the more nuanced 2.7.x bugs separately, if needed by our users; I think a bit of hand holding to catch and explain what's going on might be in reasonable there. Alternatively maybe just logging a warning (e.g. directly in `has_app_data`) immediately before attempting something that might fail because of those known bugs might be a good middle ground.
chipaca: e.g. `app data has known bugs before 2.7.4 (see lp:1854348 and lp:1865229), the following call might fail`. Or something. Idea being it should be little work on our side but still not waste the users' time trying to figure *why* it broke. We can't stop it from breaking :)

Patch:
diff --git a/ops/jujuversion.py b/ops/jujuversion.py
index 4517886..b2b8177 100755
--- a/ops/jujuversion.py
+++ b/ops/jujuversion.py
@@ -12,6 +12,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+import os
 import re
 from functools import total_ordering
 
@@ -83,3 +84,15 @@ class JujuVersion:
         elif self.build != other.build:
             return self.build < other.build
         return False
+
+    @classmethod
+    def from_environ(cls) -> 'JujuVersion':
+        """Build a JujuVersion from JUJU_VERSION."""
+        v = os.environ.get('JUJU_VERSION')
+        if not v:
+            raise RuntimeError('environ has no JUJU_VERSION')
+        return cls(v)
+
+    def has_app_data(self) -> bool:
+        """Determine whether this juju version knows about app data."""
+        return (self.major, self.minor, self.patch) >= (2, 7, 0)
diff --git a/ops/model.py b/ops/model.py
index 10ae534..68ccc99 100644
--- a/ops/model.py
+++ b/ops/model.py
@@ -30,6 +30,7 @@ from pathlib import Path
 from subprocess import run, PIPE, CalledProcessError
 
 import ops
+from ops.jujuversion import JujuVersion
 
 
 class Model:
@@ -996,10 +997,18 @@ class _ModelBackend:
         if not isinstance(is_app, bool):
             raise TypeError('is_app parameter to relation_get must be a boolean')
 
+        if is_app:
+            version = JujuVersion.from_environ()
+            if not version.has_app_data():
+                raise RuntimeError(
+                    'getting application data is not supported on Juju version {}'.format(version))
+
+        args = ['relation-get', '-r', str(relation_id), '-', member_name]
+        if is_app:
+            args.append('--app')
+
         try:
-            return self._run('relation-get', '-r', str(relation_id),
-                             '-', member_name, '--app={}'.format(is_app),
-                             return_output=True, use_json=True)
+            return self._run(*args, return_output=True, use_json=True)
         except ModelError as e:
             if 'relation not found' in str(e):
                 raise RelationNotFoundError() from e
@@ -1009,9 +1018,18 @@ class _ModelBackend:
         if not isinstance(is_app, bool):
             raise TypeError('is_app parameter to relation_set must be a boolean')
 
+        if is_app:
+            version = JujuVersion.from_environ()
+            if not version.has_app_data():
+                raise RuntimeError(
+                    'setting application data is not supported on Juju version {}'.format(version))
+
+        args = ['relation-set', '-r', str(relation_id), '{}={}'.format(key, value)]
+        if is_app:
+            args.append('--app')
+
         try:
-            return self._run('relation-set', '-r', str(relation_id),
-                             '{}={}'.format(key, value), '--app={}'.format(is_app))
+            return self._run(*args)
         except ModelError as e:
             if 'relation not found' in str(e):
                 raise RelationNotFoundError() from e


Test patch:
diff --git a/test/test_jujuversion.py b/test/test_jujuversion.py
index d19fd60..1e38210 100755
--- a/test/test_jujuversion.py
+++ b/test/test_jujuversion.py
@@ -1,4 +1,3 @@
-#!/usr/bin/env python3
 # Copyright 2019 Canonical Ltd.
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -13,7 +12,9 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+import os
 import unittest
+import unittest.mock  # in this file, importing just 'patch' would be confusing
 
 from ops.jujuversion import JujuVersion
 
@@ -41,6 +42,24 @@ class TestJujuVersion(unittest.TestCase):
             self.assertEqual(v.patch, patch)
             self.assertEqual(v.build, build)
 
+    @unittest.mock.patch('os.environ', new={})
+    def test_from_environ(self):
+        with self.assertRaisesRegex(RuntimeError, 'environ has no JUJU_VERSION'):
+            JujuVersion.from_environ()
+
+        os.environ['JUJU_VERSION'] = 'no'
+        with self.assertRaisesRegex(RuntimeError, 'not a valid Juju version'):
+            JujuVersion.from_environ()
+
+        os.environ['JUJU_VERSION'] = '2.8.0'
+        v = JujuVersion.from_environ()
+        self.assertEqual(v, JujuVersion('2.8.0'))
+
+    def test_has_app_data(self):
+        self.assertTrue(JujuVersion('2.8.0').has_app_data())
+        self.assertTrue(JujuVersion('2.7.0').has_app_data())
+        self.assertFalse(JujuVersion('2.6.9').has_app_data())
+
     def test_parsing_errors(self):
         invalid_versions = [
             "xyz",
@@ -124,7 +143,3 @@ class TestJujuVersion(unittest.TestCase):
             self.assertEqual(JujuVersion(a) <= b, expected_weak)
             self.assertEqual(b > JujuVersion(a), expected_strict)
             self.assertEqual(b >= JujuVersion(a), expected_weak)
-
-
-if __name__ == "__main__":
-    unittest.main()
diff --git a/test/test_model.py b/test/test_model.py
index cb8189f..a3a1e02 100755
--- a/test/test_model.py
+++ b/test/test_model.py
@@ -915,6 +915,8 @@ class TestModelBackend(unittest.TestCase):
         self.assertTrue(model.unit.is_leader())
 
     def test_relation_tool_errors(self):
+        self.addCleanup(os.environ.pop, 'JUJU_VERSION', None)
+        os.environ['JUJU_VERSION'] = '2.8.0'
         err_msg = 'ERROR invalid value "$2" for option -r: relation not found'
 
         test_cases = [(
@@ -931,29 +933,79 @@ class TestModelBackend(unittest.TestCase):
             lambda: fake_script(self, 'relation-set', 'echo fooerror >&2 ; exit 1'),
             lambda: self.backend.relation_set(3, 'foo', 'bar', is_app=False),
             ops.model.ModelError,
-            [['relation-set', '-r', '3', 'foo=bar', '--app=False']],
+            [['relation-set', '-r', '3', 'foo=bar']],
         ), (
             lambda: fake_script(self, 'relation-set', 'echo {} >&2 ; exit 2'.format(err_msg)),
             lambda: self.backend.relation_set(3, 'foo', 'bar', is_app=False),
             ops.model.RelationNotFoundError,
-            [['relation-set', '-r', '3', 'foo=bar', '--app=False']],
+            [['relation-set', '-r', '3', 'foo=bar']],
+        ), (
+            lambda: None,
+            lambda: self.backend.relation_set(3, 'foo', 'bar', is_app=True),
+            ops.model.RelationNotFoundError,
+            [['relation-set', '-r', '3', 'foo=bar', '--app']],
         ), (
             lambda: fake_script(self, 'relation-get', 'echo fooerror >&2 ; exit 1'),
             lambda: self.backend.relation_get(3, 'remote/0', is_app=False),
             ops.model.ModelError,
-            [['relation-get', '-r', '3', '-', 'remote/0', '--app=False', '--format=json']],
+            [['relation-get', '-r', '3', '-', 'remote/0', '--format=json']],
         ), (
             lambda: fake_script(self, 'relation-get', 'echo {} >&2 ; exit 2'.format(err_msg)),
             lambda: self.backend.relation_get(3, 'remote/0', is_app=False),
             ops.model.RelationNotFoundError,
-            [['relation-get', '-r', '3', '-', 'remote/0', '--app=False', '--format=json']],
+            [['relation-get', '-r', '3', '-', 'remote/0', '--format=json']],
+        ), (
+            lambda: None,
+            lambda: self.backend.relation_get(3, 'remote/0', is_app=True),
+            ops.model.RelationNotFoundError,
+            [['relation-get', '-r', '3', '-', 'remote/0', '--app', '--format=json']],
         )]
 
-        for do_fake, run, exception, calls in test_cases:
-            do_fake()
-            with self.assertRaises(exception):
-                run()
-            self.assertEqual(fake_script_calls(self, clear=True), calls)
+        for i, (do_fake, run, exception, calls) in enumerate(test_cases):
+            with self.subTest(i):
+                do_fake()
+                with self.assertRaises(exception):
+                    run()
+                self.assertEqual(fake_script_calls(self, clear=True), calls)
+
+    def test_relation_get_juju_version_quirks(self):
+        self.addCleanup(os.environ.pop, 'JUJU_VERSION', None)
+
+        fake_script(self, 'relation-get', '''echo '{"foo": "bar"}' ''')
+
+        # on 2.7.0+, things proceed as expected
+        for v in ['2.8.0', '2.7.0']:
+            with self.subTest(v):
+                os.environ['JUJU_VERSION'] = v
+                rel_data = self.backend.relation_get(1, 'foo/0', is_app=True)
+                self.assertEqual(rel_data, {"foo": "bar"})
+                calls = [' '.join(i) for i in fake_script_calls(self, clear=True)]
+                self.assertEqual(calls, ['relation-get -r 1 - foo/0 --app --format=json'])
+
+        # before 2.7.0, it just fails (no --app support)
+        os.environ['JUJU_VERSION'] = '2.6.9'
+        with self.assertRaisesRegex(RuntimeError, 'not supported on Juju version 2.6.9'):
+            self.backend.relation_get(1, 'foo/0', is_app=True)
+        self.assertEqual(fake_script_calls(self), [])
+
+    def test_relation_set_juju_version_quirks(self):
+        self.addCleanup(os.environ.pop, 'JUJU_VERSION', None)
+
+        fake_script(self, 'relation-set', 'exit 0')
+
+        # on 2.7.0+, things proceed as expected
+        for v in ['2.8.0', '2.7.0']:
+            with self.subTest(v):
+                os.environ['JUJU_VERSION'] = v
+                self.backend.relation_set(1, 'foo', 'bar', is_app=True)
+                calls = [' '.join(i) for i in fake_script_calls(self, clear=True)]
+                self.assertEqual(calls, ['relation-set -r 1 foo=bar --app'])
+
+        # before 2.7.0, it just fails always (no --app support)
+        os.environ['JUJU_VERSION'] = '2.6.9'
+        with self.assertRaisesRegex(RuntimeError, 'not supported on Juju version 2.6.9'):
+            self.backend.relation_set(1, 'foo', 'bar', is_app=True)
+        self.assertEqual(fake_script_calls(self), [])
 
     def test_status_get(self):
         # taken from actual Juju output