Repository: canonical/operator
Pull request #925 (2023-03-24 18:00:39)

Support Layer equality checks
This PR adds support for directly comparing `Layer` objects for equality. While writing tests using `ops-scenario`, I was surprised by an assertion failing with identical `Layers`. 

## Checklist

 - [ ] Have any types changed? If so, have the type annotations been updated?
 - [ ] If this code exposes model data, does it do so thoughtfully, in a way that aids understanding?

       We want to avoid simple passthrough of model data, without contextualization, where possible.

 - [ ] Do error messages, if any, present charm authors or operators with enough information to solve the problem?

## QA steps

The PR includes a new test for this feature.

## Documentation changes

This does not impact documentation, but will more correctly match users expectation that 2 identical `Layers` are equal.

## Bug reference

N/A

## Changelog

- *Adds support for Layer equality checks*



jameinel: I tried to do an inline comment, but even across 2 different browsers, github failed me. (I'm guessing there is something going on in their backend). Here was my comment:

should we raise TypeError or should we just return 'False' or return NotImplemented ?

Note that the default implementation actually returns NotImplemented:
https://docs.python.org/3/reference/datamodel.html#object.__eq__

The nice thing about NotImplemented (AIUI) is that it allows the other object to implement __eq__.

eg, if you have `x == y`

With different types, you may have x that doesn't have __eq__ but y does.

And digging a bit, it does seem to be the recommendation:
https://stackoverflow.com/questions/57143808/should-eq-compare-objects-of-two-different-types

eg:
```
    def __eq__(self, other):
        if isinstance(other, str):
            try:
                other = self.parse(str)
            except ValueError:
                return NotImplemented

        return super(DID, self).__eq__(other)
```
ghislainbourgeois: My original PR returned `NotImplemented` for this. It is probably better to have that instead of returning `False` or raising, as Python will dispatch to the `other` object in case it is not a `Layer`. If anyone creates another object to represent a `Layer` (Like something @PietroPasotti did in `ops-scenario` for `Statuses`), it would let them implement the equality across different types:

```
class MyCustomObjectActingLikeALayer():
    ...
    
    def __eq__(self, other):
        try:
            return self.to_dict() == other.to_dict()
        except TypeError:
            return NotImplemented

l1 = Layer()
l2 = MyCustomObjectActingLikeALayer()

l1 == l2  # True
```

In that example, Python will execute `l1.__eq__(l2)`, receive `NotImplemented` and call `l2.__eq__(l1)`.
benhoyt: @jameinel @ghislainbourgeois Ah yes, sorry, you're both right -- `return NotImplemented` is better here. That is what @ghislainbourgeois had, but I too quickly changed it to match the other (incorrect) `__eq__` implementations for consistency. I've changed all to return `NotImplemented` now.

This also has the effect of making equality comparisons give `False` when the types are different, but that's expected and how almost all Python types do `__eq__`, for example `{} == 1` yields `False`, it does not raise a `TypeError` -- it's just a reflection of the fact that objects aren't (usually) equal if their types differ.

Patch:
diff --git a/ops/pebble.py b/ops/pebble.py
index 3e75dfc..bf827e5 100644
--- a/ops/pebble.py
+++ b/ops/pebble.py
@@ -744,6 +744,15 @@ class Layer:
     def __repr__(self) -> str:
         return f'Layer({self.to_dict()!r})'
 
+    def __eq__(self, other: Union['LayerDict', 'Layer']) -> bool:
+        """Reports whether this layer configuration is equal to another."""
+        if isinstance(other, dict):
+            return self.to_dict() == other
+        elif isinstance(other, Layer):
+            return self.to_dict() == other.to_dict()
+        else:
+            return NotImplemented
+
     __str__ = to_yaml
 
 
@@ -819,15 +828,13 @@ class Service:
         return f'Service({self.to_dict()!r})'
 
     def __eq__(self, other: Union['_ServiceDict', 'Service']) -> bool:
-        """Compare this service description to another."""
+        """Reports whether this service configuration is equal to another."""
         if isinstance(other, dict):
             return self.to_dict() == other
         elif isinstance(other, Service):
             return self.to_dict() == other.to_dict()
         else:
-            raise ValueError(
-                f"Cannot compare pebble.Service to {type(other)}"
-            )
+            return NotImplemented
 
 
 class ServiceStartup(enum.Enum):
@@ -938,13 +945,13 @@ class Check:
         return f'Check({self.to_dict()!r})'
 
     def __eq__(self, other: Union['_CheckDict', 'Check']) -> bool:
-        """Compare this check configuration to another."""
-        if isinstance(other, dict):  # pyright: reportUnnecessaryComparison=false
+        """Reports whether this check configuration is equal to another."""
+        if isinstance(other, dict):
             return self.to_dict() == other
         elif isinstance(other, Check):
             return self.to_dict() == other.to_dict()
         else:
-            raise ValueError(f"Cannot compare pebble.Check to {type(other)}")
+            return NotImplemented
 
 
 class CheckLevel(enum.Enum):


Test patch:
diff --git a/test/test_pebble.py b/test/test_pebble.py
index 2bd206a..586b5f0 100644
--- a/test/test_pebble.py
+++ b/test/test_pebble.py
@@ -634,6 +634,36 @@ summary: Sum Mary
 
         self.assertEqual(s.services, t.services)
 
+    def test_layer_equality(self):
+        s = pebble.Layer({})
+        self._assert_empty(s)
+
+        d = {
+            'summary': 'Sum Mary',
+            'description': 'The quick brown fox!',
+            'services': {
+                'foo': {
+                    'summary': 'Foo',
+                    'command': 'echo foo',
+                },
+                'bar': {
+                    'summary': 'Bar',
+                    'command': 'echo bar',
+                },
+            }
+        }
+        t = pebble.Layer(d)
+        self.assertNotEqual(s, t)
+        self.assertNotEqual(t, {})
+        self.assertEqual(t, d)
+
+        s = pebble.Layer(d)
+        self.assertEqual(s, t)
+        self.assertNotEqual(s, {})
+        self.assertEqual(s, d)
+
+        self.assertNotEqual(s, 5)
+
 
 class TestService(unittest.TestCase):
     def _assert_empty(self, service, name):
@@ -764,8 +794,7 @@ class TestService(unittest.TestCase):
         }
         self.assertEqual(one, as_dict)
 
-        with self.assertRaises(ValueError):
-            self.assertEqual(one, 5)
+        self.assertNotEqual(one, 5)
 
 
 class TestCheck(unittest.TestCase):
@@ -849,8 +878,7 @@ class TestCheck(unittest.TestCase):
         d['level'] = 'ready'
         self.assertNotEqual(one, d)
 
-        with self.assertRaises(ValueError):
-            self.assertEqual(one, 5)
+        self.assertNotEqual(one, 5)
 
 
 class TestServiceInfo(unittest.TestCase):