Repository: canonical/operator (language: python)

Issue:
Remove pebble.Error name and message methods
Per discussion on Mattermost:

@PietroPasotti was asking about `ops.pebble`: `PathError.message` vs `Error.message()`. The issue is the base `Error` class defines `message()` as a method, but `PathError`, which inherits from `Error`, defines `message` as a simple `str` attribute. This confuses static type checkers, not to mention human programmers!

As it happens, I was looking at some related code with @rwcarlsen the other day, and noted at https://github.com/canonical/operator/pull/754#discussion_r878941759 that the `Error.name()` and `Error.message()` methods were added by @rbarry82 in https://github.com/canonical/operator/pull/567 (I reviewed that and should have noticed then, sorry), but the code that used them was mostly reverted later.

As far as I can tell by grepping the code, we don't use `.name()` anywhere at all, and the only place we use `.message()` is in one debug log and one unit test, so it would be easy to remove. I realize this doesn't address charms that might use them -- they are part of our public API now: https://ops.readthedocs.io/en/latest/#ops.pebble.Error.message -- but I think it's _very_ unlikely that they're being used outside `operator`. My reasoning: there are only a small number of sidecar charms to begin with, only some of those would catch `pebble.Error` or its subclasses directly, even fewer of those would call methods on the resulting objects.

In any case, I don't think `name()` and `message()` are a good idea, and I think they should be removed. I don't think `name()` should exist (people can use `.__class__.__name__` or whatever they specifically want) and `message()` should just be `__str__()`.

If we remove these it will solve @PietroPasotti's typing issue while we're at it.


Discussion / hints:
For the records, comments from Mattermost:

> @rwcarlsen: That all sounds pretty reasonable to me.  With the caveat that we can concretely back up the assertion that they probably aren't used with some evidence.  We (ops team) have been starting to entertain ideas for better ways of keeping a pulse on how ops changes affect the greater ecosystem of charms beyond just publishing a release and hoping we didn't screw too much up.

> @rbarry82: +1 from me to remove them. I didn't notice the collision with PathError either. __str__, __repr__, .__class__.__name__ are all going to be slightly different than what it is now, but, as mentioned, the entire ContextManager got reverted for simplicity anyway.
>
> I'm not sure that anybody actually checks these. Every use case I've seen is "if Pebble throws an exception, we're in a scenario where we can't do anything to resolve it anyway, so log a more meaningful message from the actual workload"

> @benhoyt: Yeah, that seems likely.
>
> Robert, that might be hard to do. We could at least check the Canonical charms. And perhaps write a script to crawl code in https://github.com/canonical/operator/network/dependents? Though most of them look like they might be Canoncial's charms. Alternatively we could crawl code on all charms in the store.
>
> I'll let you guys figure that out. :slightly_smiling_face:

Fix patch:
diff --git a/ops/model.py b/ops/model.py
index 9412ae989..79af88f8b 100644
--- a/ops/model.py
+++ b/ops/model.py
@@ -1418,17 +1418,17 @@ def can_connect(self) -> bool:
             #  instance that is in fact 'ready'.
             self._pebble.get_system_info()
         except pebble.ConnectionError as e:
-            logger.debug("Pebble API is not ready; ConnectionError: %s", e.message())
+            logger.debug("Pebble API is not ready; ConnectionError: %s", e)
             return False
         except FileNotFoundError as e:
             # In some cases, charm authors can attempt to hit the Pebble API before it has had the
             # chance to create the UNIX socket in the shared volume.
-            logger.debug("Pebble API is not ready; UNIX socket not found:", str(e))
+            logger.debug("Pebble API is not ready; UNIX socket not found: %s", e)
             return False
         except pebble.APIError as e:
             # An API error is only raised when the Pebble API returns invalid JSON, or the response
             # cannot be read. Both of these are a likely indicator that something is wrong.
-            logger.warning("Pebble API is not ready; APIError: %s", str(e))
+            logger.warning("Pebble API is not ready; APIError: %s", e)
             return False
         return True
 
diff --git a/ops/pebble.py b/ops/pebble.py
index 71de6fb85..0c15c3d93 100644
--- a/ops/pebble.py
+++ b/ops/pebble.py
@@ -333,14 +333,6 @@ class Error(Exception):
     def __repr__(self):
         return '<{}.{} {}>'.format(type(self).__module__, type(self).__name__, self.args)
 
