Skip to content

Commit

Permalink
fix: remove unnecessary leadership guards in GrafanaCloudConfigRequir…
Browse files Browse the repository at this point in the history
…er (#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](#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.
  • Loading branch information
ca-scribner authored Aug 16, 2024
1 parent cded809 commit 394866d
Show file tree
Hide file tree
Showing 4 changed files with 79 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

LIBID = "e6f580481c1b4388aa4d2cdf412a47fa"
LIBAPI = 0
LIBPATCH = 5
LIBPATCH = 6

DEFAULT_RELATION_NAME = "grafana-cloud-config"

Expand Down Expand Up @@ -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):
Expand Down
65 changes: 65 additions & 0 deletions tests/scenario/test_grafana_cloud_integrator_lib.py
Original file line number Diff line number Diff line change
@@ -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)
)
3 changes: 2 additions & 1 deletion tests/unit/test_charm.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down
13 changes: 11 additions & 2 deletions tox.ini
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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

0 comments on commit 394866d

Please sign in to comment.