From 394866d63b2d444bb96edc370917132a5949a94b Mon Sep 17 00:00:00 2001 From: Andrew Scribner Date: Fri, 16 Aug 2024 14:38:00 -0400 Subject: [PATCH] fix: remove unnecessary leadership guards in GrafanaCloudConfigRequirer (#13) * fix: remove unnecessary leadership guards in GrafanaCloudConfigRequirer Previously, GrafanaCloudConfigRequirer would only emit a cloud_config_* event on relation changed/broken if this unit is a leader. This caused issues with multi-unit deployments as described in [#12](https://github.com/canonical/grafana-cloud-integrator/issues/12). Since GrafanaCloudConfigRequirer only reads the relation remote's application_data, there is no need for a leadership guard because non-leader units have permission to read this data. The current commit removes these guards. --- .../v0/cloud_config_requirer.py | 8 +-- .../test_grafana_cloud_integrator_lib.py | 65 +++++++++++++++++++ tests/unit/test_charm.py | 3 +- tox.ini | 13 +++- 4 files changed, 79 insertions(+), 10 deletions(-) create mode 100644 tests/scenario/test_grafana_cloud_integrator_lib.py diff --git a/lib/charms/grafana_cloud_integrator/v0/cloud_config_requirer.py b/lib/charms/grafana_cloud_integrator/v0/cloud_config_requirer.py index 4b829a2..8fd8550 100644 --- a/lib/charms/grafana_cloud_integrator/v0/cloud_config_requirer.py +++ b/lib/charms/grafana_cloud_integrator/v0/cloud_config_requirer.py @@ -6,7 +6,7 @@ LIBID = "e6f580481c1b4388aa4d2cdf412a47fa" LIBAPI = 0 -LIBPATCH = 5 +LIBPATCH = 6 DEFAULT_RELATION_NAME = "grafana-cloud-config" @@ -53,15 +53,9 @@ def __init__(self, charm, relation_name = DEFAULT_RELATION_NAME): self.framework.observe(event, self._on_relation_broken) def _on_relation_changed(self, event): - if not self._charm.unit.is_leader(): - return - self.on.cloud_config_available.emit() # pyright: ignore def _on_relation_broken(self, event): - if not self._charm.unit.is_leader(): - return - self.on.cloud_config_revoked.emit() # pyright: ignore def _is_not_empty(self, s): diff --git a/tests/scenario/test_grafana_cloud_integrator_lib.py b/tests/scenario/test_grafana_cloud_integrator_lib.py new file mode 100644 index 0000000..691a46d --- /dev/null +++ b/tests/scenario/test_grafana_cloud_integrator_lib.py @@ -0,0 +1,65 @@ +import pytest +from charms.grafana_cloud_integrator.v0.cloud_config_requirer import ( + CloudConfigAvailableEvent, + CloudConfigRevokedEvent, + GrafanaCloudConfigRequirer, +) +from ops import CharmBase, Framework +from scenario import Context, Relation, State + + +class MyCharm(CharmBase): + def __init__(self, framework: Framework): + super().__init__(framework) + self.cloud = GrafanaCloudConfigRequirer(self) + + +@pytest.fixture() +def mycharm_context(): + """Returns a Context object with a MyCharm instance.""" + return Context( + charm_type=MyCharm, + meta={ + "name": "my-charm", + "requires": { + "grafana-cloud-config": {"interface": "grafana_cloud_config", "limit": 1} + }, + }, + ) + + +@pytest.mark.parametrize("is_leader", [(True,), (False,)]) +def test_requirer_emits_cloud_config_available_event_on_relation_changed( + is_leader, mycharm_context +): + # GIVEN a grafana-cloud-config relation and a leadership status + grafana_cloud_config_relation = Relation("grafana-cloud-config") + state = State(leader=is_leader, relations=[grafana_cloud_config_relation]) + + # WHEN the grafana-cloud-config relation changes + mycharm_context.run(grafana_cloud_config_relation.changed_event, state) + + # THEN the CloudConfigAvailableEvent event is emitted + assert any( + event + for event in mycharm_context.emitted_events + if isinstance(event, CloudConfigAvailableEvent) + ) + + +@pytest.mark.parametrize("is_leader", [(True,), (False,)]) +def test_requirer_emits_cloud_config_revoked_event_on_relation_broken(is_leader, mycharm_context): + # GIVEN a grafana-cloud-config relation + grafana_cloud_config_relation = Relation("grafana-cloud-config") + # AND GIVEN leadership/non-leadership + state = State(leader=is_leader, relations=[grafana_cloud_config_relation]) + + # WHEN the grafana-cloud-config relation changes + mycharm_context.run(grafana_cloud_config_relation.broken_event, state) + + # THEN the CloudConfigAvailableEvent event is emitted + assert any( + event + for event in mycharm_context.emitted_events + if isinstance(event, CloudConfigRevokedEvent) + ) diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index 08c83c5..7ddd60e 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -6,10 +6,11 @@ import unittest import ops.testing -from charm import GrafanaCloudIntegratorCharm from ops.model import ActiveStatus, BlockedStatus from ops.testing import Harness +from charm import GrafanaCloudIntegratorCharm + class TestCharm(unittest.TestCase): def setUp(self): diff --git a/tox.ini b/tox.ini index a76a154..98bda1b 100644 --- a/tox.ini +++ b/tox.ini @@ -69,11 +69,20 @@ deps = -r{toxinidir}/requirements.txt commands = coverage run --source={[vars]src_path} \ - -m pytest --ignore={[vars]tst_path}integration -v --tb native -s {posargs} + -m pytest -v --tb native -s {posargs} {[vars]tst_path}unit coverage report [testenv:scenario] description = Run scenario tests +deps = + pytest + coverage[toml] + ops-scenario>=4.0.3 + -r{toxinidir}/requirements.txt +commands = + coverage run --source={[vars]src_path} \ + -m pytest -v --tb native -s {posargs} {[vars]tst_path}scenario + coverage report [testenv:integration] description = Run integration tests @@ -83,4 +92,4 @@ deps = pytest-operator -r{toxinidir}/requirements.txt commands = - pytest -v --tb native --ignore={[vars]tst_path}unit --log-cli-level=INFO -s {posargs} + pytest -v --tb native --log-cli-level=INFO -s {posargs} {[vars]tst_path}integration