-    def name(self):
-        """Return a string representation of the model plus class."""
-        return '<{}.{}>'.format(type(self).__module__, type(self).__name__)
-
-    def message(self):
-        """Return the message passed as an argument."""
-        return self.args[0]
-
 
 class TimeoutError(TimeoutError, Error):
     """Raised when a polling timeout occurs."""


Test patch:
diff --git a/ops/testing.py b/ops/testing.py
index d92609bdd..98507a5aa 100755
--- a/ops/testing.py
+++ b/ops/testing.py
@@ -1406,6 +1406,7 @@ def _cleanup(self):
         if self._resource_dir is not None:
             self._resource_dir.cleanup()
             self._resource_dir = None
+        self._harness_tmp_dir.cleanup()
 
     def _get_resource_dir(self) -> pathlib.Path:
         if self._resource_dir is None:
diff --git a/test/test_model.py b/test/test_model.py
index 173168fa6..e1ff14d17 100755
--- a/test/test_model.py
+++ b/test/test_model.py
@@ -33,7 +33,14 @@
 from ops import model
 from ops._private import yaml
 from ops.charm import RelationMeta, RelationRole
-from ops.pebble import APIError, FileInfo, FileType, ServiceInfo
+from ops.pebble import (
+    APIError,
+    ConnectionError,
+    FileInfo,
+    FileType,
+    ServiceInfo,
+    SystemInfo,
+)
 
 
 class TestModel(unittest.TestCase):
@@ -1354,8 +1361,9 @@ def test_replan(self):
         self.container.replan()
         self.assertEqual(self.pebble.requests, [('replan',)])
 
-    def test_get_system_info(self):
-        self.container.can_connect()
+    def test_can_connect(self):
+        self.pebble.responses.append(SystemInfo.from_dict({'version': '1.0.0'}))
+        self.assertTrue(self.container.can_connect())
         self.assertEqual(self.pebble.requests, [('get_system_info',)])
 
     def test_start(self):
@@ -1687,10 +1695,37 @@ def test_remove_path(self):
             ('remove_path', '/path/2', True),
         ])
 
-    def test_bare_can_connect_call(self):
-        self.pebble.responses.append('dummy')
+    def test_can_connect_simple(self):
+        self.pebble.responses.append(SystemInfo.from_dict({'version': '1.0.0'}))
         self.assertTrue(self.container.can_connect())
 
+    def test_can_connect_connection_error(self):
+        def raise_error():
+            raise ConnectionError('connection error!')
+        self.pebble.get_system_info = raise_error
+        with self.assertLogs('ops.model', level='DEBUG') as cm:
+            self.assertFalse(self.container.can_connect())
+        self.assertEqual(len(cm.output), 1)
+        self.assertRegex(cm.output[0], r'DEBUG:ops.model:.*: connection error!')
+
+    def test_can_connect_file_not_found_error(self):
+        def raise_error():
+            raise FileNotFoundError('file not found!')
+        self.pebble.get_system_info = raise_error
+        with self.assertLogs('ops.model', level='DEBUG') as cm:
+            self.assertFalse(self.container.can_connect())
+        self.assertEqual(len(cm.output), 1)
+        self.assertRegex(cm.output[0], r'DEBUG:ops.model:.*: file not found!')
+
+    def test_can_connect_api_error(self):
+        def raise_error():
+            raise APIError('body', 404, 'status', 'api error!')
+        self.pebble.get_system_info = raise_error
+        with self.assertLogs('ops.model') as cm:
+            self.assertFalse(self.container.can_connect())
+        self.assertEqual(len(cm.output), 1)
+        self.assertRegex(cm.output[0], r'WARNING:ops.model:.*: api error!')
+
     def test_exec(self):
         self.pebble.responses.append('fake_exec_process')
         p = self.container.exec(
@@ -1759,6 +1794,7 @@ def autostart_services(self):
 
     def get_system_info(self):
         self.requests.append(('get_system_info',))
+        return self.responses.pop(0)
 
     def replan_services(self):
         self.requests.append(('replan',))
diff --git a/test/test_pebble.py b/test/test_pebble.py
index 924d11014..d07679584 100644
--- a/test/test_pebble.py
+++ b/test/test_pebble.py
@@ -1248,7 +1248,7 @@ def handle_body(data, done=False):
                     if not test.error:
                         self.fail('unexpected error:', err)
                         break
-                    self.assertEqual(test.error, err.message())
+                    self.assertEqual(test.error, str(err))
                 else:
                     if test.error:
                         self.fail('missing expected error: {!r}'.format(test.error))