Repository: canonical/operator

Issue:
Combine Pebble layers in testing get_plan; merge "checks" and "log-targets"
As I discovered when reviewing https://github.com/canonical/operator/pull/1111 ([comment](https://github.com/canonical/operator/pull/1111#discussion_r1463868110)), we don't combine layers properly in the `_TestingPebbleClient.get_plan` result.

We already kind of have the code to combine a layer ([in `_TestingPebbleClient.add_layer`](https://github.com/canonical/operator/blob/dc47640f8ea915049d9e20a1fe4ea189c96ae8d1/ops/testing.py#L2912-L2929)), we just need to use it for `get_plan` as well.

Additionally, we need to combine/merge health checks and log targets. We have `pebble.Service._merge` (used by `add_layer` above), but we'll need similar `_merge` methods on `pebble.Check` and `pebble.LogTarget` and update appropriately.

Discussion:
dimaqq: I apologise for this project’s slightly unconventional settings wrt. quotation style and line length.

Please use `pre-commit` and it will auto fix the code un your branch 
amandahla: off topic: Why the PR title validation fails if I change it to "fix: add checks and log_targets to ops.testing"?
benhoyt: > off topic: Why the PR title validation fails if I change it to "fix: add checks and log_targets to ops.testing"?

@amandahla Must have been a glitch -- I just changed it back to that now, and it ran fine. We'll keep an eye on it.
benhoyt: @amandahla We require two reviews for PRs on this repo, so I've asked one more Charm Tech reviewer to review this (probably in the next couple of days), but it's looking good!

Patch:
diff --git a/ops/pebble.py b/ops/pebble.py
index bc4c032..7497114 100644
--- a/ops/pebble.py
+++ b/ops/pebble.py
@@ -1135,6 +1135,73 @@ class Check:
         else:
             return NotImplemented
 
+    def _merge_exec(self, other: 'ExecDict') -> None:
+        """Merges this exec object with another exec definition.
+
+        For attributes present in both objects, the passed in exec
+        attributes take precedence.
+        """
+        if self.exec is None:
+            self.exec = {}
+        for name, value in other.items():
+            if not value:
+                continue
+            if name == 'environment':
+                current = self.exec.get(name, {})
+                current.update(value)  # type: ignore
+                self.exec[name] = current
+            else:
+                self.exec[name] = value
+
+    def _merge_http(self, other: 'HttpDict') -> None:
+        """Merges this http object with another http definition.
+
+        For attributes present in both objects, the passed in http
+        attributes take precedence.
+        """
+        if self.http is None:
+            self.http = {}
+        for name, value in other.items():
+            if not value:
+                continue
+            if name == 'headers':
+                current = self.http.get(name, {})
+                current.update(value)  # type: ignore
+                self.http[name] = current
+            else:
+                self.http[name] = value
+
+    def _merge_tcp(self, other: 'TcpDict') -> None:
+        """Merges this tcp object with another tcp definition.
+
+        For attributes present in both objects, the passed in tcp
+        attributes take precedence.
+        """
+        if self.tcp is None:
+            self.tcp = {}
+        for name, value in other.items():
+            if not value:
+                continue
+            self.tcp[name] = value
+
+    def _merge(self, other: 'Check'):
+        """Merges this check object with another check definition.
+
+        For attributes present in both objects, the passed in check
+        attributes take precedence.
+        """
+        for name, value in other.__dict__.items():
+            if not value or name == 'name':
+                continue
+            if name == 'http':
+                self._merge_http(value)
+            elif name == 'tcp':
+                self._merge_tcp(value)
+            elif name == 'exec':
+                self._merge_exec(value)
+            else:
+                setattr(self, name, value)
+
 
 class CheckLevel(enum.Enum):
     """Enum of check levels."""
@@ -1162,9 +1229,8 @@ class LogTarget:
         self.location = dct.get('location', '')
         self.services: List[str] = list(dct.get('services', []))
         labels = dct.get('labels')
-        if labels is not None:
-            labels = copy.deepcopy(labels)
-        self.labels: Optional[Dict[str, str]] = labels
+        labels = copy.deepcopy(labels) if labels is not None else {}
+        self.labels: Dict[str, str] = labels
 
     def to_dict(self) -> 'LogTargetDict':
         """Convert this log target object to its dict representation."""
@@ -1189,6 +1255,22 @@ class LogTarget:
         else:
             return NotImplemented
 
+    def _merge(self, other: 'LogTarget'):
+        """Merges this log target object with another log target definition.
+
+        For attributes present in both objects, the passed in log target
+        attributes take precedence.
+        """
+        for name, value in other.__dict__.items():
+            if not value or name == 'name':
+                continue
+            if name == 'labels':
+                getattr(self, name).update(value)
+            elif name == 'services':
+                getattr(self, name).extend(value)
+            else:
+                setattr(self, name, value)
+
 
 class FileType(enum.Enum):
     """Enum of file types."""


Test patch:
diff --git a/ops/testing.py b/ops/testing.py
index 129e53a..8e00cbe 100644
--- a/ops/testing.py
+++ b/ops/testing.py
@@ -3183,6 +3183,7 @@ class _TestingPebbleClient:
             if not combine:
                 raise RuntimeError(f'400 Bad Request: layer "{label}" already exists')
             layer = self._layers[label]
+
             for name, service in layer_obj.services.items():
                 # 'override' is actually single quoted in the real error, but
                 # it shouldn't be, hopefully that gets cleaned up.
@@ -3200,11 +3201,48 @@ class _TestingPebbleClient:
                     layer.services[name] = service
                 elif service.override == 'merge':
                     if combine and name in layer.services:
-                        s = layer.services[name]
-                        s._merge(service)
+                        layer.services[name]._merge(service)
                     else:
                         layer.services[name] = service
 
+            for name, check in layer_obj.checks.items():
+                if not check.override:
+                    raise RuntimeError(
+                        f'500 Internal Server Error: layer "{label}" must define'
+                        f'"override" for check "{name}"'
+                    )
+                if check.override not in ('merge', 'replace'):
+                    raise RuntimeError(
+                        f'500 Internal Server Error: layer "{label}" has invalid '
+                        f'"override" value for check "{name}"'
+                    )
+                elif check.override == 'replace':
+                    layer.checks[name] = check
+                elif check.override == 'merge':
+                    if combine and name in layer.checks:
+                        layer.checks[name]._merge(check)
+                    else:
+                        layer.checks[name] = check
+
+            for name, log_target in layer_obj.log_targets.items():
+                if not log_target.override:
+                    raise RuntimeError(
+                        f'500 Internal Server Error: layer "{label}" must define'
+                        f'"override" for log target "{name}"'
+                    )
+                if log_target.override not in ('merge', 'replace'):
+                    raise RuntimeError(
+                        f'500 Internal Server Error: layer "{label}" has invalid '
+                        f'"override" value for log target "{name}"'
+                    )
+                elif log_target.override == 'replace':
+                    layer.log_targets[name] = log_target
+                elif log_target.override == 'merge':
+                    if combine and name in layer.log_targets:
+                        layer.log_targets[name]._merge(log_target)
+                    else:
+                        layer.log_targets[name] = log_target
+
         else:
             self._layers[label] = layer_obj
 
diff --git a/test/test_pebble.py b/test/test_pebble.py
index 5ac5d7d..2141cd6 100644
--- a/test/test_pebble.py
+++ b/test/test_pebble.py
@@ -1150,7 +1150,7 @@ class TestLogTarget:
         assert target.type == ''
         assert target.location == ''
         assert target.services == []
-        assert target.labels is None
+        assert target.labels == {}
 
     def test_name_only(self):
         target = pebble.LogTarget('tgt')
diff --git a/test/test_testing.py b/test/test_testing.py
index 8ee7596..edcd32f 100644
--- a/test/test_testing.py
+++ b/test/test_testing.py
@@ -4073,6 +4073,297 @@ class TestTestingPebbleClient:
             == plan.to_yaml()
         )
 
+    def test_add_layer_checks_combine_override_replace(self, client: _TestingPebbleClient):
+        plan = client.get_plan()
+        assert isinstance(plan, pebble.Plan)
+        assert plan.to_yaml() == '{}\n'
+        client.add_layer(
+            'foo',
+            pebble.Layer("""\
+            checks:
+                up:
+                    level: alive
+                    period: 30s
+                    threshold: 1
+                    exec:
+                        command: service nginx status
+                ready-http:
+                    level: ready
+                    period: 30s
+                    threshold: 1
+                    http:
+                        url: https://example.com:3100/health
+                        headers:
+                            header1: value1
+                ready-tcp:
+                    level: ready
+                    period: 30s
+                    threshold: 1
+                    tcp:
+                        port: 8080
+                        host: localhost
+            """),
+        )
+        client.add_layer(
+            'foo',
+            """\
+            checks:
+                up:
+                    override: replace
+                    level: alive
+                    period: 10s
+                    threshold: 5
+                    exec:
+                        command: service nginx status
+                        environment:
+                            key1: value1
+                ready-http:
+                    override: replace
+                    level: ready
+                    period: 10s
+                    threshold: 5
+                    http:
+                        url: https://example.com:3101/health
+                ready-tcp:
+                    override: replace
+                    level: ready
+                    period: 10s
+                    threshold: 5
+                    tcp:
+                        port: 8081
+                        host: localhost1
+            """,
+            combine=True,
+        )
+        # Expected changes:
+        #  * All checks should have `period` 10s and `threshold` 5
+        #  * `environment` should be added to `up` check
+        #  * `headers` should be removed from `ready-http` check
+        #  * `port` and `host` should be replaced by new ones in `ready-tcp` check
+        plan = client.get_plan()
+        assert (
+            textwrap.dedent("""\
+            checks:
+              ready-http:
+                http:
+                  url: https://example.com:3101/health
+                level: ready
+                override: replace
+                period: 10s
+                threshold: 5
+              ready-tcp:
+                level: ready
+                override: replace
+                period: 10s
+                tcp:
+                  host: localhost1
+                  port: 8081
+                threshold: 5
+              up:
+                exec:
+                  command: service nginx status
+                  environment:
+                    key1: value1
+                level: alive
+                override: replace
+                period: 10s
+                threshold: 5
+            """)
+            == plan.to_yaml()
+        )
+
+    def test_add_layer_checks_combine_override_merge(self, client: _TestingPebbleClient):
+        plan = client.get_plan()
+        assert isinstance(plan, pebble.Plan)
+        assert plan.to_yaml() == '{}\n'
+        client.add_layer(
+            'foo',
+            pebble.Layer("""\
+            checks:
+                up:
+                    level: alive
+                    period: 30s
+                    threshold: 1
+                    exec:
+                        command: service nginx status
+                        environment:
+                            key1: value1
+                ready-http:
+                    level: ready
+                    period: 30s
+                    threshold: 1
+                    http:
+                        url: https://example.com:3100/health
+                        headers:
+                            header1: value1
+                ready-tcp:
+                    level: ready
+                    period: 30s
+                    threshold: 1
+                    tcp:
+                        port: 8080
+                        host: localhost
+            """),
+        )
+        client.add_layer(
+            'foo',
+            """\
+            checks:
+                up:
+                    level: alive
+                    override: merge
+                    exec:
+                        command: service nginx status 1
+                        environment:
+                            key2: value2
+                ready-http:
+                    level: ready
+                    override: merge
+                    http:
+                        headers:
+                            header2: value2
+                ready-tcp:
+                    level: ready
+                    override: merge
+                    tcp:
+                        port: 8082
+            """,
+            combine=True,
+        )
+        # Expected changes:
+        #  * `key2` should be added to `environment` in `up` check
+        #  * `header2` should be added to `headers` in `ready-http` check
+        #  * `port` should be changed to 8082 in `ready-tcp` check
+        #  * All other properties should remain the same
+        plan = client.get_plan()
+        assert (
+            textwrap.dedent("""\
+            checks:
+              ready-http:
+                http:
+                  headers:
+                    header1: value1
+                    header2: value2
+                  url: https://example.com:3100/health
+                level: ready
+                override: merge
+                period: 30s
+                threshold: 1
+              ready-tcp:
+                level: ready
+                override: merge
+                period: 30s
+                tcp:
+                  host: localhost
+                  port: 8082
+                threshold: 1
+              up:
+                exec:
+                  command: service nginx status 1
+                  environment:
+                    key1: value1
+                    key2: value2
+                level: alive
+                override: merge
+                period: 30s
+                threshold: 1
+            """)
+            == plan.to_yaml()
+        )
+
+    def test_add_layer_log_targets_combine_override_replace(self, client: _TestingPebbleClient):
+        plan = client.get_plan()
+        assert isinstance(plan, pebble.Plan)
+        assert plan.to_yaml() == '{}\n'
+        client.add_layer(
+            'foo',
+            pebble.Layer("""\
+            log-targets:
+                baz:
+                    override: replace
+                    type: loki
+                    location: https://example.com:3100/loki/api/v1/push
+                    services:
+                        - foo
+                    labels:
+                        key: value
+                        key1: value1
+            """),
+        )
+        client.add_layer(
+            'foo',
+            pebble.Layer("""\
+            log-targets:
+                baz:
+                    override: replace
+                    type: loki
+                    location: https://example123.com:3100/loki/api/v1/push
+                    services:
+                        - foo
+                    labels:
+                        key1: value1
+            """),
+            combine=True,
+        )
+        plan = client.get_plan()
+        assert (
+            textwrap.dedent("""\
+            log-targets:
+              baz:
+                labels:
+                  key1: value1
+                location: https://example123.com:3100/loki/api/v1/push
+                override: replace
+                services:
+                - foo
+                type: loki
+            """)
+            == plan.to_yaml()
+        )
+
+    def test_add_layer_log_targets_combine_override_merge(self, client: _TestingPebbleClient):
+        client.add_layer(
+            'foo',
+            pebble.Layer("""\
+            log-targets:
+                baz:
+                    type: loki
+                    location: https://example123.com:3100/loki/api/v1/push
+                    services:
+                        - foo
+                    labels:
+                        key1: value1
+            """),
+        )
+        client.add_layer(
+            'foo',
+            pebble.Layer("""\
+            log-targets:
+                baz:
+                    override: merge
+                    services:
+                        - foo1
+                        - foo2
+            """),
+            combine=True,
+        )
+        assert (
+            textwrap.dedent("""\
+            log-targets:
+              baz:
+                labels:
+                  key1: value1
+                location: https://example123.com:3100/loki/api/v1/push
+                override: merge
+                services:
+                - foo
+                - foo1
+                - foo2
+                type: loki
+            """)
+            == client.get_plan().to_yaml()
+        )
+
     def test_add_layer_not_combined(self, client: _TestingPebbleClient):
         plan = client.get_plan()
         assert isinstance(plan, pebble.Plan)