Repository: canonical/operator
Pull request #1134 (2024-02-25 23:12:55)

feat: add the ability to compare two Plan objects with == and create a Plan from a dict
This PR adds two minor features to `pebble.Plan` objects to simplify testing (particularly with Scenario):

* The ability to compare two `pebble.Plan` objects, or a `Plan` with a `dict`, with `==` - comparison to any other type of object  will still return False.
* The ability to instantiate a `pebble.Plan` object from a `pebble.PlanDict` (or equivalent dict).

This aligns the `Plan` functionality with `Layer` objects.

tonyandrewmeyer: The spacing lint errors in [pebble.py](ops/pebble.py) and [docs/conf.py](docs/conf.py) are ones that Ruff wants. This seems to be a change in 0.2.2 ([#9266](https://github.com/astral-sh/ruff/pull/9266)), but I'm not sure why it didn't show up in the PR that bumped ruff to 0.2.2. I'll make the adjustments, since they seem minorly better than `noqa`s.
benhoyt: Do you mean the PR that bumped ruff to 0.2.1? (https://github.com/canonical/operator/commit/1836df5affb42b3183125b1904c794090aa1862b#diff-ef2cef9f88b4fe09ca3082140e67f5ad34fb65fb6e228f119d3812261ae51449) I wonder if we should pin an exact version for Ruff, if this is going to be a problem (particularly as we have preview turned on).
tonyandrewmeyer: > Do you mean the PR that bumped ruff to 0.2.1? ([1836df5#diff-ef2cef9f88b4fe09ca3082140e67f5ad34fb65fb6e228f119d3812261ae51449](https://github.com/canonical/operator/commit/1836df5affb42b3183125b1904c794090aa1862b#diff-ef2cef9f88b4fe09ca3082140e67f5ad34fb65fb6e228f119d3812261ae51449)) I wonder if we should pin an exact version for Ruff, if this is going to be a problem (particularly as we have preview turned on).

Ha, yes. I remembered it going to 0.2 and assumed it must have been 0.2.2 but the "why did it change" obvious answer is indeed that 😂.

We currently have it `ruff~=0.2.1`- I would have thought adding new rules would be a minor bump, not a patch one.looking over the changelog, 0.2 had breaking changes from 0.1, so it's like semvar shifted to the right 😞.

I'd be fine pinning it to an exact version (we could reconsider at 1.0.0 maybe). In the other hand, if the changes tend to be small, I'm also fine with just dealing with them as they come, since we have a small team, so don't generally have a lot of PRs at once.
benhoyt: > We currently have it ruff~=0.2.1 - I would have thought adding new rules would be a minor bump, not a patch one. Looking over the changelog, 0.2 had breaking changes from 0.1, so it's like semvar shifted to the right 😞.

Heh, Pyright-style "semver". :-)

> I'd be fine pinning it to an exact version (we could reconsider at 1.0.0 maybe). In the other hand, if the changes tend to be small, I'm also fine with just dealing with them as they come, since we have a small team, so don't generally have a lot of PRs at once.

Okay, let's leave as is for now, and we can always reconsider if it keeps being a problem.

Patch:
diff --git a/CHANGES.md b/CHANGES.md
index 8a52d81..ffe365b 100644
--- a/CHANGES.md
+++ b/CHANGES.md
@@ -2,6 +2,8 @@
 
 * `StopEvent`, `RemoveEvent`, and all `LifeCycleEvent`s are no longer deferrable, and will raise a `RuntimeError` if `defer()` is called on the event object.
 * Added `ActionEvent.id`, exposing the JUJU_ACTION_UUID environment variable.
+* Added support for creating `pebble.Plan` objects by passing in a `pebble.PlanDict`, the
+  ability to compare two `Plan` objects with `==`, and the ability to create an empty Plan with `Plan()`.
 
 # 2.10.0
 
diff --git a/docs/conf.py b/docs/conf.py
index 556500e..53011cb 100644
--- a/docs/conf.py
+++ b/docs/conf.py
@@ -22,12 +22,14 @@ sys.path.insert(0, str(pathlib.Path(__file__).parent.parent))
 
 _old_compute_navigation_tree = furo._compute_navigation_tree
 
+
 def _compute_navigation_tree(context):
     tree_html = _old_compute_navigation_tree(context)
     if not tree_html and context.get("toc"):
         tree_html = furo.navigation.get_navigation_tree(context["toc"])
     return tree_html
 
+
 furo._compute_navigation_tree = _compute_navigation_tree
 
 
diff --git a/ops/pebble.py b/ops/pebble.py
index cc091de..0b96d59 100644
--- a/ops/pebble.py
+++ b/ops/pebble.py
@@ -179,13 +179,17 @@ _HeaderHandler = Callable[[bytes], None]
 
 class _Tempfile(Protocol):
     name = ''
+
     def write(self, data: bytes): ...
+
     def close(self): ...
 
 
 class _FileLikeIO(Protocol[typing.AnyStr]):  # That also covers TextIO and BytesIO
     def read(self, __n: int = ...) -> typing.AnyStr: ...  # for BinaryIO
+
     def write(self, __s: typing.AnyStr) -> int: ...
+
     def __enter__(self) -> typing.IO[typing.AnyStr]: ...
 
 
@@ -276,9 +280,13 @@ if TYPE_CHECKING:
 
 class _WebSocket(Protocol):
     def connect(self, url: str, socket: socket.socket): ...
+
     def shutdown(self): ...
+
     def send(self, payload: str): ...
+
     def send_binary(self, payload: bytes): ...
+
     def recv(self) -> Union[str, bytes]: ...
 
 
@@ -735,8 +743,11 @@ class Plan:
     documented at https://github.com/canonical/pebble/#layer-specification.
     """
 
-    def __init__(self, raw: str):
-        d = yaml.safe_load(raw) or {}  # type: ignore
+    def __init__(self, raw: Optional[Union[str, 'PlanDict']] = None):
+        if isinstance(raw, str):  # noqa: SIM108
+            d = yaml.safe_load(raw) or {}  # type: ignore
+        else:
+            d = raw or {}
         d = typing.cast('PlanDict', d)
 
         self._raw = raw
@@ -788,6 +799,13 @@ class Plan:
 
     __str__ = to_yaml
 
+    def __eq__(self, other: Union['PlanDict', 'Plan']) -> bool:
+        if isinstance(other, dict):
+            return self.to_dict() == other
+        elif isinstance(other, Plan):
+            return self.to_dict() == other.to_dict()
+        return NotImplemented
+
 
 class Layer:
     """Represents a Pebble configuration layer.
diff --git a/tox.ini b/tox.ini
index d1ba3fe..a5330ae 100644
--- a/tox.ini
+++ b/tox.ini
@@ -44,7 +44,7 @@ commands =
 [testenv:lint]
 description = Check code against coding style standards
 deps =
-    ruff~=0.2.1
+    ruff~=0.2.2
 commands =
     ruff check --preview
 


Test patch:
diff --git a/test/test_pebble.py b/test/test_pebble.py
index 4709e94..7c7f429 100644
--- a/test/test_pebble.py
+++ b/test/test_pebble.py
@@ -492,10 +492,6 @@ single log
 
 
 class TestPlan(unittest.TestCase):
-    def test_no_args(self):
-        with self.assertRaises(TypeError):
-            pebble.Plan()  # type: ignore
-
     def test_services(self):
         plan = pebble.Plan('')
         self.assertEqual(plan.services, {})
@@ -589,6 +585,37 @@ log-targets:
         self.assertEqual(plan.to_yaml(), reformed)
         self.assertEqual(str(plan), reformed)
 
+    def test_plandict(self):
+        # Starting with nothing, we get the empty result.
+        plan = pebble.Plan({})
+        self.assertEqual(plan.to_dict(), {})
+        plan = pebble.Plan()
+        self.assertEqual(plan.to_dict(), {})
+
+        # With a service, we return validated yaml content.
+        raw: pebble.PlanDict = {
+            "services": {
+                "foo": {
+                    "override": "replace",
+                    "command": "echo foo",
+                },
+            },
+            "checks": {
+                "bar": {
+                    "http": {"url": "https://example.com/"},
+                },
+            },
+            "log-targets": {
+                "baz": {
+                    "override": "replace",
+                    "type": "loki",
+                    "location": "https://example.com:3100/loki/api/v1/push",
+                },
+            },
+        }
+        plan = pebble.Plan(raw)
+        self.assertEqual(plan.to_dict(), raw)
+
     def test_service_equality(self):
         plan = pebble.Plan("""
 services:
@@ -610,6 +637,114 @@ services:
         }
         self.assertEqual(plan.services, services_as_dict)
 
+    def test_plan_equality(self):
+        plan1 = pebble.Plan('''
+services:
+  foo:
+    override: replace
+    command: echo foo
+''')
+        self.assertNotEqual(plan1, "foo")
+        plan2 = pebble.Plan('''
+services:
+  foo:
+    command: echo foo
+    override: replace
+''')
+        self.assertEqual(plan1, plan2)
+        plan1_as_dict = {
+            "services": {
+                "foo": {
+                    "command": "echo foo",
+                    "override": "replace",
+                },
+            },
+        }
+        self.assertEqual(plan1, plan1_as_dict)
+        plan3 = pebble.Plan('''
+services:
+  foo:
+    override: replace
+    command: echo bar
+''')
+        # Different command.
+        self.assertNotEqual(plan1, plan3)
+        plan4 = pebble.Plan('''
+services:
+ foo:
+  override: replace
+  command: echo foo
+
+checks:
+ bar:
+  http:
+   https://example.com/
+
+log-targets:
+ baz:
+  override: replace
+  type: loki
+  location: https://example.com:3100/loki/api/v1/push
+''')
+        plan5 = pebble.Plan('''
+services:
+ foo:
+  override: replace
+  command: echo foo
+
+checks:
+ bar:
+  http:
+   https://different.example.com/
+
+log-targets:
+ baz:
+  override: replace
+  type: loki
+  location: https://example.com:3100/loki/api/v1/push
+''')
+        # Different checks.bar.http
+        self.assertNotEqual(plan4, plan5)
+        plan6 = pebble.Plan('''
+services:
+ foo:
+  override: replace
+  command: echo foo
+
+checks:
+ bar:
+  http:
+   https://example.com/
+
+log-targets:
+ baz:
+  override: replace
+  type: loki
+  location: https://example.com:3200/loki/api/v1/push
+''')
+        # Reordered elements.
+        self.assertNotEqual(plan4, plan6)
+        plan7 = pebble.Plan('''
+services:
+ foo:
+  command: echo foo
+  override: replace
+
+log-targets:
+ baz:
+  type: loki
+  override: replace
+  location: https://example.com:3100/loki/api/v1/push
+
+checks:
+ bar:
+  http:
+   https://example.com/
+
+''')
+        # Reordered sections.
+        self.assertEqual(plan4, plan7)
+
 
 class TestLayer(unittest.TestCase):
     def _assert_empty(self, layer: pebble.Layer):