From b06e410fc9c26d84fc2e3ed42c440900b4258703 Mon Sep 17 00:00:00 2001 From: sed-i <82407168+sed-i@users.noreply.github.com> Date: Sat, 2 Nov 2024 12:40:26 +0100 Subject: [PATCH 01/13] Add config option for disabling reporting --- charmcraft.yaml | 11 +++++++++++ src/charm.py | 23 ++++++++++++++++++++++- 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/charmcraft.yaml b/charmcraft.yaml index 79bd42e4..b20bb9fe 100644 --- a/charmcraft.yaml +++ b/charmcraft.yaml @@ -212,3 +212,14 @@ config: automatically deduced from it). See https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/ type: string + phone_home: + description: | + When disabled, Grafana will be configured to not send anonymous usage statistics to stats.grafana.org, nor + periodically check for updates. + It is very helpful to the Grafana project, so please leave this enabled. + + When enabled, Grafana will use its default values for analytics. + + Ref: https://grafana.com/docs/grafana/latest/setup-grafana/configure-grafana/#analytics + type: boolean + default: true diff --git a/src/charm.py b/src/charm.py index bee5a36f..9fa44ebd 100755 --- a/src/charm.py +++ b/src/charm.py @@ -752,7 +752,7 @@ def _generate_grafana_config(self) -> str: can be set in ENV variables, but leave for expansion later so we can hide auth secrets """ - configs = [self._generate_tracing_config()] + configs = [self._generate_tracing_config(), self._generate_analytics_config()] if self.has_db: configs.append(self._generate_database_config()) else: @@ -799,6 +799,27 @@ def _generate_tracing_config(self) -> str: ret = data.getvalue() return ret + def _generate_analytics_config(self) -> str: + """Generate analytics configuration. + + Returns: + A string containing the analytics config to be stubbed into the config file. + """ + if self.config["phone_home"]: + return "" + config_ini = configparser.ConfigParser() + # Ref: https://grafana.com/docs/grafana/latest/setup-grafana/configure-grafana/#analytics + config_ini["analytics"] = { + "reporting_enabled": "false", + "check_for_updates": "false", + "check_for_plugin_updates": "false", + } + + data = StringIO() + config_ini.write(data) + ret = data.getvalue() + return ret + def _generate_database_config(self) -> str: """Generate a database configuration. From d24b8daadea1183f319650fc66811a67c302f7d9 Mon Sep 17 00:00:00 2001 From: sed-i <82407168+sed-i@users.noreply.github.com> Date: Tue, 12 Nov 2024 13:34:07 -0500 Subject: [PATCH 02/13] Rename config option --- charmcraft.yaml | 2 +- src/charm.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/charmcraft.yaml b/charmcraft.yaml index b20bb9fe..23349ffd 100644 --- a/charmcraft.yaml +++ b/charmcraft.yaml @@ -212,7 +212,7 @@ config: automatically deduced from it). See https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/ type: string - phone_home: + reporting_enabled: description: | When disabled, Grafana will be configured to not send anonymous usage statistics to stats.grafana.org, nor periodically check for updates. diff --git a/src/charm.py b/src/charm.py index 9fa44ebd..f202209a 100755 --- a/src/charm.py +++ b/src/charm.py @@ -805,7 +805,7 @@ def _generate_analytics_config(self) -> str: Returns: A string containing the analytics config to be stubbed into the config file. """ - if self.config["phone_home"]: + if self.config["reporting_enabled"]: return "" config_ini = configparser.ConfigParser() # Ref: https://grafana.com/docs/grafana/latest/setup-grafana/configure-grafana/#analytics From 8ebe44991ad3e500d2004da62e86f5e80a66aedb Mon Sep 17 00:00:00 2001 From: sed-i <82407168+sed-i@users.noreply.github.com> Date: Tue, 12 Nov 2024 13:49:39 -0500 Subject: [PATCH 03/13] Cleanup deps --- requirements.txt | 4 +--- tests/scenario/conftest.py | 2 +- tests/scenario/test_charm.py | 22 +++------------------- tox.ini | 2 +- 4 files changed, 6 insertions(+), 24 deletions(-) diff --git a/requirements.txt b/requirements.txt index fc139c8b..7eac6b3a 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,8 +1,6 @@ jinja2 < 3 lightkube >= 0.11 -markupsafe == 2.0.1 -# pinned to 2.16 as 2.17 breaks our unittests -ops == 2.16 +ops pyyaml urllib3 jsonschema diff --git a/tests/scenario/conftest.py b/tests/scenario/conftest.py index 06370c77..24caf6e3 100644 --- a/tests/scenario/conftest.py +++ b/tests/scenario/conftest.py @@ -1,7 +1,7 @@ from unittest.mock import patch import pytest -from scenario import Context +from ops.testing import Context from charm import GrafanaCharm diff --git a/tests/scenario/test_charm.py b/tests/scenario/test_charm.py index f20d036d..4fad0653 100644 --- a/tests/scenario/test_charm.py +++ b/tests/scenario/test_charm.py @@ -1,21 +1,5 @@ import pytest -from scenario import State, PeerRelation +from ops.testing import State, PeerRelation - -@pytest.mark.parametrize("leader", (True, False)) -def test_peer_data_does_not_raise_if_no_peers(ctx, leader): - state = State(leader=leader) - with ctx.manager("update-status", state) as mgr: - charm = mgr.charm - assert not charm.peers - charm.set_peer_data("1", 2) - assert charm.get_peer_data("1") == {} - - -def test_peer_data_if_peers(ctx): - state = State(leader=True, relations=[PeerRelation("grafana")]) - with ctx.manager("update-status", state) as mgr: - charm = mgr.charm - assert charm.peers - charm.set_peer_data("1", 2) - assert charm.get_peer_data("1") == 2 +def test_placeholder(ctx): + pass diff --git a/tox.ini b/tox.ini index ed2129d5..b2638b2d 100644 --- a/tox.ini +++ b/tox.ini @@ -87,7 +87,7 @@ deps = pytest<8.2.0 # https://github.com/pytest-dev/pytest/issues/12263 responses cosl - ops-scenario<7.0.0 + ops[testing] -r{toxinidir}/requirements.txt commands = /usr/bin/env sh -c 'stat sqlite-static > /dev/null 2>&1 || curl -L https://github.com/CompuRoot/static-sqlite3/releases/latest/download/sqlite3 -o sqlite-static && chmod +x sqlite-static' From d2133ae271eb8c50c6eb84f10ee036b25a79e5ab Mon Sep 17 00:00:00 2001 From: sed-i <82407168+sed-i@users.noreply.github.com> Date: Tue, 12 Nov 2024 14:31:07 -0500 Subject: [PATCH 04/13] Add scenario tests --- tests/scenario/test_charm.py | 43 ++++++++++++++++++++++++++++++++---- 1 file changed, 39 insertions(+), 4 deletions(-) diff --git a/tests/scenario/test_charm.py b/tests/scenario/test_charm.py index 4fad0653..c0a092bb 100644 --- a/tests/scenario/test_charm.py +++ b/tests/scenario/test_charm.py @@ -1,5 +1,40 @@ -import pytest -from ops.testing import State, PeerRelation +from ops.testing import State +from configparser import ConfigParser -def test_placeholder(ctx): - pass + +def test_reporting_enabled(ctx): + # GIVEN the "reporting_enabled" config option is set to True + state = State(leader=True, config={"reporting_enabled": True}) + # WHEN config-changed fires + out = ctx.run(ctx.on.config_changed(), state) + + # THEN the config file is written WITHOUT the [analytics] section being rendered + simulated_pebble_filesystem = out.get_container("grafana").get_filesystem(ctx) + grafana_config_path = simulated_pebble_filesystem / "etc/grafana/grafana-config.ini" + + config = ConfigParser() + config.read(grafana_config_path) + assert "analytics" not in config + + +def test_reporting_disabled(ctx): + # GIVEN the "reporting_enabled" config option is set to True + state = State(leader=True, config={"reporting_enabled": False}) + # WHEN config-changed fires + out = ctx.run(ctx.on.config_changed(), state) + + # THEN the config file is written WITH the [analytics] section being rendered + simulated_pebble_filesystem = out.get_container("grafana").get_filesystem(ctx) + grafana_config_path = simulated_pebble_filesystem / "etc/grafana/grafana-config.ini" + + config = ConfigParser() + config.read(grafana_config_path) + assert dict(config["analytics"]) == { + 'reporting_enabled': 'false', + 'check_for_updates': 'false', + 'check_for_plugin_updates': 'false', + } + + # AND the "grafana" service is restarted + # TODO Does it make sense to check this if the charm under test's lifetime is only for the config-changed? + # TODO How to assert this? \ No newline at end of file From b0faa94efa299a012ee95287f13318c8152b7f22 Mon Sep 17 00:00:00 2001 From: sed-i <82407168+sed-i@users.noreply.github.com> Date: Tue, 12 Nov 2024 14:48:56 -0500 Subject: [PATCH 05/13] Fixes --- tests/scenario/test_charm.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/tests/scenario/test_charm.py b/tests/scenario/test_charm.py index c0a092bb..e6edded3 100644 --- a/tests/scenario/test_charm.py +++ b/tests/scenario/test_charm.py @@ -1,4 +1,4 @@ -from ops.testing import State +from ops.testing import State, Container from configparser import ConfigParser @@ -18,8 +18,12 @@ def test_reporting_enabled(ctx): def test_reporting_disabled(ctx): - # GIVEN the "reporting_enabled" config option is set to True - state = State(leader=True, config={"reporting_enabled": False}) + # GIVEN the "reporting_enabled" config option is set to False + state = State( + leader=True, + config={"reporting_enabled": False}, + containers=[Container(name="grafana")] + ) # WHEN config-changed fires out = ctx.run(ctx.on.config_changed(), state) From 5fde8fc968eb3951704b33571d46b5876cedfe40 Mon Sep 17 00:00:00 2001 From: sed-i <82407168+sed-i@users.noreply.github.com> Date: Tue, 12 Nov 2024 14:50:14 -0500 Subject: [PATCH 06/13] Fixes --- tests/scenario/test_charm.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/tests/scenario/test_charm.py b/tests/scenario/test_charm.py index e6edded3..273d4709 100644 --- a/tests/scenario/test_charm.py +++ b/tests/scenario/test_charm.py @@ -4,8 +4,11 @@ def test_reporting_enabled(ctx): # GIVEN the "reporting_enabled" config option is set to True - state = State(leader=True, config={"reporting_enabled": True}) - # WHEN config-changed fires + state = State( + leader=True, + config={"reporting_enabled": True}, + containers=[Container(name="grafana"), Container(name="litestream")] + ) # WHEN config-changed fires out = ctx.run(ctx.on.config_changed(), state) # THEN the config file is written WITHOUT the [analytics] section being rendered @@ -22,7 +25,7 @@ def test_reporting_disabled(ctx): state = State( leader=True, config={"reporting_enabled": False}, - containers=[Container(name="grafana")] + containers=[Container(name="grafana"), Container(name="litestream")] ) # WHEN config-changed fires out = ctx.run(ctx.on.config_changed(), state) From a551f864c0e475e76a77140d86694360ae6d068c Mon Sep 17 00:00:00 2001 From: sed-i <82407168+sed-i@users.noreply.github.com> Date: Tue, 12 Nov 2024 14:51:22 -0500 Subject: [PATCH 07/13] Fixes --- tests/scenario/test_charm.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/scenario/test_charm.py b/tests/scenario/test_charm.py index 273d4709..3d93eb18 100644 --- a/tests/scenario/test_charm.py +++ b/tests/scenario/test_charm.py @@ -36,6 +36,7 @@ def test_reporting_disabled(ctx): config = ConfigParser() config.read(grafana_config_path) + assert "analytics" in config assert dict(config["analytics"]) == { 'reporting_enabled': 'false', 'check_for_updates': 'false', From 40995a903b8fdc7c450d9cef10478ce598d43537 Mon Sep 17 00:00:00 2001 From: sed-i <82407168+sed-i@users.noreply.github.com> Date: Tue, 12 Nov 2024 14:52:54 -0500 Subject: [PATCH 08/13] Fixes --- tests/scenario/test_charm.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/scenario/test_charm.py b/tests/scenario/test_charm.py index 3d93eb18..59c12088 100644 --- a/tests/scenario/test_charm.py +++ b/tests/scenario/test_charm.py @@ -2,12 +2,15 @@ from configparser import ConfigParser +containers = [Container(name="grafana", can_connect=True), Container(name="litestream", can_connect=True)] + + def test_reporting_enabled(ctx): # GIVEN the "reporting_enabled" config option is set to True state = State( leader=True, config={"reporting_enabled": True}, - containers=[Container(name="grafana"), Container(name="litestream")] + containers=containers ) # WHEN config-changed fires out = ctx.run(ctx.on.config_changed(), state) @@ -25,7 +28,7 @@ def test_reporting_disabled(ctx): state = State( leader=True, config={"reporting_enabled": False}, - containers=[Container(name="grafana"), Container(name="litestream")] + containers=containers ) # WHEN config-changed fires out = ctx.run(ctx.on.config_changed(), state) From dae8220c286981ed2219b3afb7292c5294370858 Mon Sep 17 00:00:00 2001 From: sed-i <82407168+sed-i@users.noreply.github.com> Date: Tue, 12 Nov 2024 14:57:06 -0500 Subject: [PATCH 09/13] fmt --- tests/scenario/test_charm.py | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/tests/scenario/test_charm.py b/tests/scenario/test_charm.py index 59c12088..b2e6fc5b 100644 --- a/tests/scenario/test_charm.py +++ b/tests/scenario/test_charm.py @@ -2,16 +2,17 @@ from configparser import ConfigParser -containers = [Container(name="grafana", can_connect=True), Container(name="litestream", can_connect=True)] +containers = [ + Container(name="grafana", can_connect=True), + Container(name="litestream", can_connect=True), +] def test_reporting_enabled(ctx): # GIVEN the "reporting_enabled" config option is set to True state = State( - leader=True, - config={"reporting_enabled": True}, - containers=containers - ) # WHEN config-changed fires + leader=True, config={"reporting_enabled": True}, containers=containers + ) # WHEN config-changed fires out = ctx.run(ctx.on.config_changed(), state) # THEN the config file is written WITHOUT the [analytics] section being rendered @@ -25,11 +26,7 @@ def test_reporting_enabled(ctx): def test_reporting_disabled(ctx): # GIVEN the "reporting_enabled" config option is set to False - state = State( - leader=True, - config={"reporting_enabled": False}, - containers=containers - ) + state = State(leader=True, config={"reporting_enabled": False}, containers=containers) # WHEN config-changed fires out = ctx.run(ctx.on.config_changed(), state) @@ -41,11 +38,11 @@ def test_reporting_disabled(ctx): config.read(grafana_config_path) assert "analytics" in config assert dict(config["analytics"]) == { - 'reporting_enabled': 'false', - 'check_for_updates': 'false', - 'check_for_plugin_updates': 'false', + "reporting_enabled": "false", + "check_for_updates": "false", + "check_for_plugin_updates": "false", } # AND the "grafana" service is restarted # TODO Does it make sense to check this if the charm under test's lifetime is only for the config-changed? - # TODO How to assert this? \ No newline at end of file + # TODO How to assert this? From 76636e661ffe984fb8930ed394eac8c99564f6cb Mon Sep 17 00:00:00 2001 From: sed-i <82407168+sed-i@users.noreply.github.com> Date: Tue, 12 Nov 2024 15:31:10 -0500 Subject: [PATCH 10/13] Rename test --- .../scenario/{test_charm.py => test_config_reporting_enabled.py} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename tests/scenario/{test_charm.py => test_config_reporting_enabled.py} (100%) diff --git a/tests/scenario/test_charm.py b/tests/scenario/test_config_reporting_enabled.py similarity index 100% rename from tests/scenario/test_charm.py rename to tests/scenario/test_config_reporting_enabled.py From fde803e86c892e09dbea78ac1a4cc7e8705d6a0b Mon Sep 17 00:00:00 2001 From: sed-i <82407168+sed-i@users.noreply.github.com> Date: Tue, 12 Nov 2024 15:34:40 -0500 Subject: [PATCH 11/13] Fix the fmt --- tests/scenario/test_config_reporting_enabled.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/scenario/test_config_reporting_enabled.py b/tests/scenario/test_config_reporting_enabled.py index b2e6fc5b..345c8b63 100644 --- a/tests/scenario/test_config_reporting_enabled.py +++ b/tests/scenario/test_config_reporting_enabled.py @@ -12,7 +12,9 @@ def test_reporting_enabled(ctx): # GIVEN the "reporting_enabled" config option is set to True state = State( leader=True, config={"reporting_enabled": True}, containers=containers - ) # WHEN config-changed fires + ) + + # WHEN config-changed fires out = ctx.run(ctx.on.config_changed(), state) # THEN the config file is written WITHOUT the [analytics] section being rendered @@ -27,6 +29,7 @@ def test_reporting_enabled(ctx): def test_reporting_disabled(ctx): # GIVEN the "reporting_enabled" config option is set to False state = State(leader=True, config={"reporting_enabled": False}, containers=containers) + # WHEN config-changed fires out = ctx.run(ctx.on.config_changed(), state) From 06ed0df6cc5e2206cf2574d3fba8a384090d6a6f Mon Sep 17 00:00:00 2001 From: sed-i <82407168+sed-i@users.noreply.github.com> Date: Tue, 12 Nov 2024 16:37:35 -0500 Subject: [PATCH 12/13] fmt --- tests/scenario/test_config_reporting_enabled.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/scenario/test_config_reporting_enabled.py b/tests/scenario/test_config_reporting_enabled.py index 345c8b63..14b4edb5 100644 --- a/tests/scenario/test_config_reporting_enabled.py +++ b/tests/scenario/test_config_reporting_enabled.py @@ -10,9 +10,7 @@ def test_reporting_enabled(ctx): # GIVEN the "reporting_enabled" config option is set to True - state = State( - leader=True, config={"reporting_enabled": True}, containers=containers - ) + state = State(leader=True, config={"reporting_enabled": True}, containers=containers) # WHEN config-changed fires out = ctx.run(ctx.on.config_changed(), state) From 4dc761d7db9062d6dac840d02dbc6b7190e8ea01 Mon Sep 17 00:00:00 2001 From: Leon <82407168+sed-i@users.noreply.github.com> Date: Wed, 13 Nov 2024 23:54:17 -0500 Subject: [PATCH 13/13] Pin websockets Signed-off-by: Leon <82407168+sed-i@users.noreply.github.com> --- tox.ini | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tox.ini b/tox.ini index b2638b2d..b28352ce 100644 --- a/tox.ini +++ b/tox.ini @@ -105,6 +105,8 @@ deps = asyncstdlib # Libjuju needs to track the juju version juju<=3.3.0,>=3.0 + # https://github.com/juju/python-libjuju/issues/1184 + websockets<14.0 pytest<8.2.0 # https://github.com/pytest-dev/pytest/issues/12263 pytest-operator pytest-playwright