Repository: canonical/operator
Pull request #326 (2020-06-10 11:01:21)

make abc TestMain private to help pytest


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_main.py b/test/test_main.py
index 76bbb63..bb958bb 100755
--- a/test/test_main.py
+++ b/test/test_main.py
@@ -131,7 +131,7 @@ class CharmInitTestCase(unittest.TestCase):
         self.assertFalse(warn_cm)
 
 
-class TestMain(abc.ABC):
+class _TestMain(abc.ABC):
 
     @abc.abstractmethod
     def _setup_entry_point(self, directory, entry_point):
@@ -643,7 +643,7 @@ log_debug: {}
         self.assertEqual(state['observed_event_types'], [InstallEvent])
 
 
-class TestMainWithNoDispatch(TestMain, unittest.TestCase):
+class TestMainWithNoDispatch(_TestMain, unittest.TestCase):
     has_dispatch = False
     hooks_are_symlinks = True
 
@@ -717,7 +717,7 @@ class TestMainWithNoDispatchButScriptsAreCopies(TestMainWithNoDispatch):
         shutil.copy(charm_path, str(path))
 
 
-class TestMainWithDispatch(TestMain, unittest.TestCase):
+class TestMainWithDispatch(_TestMain, unittest.TestCase):
     has_dispatch = True
 
     def _setup_entry_point(self, directory, entry_point):
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