From d3f503ab75e3a5e67da8e70e5738bdb4a4e7735a Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Tue, 14 Jul 2026 11:33:32 +0000 Subject: [PATCH 1/3] test(garm): refactor unit tests to the scenario framework The charm-behaviour tests faked `self` with a MagicMock and called unbound methods (`GarmCharm.restart(charm)`), which could only assert on the arguments the charm passed to its collaborators, never on what it actually did. Drive the real charm through Scenario instead and assert on the resulting State: the Juju secrets created, the files pushed, the Pebble plan left behind, and the status reported. Only GARM's HTTP surface is stubbed. ops-scenario 8.8.0 expands the go-framework extension itself, so the charm needs no packing for its container, peer relation and config options to resolve. Move the pure config-rendering and secret-generation helper tests to test_garm_toml.py, leaving test_charm.py purely Scenario-driven. charm.py statement coverage rises from 67% to 89%: Scenario exercises the relation-parsing and config-writing paths the mock-based tests never reached. --- charms/garm/AGENTS.md | 13 + charms/garm/tests/unit/test_charm.py | 1810 ++++++++-------------- charms/garm/tests/unit/test_garm_toml.py | 439 ++++++ charms/garm/tox.toml | 8 +- 4 files changed, 1128 insertions(+), 1142 deletions(-) create mode 100644 charms/garm/tests/unit/test_garm_toml.py diff --git a/charms/garm/AGENTS.md b/charms/garm/AGENTS.md index 54447bfa..5fa5dbf0 100644 --- a/charms/garm/AGENTS.md +++ b/charms/garm/AGENTS.md @@ -35,3 +35,16 @@ charm conventions; this file lists only what's specific to `garm`. rather than blocking). The port is pinned in the `_workload_config` property. - Tests: unit in `tests/unit/`; integration via `tox -e garm-integration` (`charms/tests/integration/test_garm.py`). +- **Scenario tests of this paas charm need three fixtures** that a plain `ops` charm doesn't + (all provided by the state builder in `tests/unit/test_charm.py`) — `ops-scenario` expands the + `go-framework` extension itself, so the `app` container and `secret-storage` peer exist + without a packed charm: + - **DO** give the `app` container a base layer declaring the rock's `go` service; the + framework's layer overrides that service and can't find it otherwise. + - **DO** put `go_secret_key` in the `secret-storage` peer app databag — `is_ready()` gates + on it and reports `Waiting for peer integration` without it. + - **DO** publish postgresql connection data; `postgresql` is `optional: false`, so the + framework blocks with `missing integrations: postgresql` before `restart()` runs. + - **DON'T** rebuild the mock-`self` pattern (`GarmCharm.restart(MagicMock())`) these tests + replaced: it can't see what the charm pushed or replanned, so it hid real bugs. Stub only + GARM's HTTP surface (the `garm_api` fixture) and assert on the output `State`. diff --git a/charms/garm/tests/unit/test_charm.py b/charms/garm/tests/unit/test_charm.py index 62c75c02..cdd85275 100644 --- a/charms/garm/tests/unit/test_charm.py +++ b/charms/garm/tests/unit/test_charm.py @@ -1,15 +1,27 @@ # Copyright 2026 Canonical Ltd. # See LICENSE file for licensing details. -"""Unit tests for GarmCharm.""" +"""Unit tests for GarmCharm, driven through Scenario. +Each test runs the real charm against a State and asserts on the resulting State: the Juju +secrets it created, the files it pushed, the Pebble plan it left behind, and the status it +reported. Only GARM's HTTP surface is stubbed (the ``garm_api`` fixture) — no GARM process +listens in a unit test — so the charm's own logic always runs for real. + +The pure config-rendering helpers in charm.py are tested in test_garm_toml.py. +""" + +import dataclasses import os -import string +import typing from unittest.mock import MagicMock, PropertyMock, patch import ops import pytest import yaml +from ops import pebble +from scenario import Container, Context, Model, PeerRelation, Relation, Secret, State +from scenario.errors import UncaughtCharmError try: import tomllib @@ -17,1360 +29,876 @@ import tomli as tomllib # type: ignore[no-redef] import garm_template -from charm import ( - GARM_ADMIN_CREDENTIALS_LABEL, - GARM_PORT, - GARM_SECRETS_LABEL, - GarmCharm, - _build_provider_list, - _generate_admin_password, - _generate_garm_secrets, - _proxy_environment, - render_garm_toml, +from charm import GARM_ADMIN_CREDENTIALS_LABEL, GARM_PORT, GARM_SECRETS_LABEL, GarmCharm +from charm_state import DEBUG_SSH_INTEGRATION_NAME, GARM_CONFIGURATOR_RELATION_NAME +from garm_api import GarmConnectionError +from github_reconciler import ( + DEFAULT_GITHUB_ENDPOINT, + MANAGED_CREDENTIAL_DESCRIPTION, + CredentialSpec, ) -from garm_api import GarmApiError, GarmConnectionError -from github_reconciler import DEFAULT_GITHUB_ENDPOINT -from scaleset_reconciler import ScalesetSpec - -_DEFAULT_PG_CONFIG = { - "username": "u", - "password": "p", - "hostname": "h", - "port": 5432, - "database": "d", - "sslmode": "disable", -} +MODEL_NAME = "garm-model" +CONTAINER_NAME = "app" +SERVICE_NAME = "app" +WORKLOAD_LAYER_NAME = "garm-command" -def _render(**overrides) -> dict: - """Helper: render TOML with defaults, return parsed dict.""" - kwargs = { - "jwt_secret": "test-secret", - "db_passphrase": "a" * 32, - "postgresql_config": _DEFAULT_PG_CONFIG, +# The rock ships a `go` service that the go-framework's layer overrides; without it in the +# container's base layer paas_charm's restart() fails to find the service to replace. +_ROCK_LAYER = pebble.Layer( + { + "services": { + "go": { + "override": "replace", + "summary": "GARM GitHub Actions Runner Manager", + "startup": "disabled", + "command": "/usr/local/bin/garm", + } + } } - kwargs.update(overrides) - toml_content, _ = render_garm_toml(**kwargs) - return tomllib.loads(toml_content) - - -def test_render_garm_toml_postgresql_backend(): - """The [database] section uses postgresql backend with correct fields.""" - parsed = _render( - postgresql_config={ - "username": "garm", - "password": "secret", - "hostname": "10.0.0.5", - "port": 5432, - "database": "garm_db", - "sslmode": "require", - }, - ) - assert parsed["database"]["backend"] == "postgresql" - assert parsed["database"]["postgresql"]["hostname"] == "10.0.0.5" - assert parsed["database"]["postgresql"]["username"] == "garm" - assert parsed["database"]["postgresql"]["password"] == "secret" - assert parsed["database"]["postgresql"]["port"] == 5432 - assert parsed["database"]["postgresql"]["database"] == "garm_db" - assert parsed["database"]["postgresql"]["sslmode"] == "require" - assert "sqlite3" not in parsed["database"] - - -def test_render_garm_toml_passphrase_in_database_section(): - """The passphrase appears in the [database] section.""" - passphrase = "b" * 32 - parsed = _render(db_passphrase=passphrase) - assert parsed["database"]["passphrase"] == passphrase - - -@pytest.mark.parametrize( - "sslmode", ["disable", "allow", "prefer", "require", "verify-ca", "verify-full"] -) -def test_render_garm_toml_sslmode_propagated(sslmode: str): - """The sslmode value is propagated to the postgresql section.""" - pg_config = {**_DEFAULT_PG_CONFIG, "sslmode": sslmode} - parsed = _render(postgresql_config=pg_config) - assert parsed["database"]["postgresql"]["sslmode"] == sslmode - - -@pytest.mark.parametrize( - "section,key,value,kwargs", - [ - ("apiserver", "bind", "0.0.0.0", {}), - ("apiserver", "port", 8080, {}), - ("apiserver", "use_tls", False, {}), - ("jwt_auth", "secret", "mysecret", {"jwt_secret": "mysecret"}), - ("jwt_auth", "time_to_live", "8760h", {}), - ("metrics", "disable_auth", True, {}), - ("metrics", "enable", True, {}), - ], - ids=[ - "apiserver-bind", - "apiserver-port", - "apiserver-use_tls", - "jwt_auth-secret", - "jwt_auth-time_to_live", - "metrics-disable_auth", - "metrics-enable", - ], ) -def test_render_garm_toml_section_fields(section: str, key: str, value, kwargs: dict): - """Config sections reflect the given parameters.""" - parsed = _render(**kwargs) - assert parsed[section][key] == value - - -def test_render_garm_toml_provider_section(): - """The [[provider]] section has the OpenStack provider binary.""" - parsed = _render() - assert len(parsed["provider"]) == 1 - provider = parsed["provider"][0] - assert provider["name"] == "openstack" - assert provider["provider_type"] == "external" - assert provider["external"]["provider_executable"] == "/usr/local/bin/garm-provider-openstack" - - -def test_generate_garm_secrets_returns_jwt_and_passphrase(): - """Returns a dict with jwt-secret (64-char hex) and db-passphrase (32-char alnum).""" - result = _generate_garm_secrets() - assert "jwt-secret" in result - assert "db-passphrase" in result - assert len(result["jwt-secret"]) == 64 - assert all(c in "0123456789abcdef" for c in result["jwt-secret"]) - assert len(result["db-passphrase"]) == 32 - valid_chars = string.ascii_letters + string.digits - assert all(c in valid_chars for c in result["db-passphrase"]) - - -def test_generate_garm_secrets_produces_unique_values(): - """Two calls return different secrets.""" - first = _generate_garm_secrets() - second = _generate_garm_secrets() - assert first["jwt-secret"] != second["jwt-secret"] - assert first["db-passphrase"] != second["db-passphrase"] - - -def test_render_garm_toml_with_configurator_providers(): - """ - arrange: Two Configurator units provided provider configs. - act: render_garm_toml is called with providers list. - assert: Two [[provider]] blocks are rendered with correct names and - config_file paths pointing to provider TOML files. The - returned provider_files dict contains the provider TOML and - clouds.yaml for each provider. - """ - providers = [ - { - "unit_name": "garm-configurator-0", - "auth_url": "https://ks1.example.com:5000/v3", - "username": "admin1", - "password": "pass1", - "project_name": "proj1", - "user_domain_name": "Default", - "project_domain_name": "Default", - "region_name": "RegionOne", - "network": "net1", - }, - { - "unit_name": "garm-configurator-1", - "auth_url": "https://ks2.example.com:5000/v3", - "username": "admin2", - "password": "pass2", - "project_name": "proj2", - "user_domain_name": "Default", - "project_domain_name": "Default", - "region_name": "RegionTwo", - "network": "net2", - }, - ] - toml_content, provider_files = render_garm_toml( - jwt_secret="test-secret", - db_passphrase="a" * 32, - postgresql_config=_DEFAULT_PG_CONFIG, - providers=providers, - ) - parsed = tomllib.loads(toml_content) - assert len(parsed["provider"]) == 2 - - p0 = parsed["provider"][0] - assert p0["name"] == "garm-configurator-0" - assert p0["external"]["provider_executable"] == "/usr/local/bin/garm-provider-openstack" - assert p0["external"]["config_file"] == "/etc/garm/provider-garm-configurator-0.toml" - assert "environment_variables" not in p0["external"] - - p1 = parsed["provider"][1] - assert p1["name"] == "garm-configurator-1" - assert p1["external"]["config_file"] == "/etc/garm/provider-garm-configurator-1.toml" - - # Verify provider_files contains the expected paths and content. - provider_toml_0 = provider_files["/etc/garm/provider-garm-configurator-0.toml"] - assert 'network_id = "net1"' in provider_toml_0 - assert 'cloud = "garm-configurator-0"' in provider_toml_0 - - clouds_yaml_0 = provider_files["/etc/garm/clouds-garm-configurator-0.yaml"] - clouds_0 = yaml.safe_load(clouds_yaml_0) - assert clouds_0["clouds"]["garm-configurator-0"]["auth"]["username"] == "admin1" - assert clouds_0["clouds"]["garm-configurator-0"]["auth"]["password"] == "pass1" - assert clouds_0["clouds"]["garm-configurator-0"]["region_name"] == "RegionOne" - - clouds_yaml_1 = provider_files["/etc/garm/clouds-garm-configurator-1.yaml"] - clouds_1 = yaml.safe_load(clouds_yaml_1) - assert clouds_1["clouds"]["garm-configurator-1"]["auth"]["username"] == "admin2" - assert clouds_1["clouds"]["garm-configurator-1"]["auth"]["password"] == "pass2" - - -def test_build_provider_list_returns_default_when_empty(): - """ - arrange: An empty list is passed. - act: _build_provider_list is called. - assert: Returns the default single-entry list with "openstack" provider. - """ - entries, files = _build_provider_list([]) - assert len(entries) == 1 - assert entries[0]["name"] == "openstack" - assert files == {} - - -def test_build_provider_list_password_in_clouds_yaml(): - """ - arrange: Two provider configs with passwords. - act: _build_provider_list is called. - assert: Both providers have their password rendered in clouds.yaml. - The password is resolved from Juju secrets in - _get_configurator_provider_configs before reaching - _build_provider_list, so it's always available as plaintext. - """ - providers = [ - { - "unit_name": "garm-configurator-0", - "auth_url": "https://ks1.example.com:5000/v3", - "username": "admin1", - "password": "pass1", - "project_name": "proj1", - "user_domain_name": "Default", - "project_domain_name": "Default", - "region_name": "RegionOne", - "network": "net1", - }, - { - "unit_name": "garm-configurator-1", - "auth_url": "https://ks2.example.com:5000/v3", - "username": "admin2", - "password": "pass2", - "project_name": "proj2", - "user_domain_name": "Default", - "project_domain_name": "Default", - "region_name": "RegionTwo", - "network": "net2", - }, - ] - entries, provider_files = _build_provider_list(providers) - assert len(entries) == 2 - - clouds_yaml_0 = provider_files["/etc/garm/clouds-garm-configurator-0.yaml"] - clouds_0 = yaml.safe_load(clouds_yaml_0) - assert clouds_0["clouds"]["garm-configurator-0"]["auth"]["password"] == "pass1" - assert clouds_0["clouds"]["garm-configurator-0"]["auth"]["username"] == "admin1" - assert clouds_0["clouds"]["garm-configurator-0"]["region_name"] == "RegionOne" - - clouds_yaml_1 = provider_files["/etc/garm/clouds-garm-configurator-1.yaml"] - clouds_1 = yaml.safe_load(clouds_yaml_1) - assert clouds_1["clouds"]["garm-configurator-1"]["auth"]["password"] == "pass2" - - assert entries[0]["external"]["config_file"] == "/etc/garm/provider-garm-configurator-0.toml" - assert entries[1]["external"]["config_file"] == "/etc/garm/provider-garm-configurator-1.toml" - - -def test_render_clouds_yaml_quotes_special_chars(): - """ - arrange: A password containing YAML-significant characters (colon, hash). - act: _render_clouds_yaml is called (via _build_provider_list). - assert: The resulting clouds.yaml is valid YAML and the password - parses correctly (not truncated or misinterpreted). - """ - providers = [ - { - "unit_name": "special-provider", - "auth_url": "https://keystone.example.com:5000/v3", - "username": "admin", - "password": "p@ss:w0rd#123", - "project_name": "proj", - "user_domain_name": "Default", - "project_domain_name": "Default", - "region_name": "RegionOne", - "network": "net1", - }, - ] - _, provider_files = _build_provider_list(providers) - clouds_yaml = provider_files["/etc/garm/clouds-special-provider.yaml"] - parsed = yaml.safe_load(clouds_yaml) - assert parsed["clouds"]["special-provider"]["auth"]["password"] == "p@ss:w0rd#123" +# The key paas_charm's KeySecretStorage keeps in the peer databag; is_ready() gates on it. +_SECRET_STORAGE_KEY = "go_secret_key" -def test_generate_admin_password_meets_garm_policy(): - """ - arrange: No setup required. - act: Call _generate_admin_password(). - assert: The result satisfies GARM's strong-password requirements. - """ - password = _generate_admin_password() - assert len(password) >= 12 - assert any(c.isupper() for c in password) - assert any(c.islower() for c in password) - assert any(c.isdigit() for c in password) - symbols = set(password) - set(string.ascii_letters + string.digits) - assert len(symbols) > 0 - - -def test_generate_admin_password_produces_unique_values(): - """ - arrange: No setup required. - act: Call _generate_admin_password() twice. - assert: The two passwords are different. - """ - assert _generate_admin_password() != _generate_admin_password() - - -_MOCK_ADMIN_CREDS = { +_GARM_SECRETS_CONTENT = {"jwt-secret": "test-jwt-secret", "db-passphrase": "a" * 32} +_ADMIN_CREDENTIALS_CONTENT = { "username": "admin", "password": "TestPass-123!", "email": "admin@garm.local", "full-name": "GARM Admin", } +_POSTGRESQL_DATA = { + "endpoints": "10.0.0.5:5432", + "username": "garm", + "password": "pw", + "database": "garm", +} -def test_get_admin_credentials_returns_content_when_secret_exists(): - """ - arrange: garm-admin-credentials secret exists in Juju. - act: Call _get_admin_credentials(). - assert: Returns the secret content dict. - """ - charm = MagicMock() - mock_secret = MagicMock() - mock_secret.get_content.return_value = _MOCK_ADMIN_CREDS - charm.model.get_secret.return_value = mock_secret - - result = GarmCharm._get_admin_credentials(charm) - - assert result == _MOCK_ADMIN_CREDS - - -def test_get_admin_credentials_returns_none_when_secret_not_found(): - """ - arrange: garm-admin-credentials secret does not exist in Juju. - act: Call _get_admin_credentials(). - assert: Returns None. - """ - charm = MagicMock() - charm.model.get_secret.side_effect = ops.SecretNotFoundError("not found") - - result = GarmCharm._get_admin_credentials(charm) +_PROVIDER_UNIT_DATA = { + "openstack_auth_url": "https://ks1.example.com:5000/v3", + "openstack_username": "admin1", + "openstack_password": "pass1", + "openstack_project_name": "proj1", + "openstack_user_domain_name": "Default", + "openstack_project_domain_name": "Default", + "openstack_region_name": "RegionOne", + "openstack_network": "net1", +} - assert result is None +_SCALESET_UNIT_DATA = { + "name": "my-scaleset", + "provider_name": "garm-configurator-0", + "image_id": "img-1", + "flavor": "m1.large", + "os_arch": "amd64", + "max_runner": "5", + "min_idle_runner": "1", + "repo": "myorg/myrepo", +} +_TEMPLATE_ID = 99 -def test_ensure_secrets_skips_when_not_leader(): - """ - arrange: Unit is not the Juju leader. - act: Call _ensure_secrets(). - assert: No secrets are created. - """ - charm = MagicMock() - charm.unit.is_leader.return_value = False - GarmCharm._ensure_secrets(charm) +@pytest.fixture(name="ctx") +def ctx_fixture() -> Context: + return Context(GarmCharm) - charm.app.add_secret.assert_not_called() +@dataclasses.dataclass +class _GarmApiMocks: + """The stubbed GARM HTTP surface. -def test_ensure_secrets_creates_secrets_on_first_run(): - """ - arrange: Leader unit; neither garm-secrets nor garm-admin-credentials exist. - act: Call _ensure_secrets(). - assert: Both secrets are created and labelled correctly. + Attributes: + client: charm.GarmApiClient — the unauthenticated client used for first-run. + auth: charm.GarmAuthenticatedClient. + auth_client: The authenticated client instance every reconciler is built against. + github: charm.GithubReconciler. + entity: charm.EntityReconciler. + scaleset: charm.ScalesetReconciler. + apply_template: charm._apply_garm_template, returning _TEMPLATE_ID. + calls: Parent mock recording the reconcile steps in the order they run. """ - charm = MagicMock() - charm.unit.is_leader.return_value = True - charm.model.get_secret.side_effect = ops.SecretNotFoundError("not found") - - GarmCharm._ensure_secrets(charm) - - assert charm.app.add_secret.call_count == 2 - labels = {c.kwargs["label"] for c in charm.app.add_secret.call_args_list} - assert GARM_SECRETS_LABEL in labels - assert GARM_ADMIN_CREDENTIALS_LABEL in labels + client: MagicMock + auth: MagicMock + auth_client: MagicMock + github: MagicMock + entity: MagicMock + scaleset: MagicMock + apply_template: MagicMock + calls: MagicMock -def test_ensure_secrets_skips_creation_when_secrets_exist(): - """ - arrange: Leader unit; both secrets already exist in Juju. - act: Call _ensure_secrets(). - assert: No secrets are created. - """ - charm = MagicMock() - charm.unit.is_leader.return_value = True - - GarmCharm._ensure_secrets(charm) - charm.app.add_secret.assert_not_called() +@pytest.fixture(name="garm_api") +def garm_api_fixture() -> typing.Iterator[_GarmApiMocks]: + """Stub GARM's HTTP surface, recording the reconcile steps in call order.""" + with ( + patch("charm.GarmApiClient") as client_cls, + patch("charm.GarmAuthenticatedClient") as auth_cls, + patch("charm.GithubReconciler") as github_cls, + patch("charm.EntityReconciler") as entity_cls, + patch("charm.ScalesetReconciler") as scaleset_cls, + patch("charm._apply_garm_template", return_value=_TEMPLATE_ID) as apply_template, + ): + auth_client = auth_cls.from_login.return_value + calls = MagicMock() + calls.attach_mock(auth_client.update_controller, "controller") + calls.attach_mock(github_cls.return_value.reconcile, "github") + calls.attach_mock(entity_cls.return_value.reconcile, "entity") + calls.attach_mock(scaleset_cls.return_value.reconcile, "scaleset") + yield _GarmApiMocks( + client=client_cls, + auth=auth_cls, + auth_client=auth_client, + github=github_cls, + entity=entity_cls, + scaleset=scaleset_cls, + apply_template=apply_template, + calls=calls, + ) -def test_maybe_first_run_skips_when_not_leader(): - """ - arrange: Unit is not the Juju leader. - act: Call _maybe_first_run(). - assert: No GARM API call is made. - """ - charm = MagicMock() - charm.unit.is_leader.return_value = False +def _owned_secrets(admin_content: dict | None = None) -> list[Secret]: + """The two labelled secrets a leader creates, as they exist on later reconciles.""" + return [ + Secret(label=GARM_SECRETS_LABEL, owner="app", tracked_content=_GARM_SECRETS_CONTENT), + Secret( + label=GARM_ADMIN_CREDENTIALS_LABEL, + owner="app", + tracked_content=_ADMIN_CREDENTIALS_CONTENT if admin_content is None else admin_content, + ), + ] - with patch("charm.GarmApiClient") as mock_client_cls: - GarmCharm._maybe_first_run(charm) - mock_client_cls.assert_not_called() +def _state( + *, + leader: bool = True, + can_connect: bool = True, + postgresql_data: dict | None = None, + configurator_related: bool = True, + configurator_units_data: dict[int, dict] | None = None, + debug_ssh_related: bool = False, + secrets: typing.Sequence[Secret] = (), + unit_status: ops.StatusBase | None = None, + app_status: ops.StatusBase | None = None, +) -> State: + """Build a GARM charm State that is ready to serve unless a caller opts out. + + Defaults to a leader whose workload is reachable, whose postgresql relation carries + connection data, and whose single configurator unit publishes a full OpenStack provider + and scaleset spec — the state in which restart() runs end to end. + """ + relations: list[Relation | PeerRelation] = [ + Relation( + endpoint="postgresql", + remote_app_name="postgresql", + remote_app_data=_POSTGRESQL_DATA if postgresql_data is None else postgresql_data, + ), + PeerRelation( + endpoint="secret-storage", local_app_data={_SECRET_STORAGE_KEY: "peer-secret-key"} + ), + ] + if configurator_related: + relations.append( + Relation( + endpoint=GARM_CONFIGURATOR_RELATION_NAME, + remote_app_name="garm-configurator", + remote_units_data=( + {0: {**_PROVIDER_UNIT_DATA, **_SCALESET_UNIT_DATA}} + if configurator_units_data is None + else configurator_units_data + ), + ) + ) + if debug_ssh_related: + relations.append( + Relation( + endpoint=DEBUG_SSH_INTEGRATION_NAME, + remote_app_name="tmate-ssh-server", + remote_units_data={0: {}}, + ) + ) + statuses = {} + if unit_status is not None: + statuses["unit_status"] = unit_status + if app_status is not None: + statuses["app_status"] = app_status + return State( + leader=leader, + model=Model(name=MODEL_NAME), + containers=[ + Container(name=CONTAINER_NAME, can_connect=can_connect, layers={"garm": _ROCK_LAYER}) + ], + relations=relations, + secrets=list(secrets), + **statuses, + ) -def test_maybe_first_run_skips_when_credentials_unavailable(): - """ - arrange: Leader unit; admin credentials secret does not exist yet. - act: Call _maybe_first_run(). - assert: No GARM API call is made. - """ - charm = MagicMock() - charm.unit.is_leader.return_value = True - charm._get_admin_credentials.return_value = None +def _secret_content(state: State, label: str) -> dict[str, str]: + """Return the content of the state's secret with the given label.""" + return next(secret for secret in state.secrets if secret.label == label).tracked_content - with patch("charm.GarmApiClient") as mock_client_cls: - GarmCharm._maybe_first_run(charm) - mock_client_cls.assert_not_called() +def _service_environment(state: State) -> dict[str, str]: + """Return the app service's environment from the state's effective Pebble plan.""" + return state.get_container(CONTAINER_NAME).plan.services[SERVICE_NAME].environment -def test_maybe_first_run_skips_when_already_initialized(): - """ - arrange: Leader unit with valid credentials; GarmApiClient.is_initialized returns True. - act: Call _maybe_first_run(). - assert: first_run is not called on the client. - """ - charm = MagicMock() - charm.unit.is_leader.return_value = True - charm._get_admin_credentials.return_value = _MOCK_ADMIN_CREDS +def _workload_layer_environment(state: State) -> dict[str, str]: + """Return the app service's environment from the layer the charm itself writes.""" + layer = state.get_container(CONTAINER_NAME).layers[WORKLOAD_LAYER_NAME] + return layer.services[SERVICE_NAME].environment - with patch("charm.GarmApiClient") as mock_client_cls: - mock_client_cls.return_value.is_initialized.return_value = True - GarmCharm._maybe_first_run(charm) - mock_client_cls.return_value.first_run.assert_not_called() +def _stop_workload(state: State) -> State: + """Stop the app service, so that a subsequent replan is observable as a restart.""" + container = state.get_container(CONTAINER_NAME) + stopped = dataclasses.replace( + container, + service_statuses={ + **container.service_statuses, + SERVICE_NAME: pebble.ServiceStatus.INACTIVE, + }, + ) + return dataclasses.replace(state, containers={stopped}) -def test_maybe_first_run_calls_first_run_when_not_initialized(): - """ - arrange: Leader unit with valid credentials; GarmApiClient.is_initialized returns False. - act: Call _maybe_first_run(). - assert: first_run is called with the admin credentials. - """ - charm = MagicMock() - charm.unit.is_leader.return_value = True - charm._get_admin_credentials.return_value = _MOCK_ADMIN_CREDS +def _workload_status(state: State) -> pebble.ServiceStatus: + """Return the app service's status in the given state.""" + return state.get_container(CONTAINER_NAME).service_statuses[SERVICE_NAME] - with patch("charm.GarmApiClient") as mock_client_cls: - mock_client_cls.return_value.is_initialized.return_value = False - GarmCharm._maybe_first_run(charm) - mock_client_cls.return_value.first_run.assert_called_once_with( - username="admin", - password="TestPass-123!", - email="admin@garm.local", - full_name="GARM Admin", - ) +# --- Secrets ------------------------------------------------------------------------------- -@pytest.mark.parametrize( - "error_message", - ["refused", "GARM did not become ready within 30s"], - ids=["connection-refused", "timeout"], -) -def test_maybe_first_run_raises_on_connection_error(error_message: str): +def test_leader_creates_garm_secrets(ctx: Context, garm_api: _GarmApiMocks): """ - arrange: Leader unit; GarmApiClient.wait_for_ready raises GarmConnectionError. - act: Call _maybe_first_run(). - assert: GarmConnectionError propagates and is_initialized is not called. + arrange: A leader unit with no GARM secrets yet. + act: Run install. + assert: Both labelled secrets are created, so other units can fetch them by label. """ - charm = MagicMock() - charm.unit.is_leader.return_value = True - charm._get_admin_credentials.return_value = _MOCK_ADMIN_CREDS + out = ctx.run(ctx.on.install(), _state()) - with patch("charm.GarmApiClient") as mock_client_cls: - mock_client_cls.return_value.wait_for_ready.side_effect = GarmConnectionError( - error_message - ) - with pytest.raises(GarmConnectionError): - GarmCharm._maybe_first_run(charm) - mock_client_cls.return_value.is_initialized.assert_not_called() + assert {secret.label for secret in out.secrets} == { + GARM_SECRETS_LABEL, + GARM_ADMIN_CREDENTIALS_LABEL, + } -def test_maybe_first_run_skips_on_missing_credential_key(): +def test_non_leader_creates_no_secrets(ctx: Context, garm_api: _GarmApiMocks): """ - arrange: Leader unit; admin credentials secret is missing required keys. - act: Call _maybe_first_run(). - assert: No GARM API call is made. + arrange: A unit that is not the Juju leader. + act: Run install. + assert: No secrets are created — only the leader may create them. """ - charm = MagicMock() - charm.unit.is_leader.return_value = True - charm._get_admin_credentials.return_value = {"username": "admin"} # missing password etc. + out = ctx.run(ctx.on.install(), _state(leader=False)) - with patch("charm.GarmApiClient") as mock_client_cls: - GarmCharm._maybe_first_run(charm) + assert not out.secrets - mock_client_cls.assert_not_called() - -def test_proxy_environment_happy_path(): +def test_existing_secrets_are_not_regenerated(ctx: Context, garm_api: _GarmApiMocks): """ - arrange: All three JUJU_CHARM_* proxy vars are set in the environment. - act: Call _proxy_environment(). - assert: Returns both lower- and upper-case variants for each variable - with the expected values. + arrange: A leader whose GARM secrets already exist. + act: Run install. + assert: Their contents are untouched — regenerating them would invalidate every + JWT GARM has issued and orphan its encrypted database rows. """ - env_vars = { - "JUJU_CHARM_HTTP_PROXY": "http://proxy.example.com:3128", - "JUJU_CHARM_HTTPS_PROXY": "https://proxy.example.com:3129", - "JUJU_CHARM_NO_PROXY": "localhost,127.0.0.1", - } - with patch.dict(os.environ, env_vars, clear=True): - result = _proxy_environment() + out = ctx.run(ctx.on.install(), _state(secrets=_owned_secrets())) - assert result["http_proxy"] == "http://proxy.example.com:3128" - assert result["HTTP_PROXY"] == "http://proxy.example.com:3128" - assert result["https_proxy"] == "https://proxy.example.com:3129" - assert result["HTTPS_PROXY"] == "https://proxy.example.com:3129" - assert result["no_proxy"] == "localhost,127.0.0.1" - assert result["NO_PROXY"] == "localhost,127.0.0.1" - assert len(result) == 6 + assert _secret_content(out, GARM_SECRETS_LABEL) == _GARM_SECRETS_CONTENT + assert _secret_content(out, GARM_ADMIN_CREDENTIALS_LABEL) == _ADMIN_CREDENTIALS_CONTENT -@pytest.mark.parametrize( - "env_vars,expected_keys", - [ - # Empty string values are dropped entirely. - ( - { - "JUJU_CHARM_HTTP_PROXY": "", - "JUJU_CHARM_HTTPS_PROXY": "", - "JUJU_CHARM_NO_PROXY": "", - }, - [], - ), - # Whitespace-only values are stripped and treated as empty. - ( - { - "JUJU_CHARM_HTTP_PROXY": " ", - "JUJU_CHARM_HTTPS_PROXY": "\t", - "JUJU_CHARM_NO_PROXY": " ", - }, - [], - ), - # Nothing set → empty result. - ({}, []), - # Only http_proxy set → only that pair is present. - ( - {"JUJU_CHARM_HTTP_PROXY": "http://proxy.example.com:3128"}, - ["http_proxy", "HTTP_PROXY"], - ), - ], - ids=["all-empty", "all-whitespace", "nothing-set", "only-http"], -) -def test_proxy_environment_edge_cases(env_vars: dict, expected_keys: list): +def test_secrets_are_created_before_the_workload_is_ready(ctx: Context, garm_api: _GarmApiMocks): """ - arrange: Various incomplete or empty JUJU_CHARM_* configurations. - act: Call _proxy_environment(). - assert: Only keys for non-empty values appear; empty/whitespace values - are omitted. + arrange: A leader whose workload container is not reachable yet. + act: Run install. + assert: The secrets exist even though the charm stops at the readiness gate, so they are + in place on install/leader-elected before pebble comes up. """ - with patch.dict(os.environ, env_vars, clear=True): - result = _proxy_environment() + out = ctx.run(ctx.on.install(), _state(can_connect=False)) - assert set(result.keys()) == set(expected_keys) + assert {secret.label for secret in out.secrets} == { + GARM_SECRETS_LABEL, + GARM_ADMIN_CREDENTIALS_LABEL, + } + assert out.unit_status == ops.WaitingStatus("Waiting for pebble ready") -def test_render_garm_toml_with_proxy_var_names(): +def test_non_leader_waits_for_the_leader_to_create_secrets(ctx: Context, garm_api: _GarmApiMocks): """ - arrange: Two provider configs and a non-empty proxy_var_names list. - act: Call render_garm_toml() with proxy_var_names. - assert: Every provider's external dict contains an environment_variables - key equal to the supplied list. + arrange: A non-leader unit before the leader has created the GARM secrets. + act: Run update-status. + assert: The unit waits rather than rendering a config without a JWT secret. """ - providers = [ - { - "unit_name": "garm-configurator-0", - "auth_url": "https://ks1.example.com:5000/v3", - "username": "admin1", - "password": "pass1", - "project_name": "proj1", - "user_domain_name": "Default", - "project_domain_name": "Default", - "region_name": "RegionOne", - "network": "net1", - }, - { - "unit_name": "garm-configurator-1", - "auth_url": "https://ks2.example.com:5000/v3", - "username": "admin2", - "password": "pass2", - "project_name": "proj2", - "user_domain_name": "Default", - "project_domain_name": "Default", - "region_name": "RegionTwo", - "network": "net2", - }, - ] - proxy_var_names = [ - "HTTP_PROXY", - "HTTPS_PROXY", - "NO_PROXY", - "http_proxy", - "https_proxy", - "no_proxy", - ] - toml_content, _ = render_garm_toml( - jwt_secret="test-secret", - db_passphrase="a" * 32, - postgresql_config=_DEFAULT_PG_CONFIG, - providers=providers, - proxy_var_names=proxy_var_names, - ) - parsed = tomllib.loads(toml_content) - - for provider in parsed["provider"]: - assert provider["external"]["environment_variables"] == proxy_var_names - - -def test_render_garm_toml_no_proxy_var_names_omits_key(): - """ - arrange: Provider configs but no proxy_var_names (default None). - act: Call render_garm_toml() without proxy_var_names. - assert: environment_variables is absent from every provider external dict - (preserves existing behaviour; the existing assertion in - test_render_garm_toml_with_configurator_providers also covers this). - """ - providers = [ - { - "unit_name": "garm-configurator-0", - "auth_url": "https://ks1.example.com:5000/v3", - "username": "admin1", - "password": "pass1", - "project_name": "proj1", - "user_domain_name": "Default", - "project_domain_name": "Default", - "region_name": "RegionOne", - "network": "net1", - }, - ] - toml_content, _ = render_garm_toml( - jwt_secret="test-secret", - db_passphrase="a" * 32, - postgresql_config=_DEFAULT_PG_CONFIG, - providers=providers, - ) - parsed = tomllib.loads(toml_content) - - for provider in parsed["provider"]: - assert "environment_variables" not in provider["external"] + out = ctx.run(ctx.on.update_status(), _state(leader=False)) + assert out.unit_status == ops.WaitingStatus("Waiting for GARM secrets") -_RESTART_PROVIDER_CONFIGS = [ - { - "unit_name": "garm-configurator-0", - "auth_url": "https://ks1.example.com:5000/v3", - "username": "admin1", - "password": "pass1", - "project_name": "proj1", - "user_domain_name": "Default", - "project_domain_name": "Default", - "region_name": "RegionOne", - "network": "net1", - } -] +# --- Workload configuration --------------------------------------------------------------- -def _plan_with_service_env(environment: dict | None) -> ops.pebble.Plan: - """Build a Pebble plan whose 'app' service carries the given environment. - Passing None yields an empty plan (no 'app' service), which models a - container that has never been configured — previous_hash resolves to None. +def test_workload_is_configured_from_relation_data(ctx: Context, garm_api: _GarmApiMocks): """ - if environment is None: - return ops.pebble.Plan() - plan = ops.pebble.Plan() - plan.services["app"] = ops.pebble.Service( - "app", - {"override": "replace", "command": "x", "environment": environment}, - ) - return plan - - -def _expected_config_hash(proxy_env: dict, providers: list | None = None) -> str: - """Reproduce restart()'s hash-input construction for assertion purposes. - - Mirrors the exact serialization restart() feeds into _hash_toml: the - rendered TOML, the sorted provider files, then — only when a proxy is - configured — the sorted proxy env key=value pairs. + arrange: A ready charm whose configurator unit publishes OpenStack credentials. + act: Run update-status. + assert: GARM's config.toml carries the postgresql connection and names the provider, the + provider's clouds.yaml is pushed alongside it, and the service runs GARM against + that config. """ - providers = _RESTART_PROVIDER_CONFIGS if providers is None else providers - toml_content, provider_files = render_garm_toml( - jwt_secret="test-jwt-secret", - db_passphrase="a" * 32, - postgresql_config=_DEFAULT_PG_CONFIG, - providers=providers, - proxy_var_names=sorted(proxy_env.keys()), - ) - hash_input = ( - toml_content - + "\n" - + "\n".join(f"{path}\n{content}" for path, content in sorted(provider_files.items())) + with patch.dict(os.environ, {}, clear=True): + out = ctx.run(ctx.on.update_status(), _state()) + + container = out.get_container(CONTAINER_NAME) + root = container.get_filesystem(ctx) + config = tomllib.loads((root / "etc/garm/config.toml").read_text()) + assert config["database"]["postgresql"]["hostname"] == "10.0.0.5" + assert config["database"]["postgresql"]["port"] == 5432 + assert config["database"]["postgresql"]["username"] == "garm" + assert config["provider"][0]["name"] == "garm-configurator-0" + + clouds = yaml.safe_load((root / "etc/garm/clouds-garm-configurator-0.yaml").read_text()) + assert clouds["clouds"]["garm-configurator-0"]["auth"]["password"] == "pass1" + + assert ( + container.plan.services[SERVICE_NAME].command + == "/usr/local/bin/garm -config /etc/garm/config.toml" ) - if proxy_env: - hash_input += "\n" + "\n".join(f"{k}={v}" for k, v in sorted(proxy_env.items())) - return GarmCharm._hash_toml(hash_input) - + assert out.unit_status == ops.ActiveStatus() -def _make_restart_charm(previous_plan_env: dict | None = None) -> MagicMock: - """Build a minimal GarmCharm mock suitable for testing restart() proxy injection. - Mirrors the mock setup pattern used by existing restart-related tests. - The mock wires the real _hash_toml so the actual hash logic runs, and - seeds the container's current Pebble plan (the source of previous_hash) - from ``previous_plan_env``; None models a never-configured container. +def test_workload_is_not_configured_without_postgresql_data(ctx: Context, garm_api: _GarmApiMocks): """ - charm = MagicMock() - charm.is_ready.return_value = True - charm.config.get.return_value = None - - # PostgreSQL relation data - charm._get_postgresql_config.return_value = _DEFAULT_PG_CONFIG - - # GARM secrets - charm._get_secrets.return_value = { - "jwt-secret": "test-jwt-secret", - "db-passphrase": "a" * 32, - } - - # Configurator provider configs — one real provider so render succeeds. - charm._get_configurator_provider_configs.return_value = list(_RESTART_PROVIDER_CONFIGS) - - # Wire the real hash function so hash assertions are meaningful. - # _hash_toml is a staticmethod, so GarmCharm._hash_toml is already a plain - # function — pass it directly as the side_effect. - charm._hash_toml.side_effect = GarmCharm._hash_toml - - # _current_config_hash is a staticmethod; run the real plan-reading logic - # against the seeded container plan (the source of previous_hash). - charm._current_config_hash.side_effect = GarmCharm._current_config_hash - - mock_container = MagicMock() - mock_container.get_plan.return_value = _plan_with_service_env(previous_plan_env) - charm.unit.get_container.return_value = mock_container - - return charm - - -_SENTINEL = object() # used to stop restart() execution before super().restart() - + arrange: A postgresql relation that has not published connection data yet. + act: Run update-status. + assert: The workload is left unconfigured — GARM cannot start without a database — and the + unit reports the missing integration. + """ + out = ctx.run(ctx.on.update_status(), _state(postgresql_data={})) -def _layer_service_env(charm: MagicMock) -> dict: - """Return the app service environment from the layer captured by add_layer.""" - layer_arg = charm.unit.get_container.return_value.add_layer.call_args[0][1] - return layer_arg["services"]["app"]["environment"] + assert SERVICE_NAME not in out.get_container(CONTAINER_NAME).plan.services + assert out.unit_status == ops.BlockedStatus("missing integrations: postgresql") -def test_restart_proxy_vars_appear_in_pebble_layer(): +def test_proxy_vars_reach_the_workload(ctx: Context, garm_api: _GarmApiMocks): """ - arrange: JUJU_CHARM_HTTP_PROXY and JUJU_CHARM_HTTPS_PROXY are set; container - has no existing config (forces replan path). - act: Call GarmCharm.restart() with the charm mock, intercepting execution - after add_layer via a sentinel exception raised by _maybe_first_run. - assert: The Pebble layer passed to add_layer has the proxy vars in the - service environment alongside config_hash. + arrange: Juju model proxy config for HTTP and HTTPS. + act: Run update-status. + assert: Both case variants land in the service environment, where GARM and the provider + executables it forks read them. """ - proxy_env_vars = { + proxy = { "JUJU_CHARM_HTTP_PROXY": "http://proxy.example.com:3128", "JUJU_CHARM_HTTPS_PROXY": "https://proxy.example.com:3129", } - charm = _make_restart_charm() - # Raise after add_layer so we never reach super().restart(), which requires - # self to be a real GarmCharm instance for the zero-arg super() check. - charm._maybe_first_run.side_effect = StopIteration(_SENTINEL) + with patch.dict(os.environ, proxy, clear=True): + out = ctx.run(ctx.on.update_status(), _state()) - with patch.dict(os.environ, proxy_env_vars, clear=True): - with patch("charm.GarmApiClient"): - with pytest.raises(StopIteration): - GarmCharm.restart(charm) + environment = _service_environment(out) + assert environment["http_proxy"] == "http://proxy.example.com:3128" + assert environment["HTTP_PROXY"] == "http://proxy.example.com:3128" + assert environment["https_proxy"] == "https://proxy.example.com:3129" + assert environment["HTTPS_PROXY"] == "https://proxy.example.com:3129" - charm.unit.get_container.return_value.add_layer.assert_called_once() - service_env = _layer_service_env(charm) - assert service_env["http_proxy"] == "http://proxy.example.com:3128" - assert service_env["HTTP_PROXY"] == "http://proxy.example.com:3128" - assert service_env["https_proxy"] == "https://proxy.example.com:3129" - assert service_env["HTTPS_PROXY"] == "https://proxy.example.com:3129" - assert "config_hash" in service_env +def test_unchanged_config_does_not_restart_the_workload(ctx: Context, garm_api: _GarmApiMocks): + """ + arrange: A charm that has already configured the workload, with the service since stopped. + act: Run update-status again against the same config. + assert: The service is left stopped — an unchanged config must not replan — while the + GARM-side reconcile still runs. + """ + with patch.dict(os.environ, {}, clear=True): + configured = ctx.run(ctx.on.update_status(), _state()) + out = ctx.run(ctx.on.update_status(), _stop_workload(configured)) + + assert _workload_status(out) == pebble.ServiceStatus.INACTIVE + assert out.unit_status == ops.ActiveStatus() -def test_restart_no_proxy_hash_matches_rendered_config(): +def test_proxy_value_change_rewrites_the_workload_layer(ctx: Context, garm_api: _GarmApiMocks): """ - arrange: No proxy vars are set; the charm renders config for one provider. - act: Run restart() and capture the config_hash it computes. - assert: It equals the hash of the rendered config folded with the (empty) - proxy env, so an unchanged config never triggers a spurious replan. + arrange: A configured workload whose model proxy then changes value only — the same + variable stays set, so the variable names embedded in the TOML do not change. + act: Run update-status. + assert: The layer is rewritten with the new value, i.e. a value-only change is detected + rather than being missed as an unchanged config. """ - charm = _make_restart_charm() - charm._maybe_first_run.side_effect = StopIteration(_SENTINEL) - - with patch.dict(os.environ, {}, clear=True): - with patch("charm.GarmApiClient"): - with pytest.raises(StopIteration): - GarmCharm.restart(charm) - captured = _layer_service_env(charm)["config_hash"] + with patch.dict(os.environ, {"JUJU_CHARM_HTTP_PROXY": "http://proxy-a:3128"}, clear=True): + configured = ctx.run(ctx.on.update_status(), _state()) + with patch.dict(os.environ, {"JUJU_CHARM_HTTP_PROXY": "http://proxy-b:3128"}, clear=True): + out = ctx.run(ctx.on.update_status(), _stop_workload(configured)) - assert captured == _expected_config_hash({}) + environment = _workload_layer_environment(out) + assert environment["http_proxy"] == "http://proxy-b:3128" + assert environment["HTTP_PROXY"] == "http://proxy-b:3128" -def test_restart_proxy_value_change_forces_layer_rewrite(): +def test_clearing_the_proxy_removes_it_from_the_workload_layer( + ctx: Context, garm_api: _GarmApiMocks +): """ - arrange: The container's current plan already carries a config_hash and an - http_proxy value computed from proxy value A; the model proxy is - now value B (same variable set, only the value changed). - act: Run restart(). - assert: The rewritten layer's service environment reflects value B, i.e. a - proxy-value-only change is detected and replanned. + arrange: A configured workload whose model proxy is then cleared. + act: Run update-status. + assert: The rewritten layer drops the proxy variables rather than carrying them over from + the previous one. """ - old_env = {"JUJU_CHARM_HTTP_PROXY": "http://proxy-a.example.com:3128"} - new_env = {"JUJU_CHARM_HTTP_PROXY": "http://proxy-b.example.com:3128"} - with patch.dict(os.environ, old_env, clear=True): - old_proxy = _proxy_environment() - old_hash = _expected_config_hash(old_proxy) + with patch.dict(os.environ, {"JUJU_CHARM_HTTP_PROXY": "http://proxy-a:3128"}, clear=True): + configured = ctx.run(ctx.on.update_status(), _state()) + with patch.dict(os.environ, {}, clear=True): + out = ctx.run(ctx.on.update_status(), configured) - charm = _make_restart_charm(previous_plan_env={"config_hash": old_hash, **old_proxy}) - charm._maybe_first_run.side_effect = StopIteration(_SENTINEL) + environment = _workload_layer_environment(out) + assert "http_proxy" not in environment + assert "HTTP_PROXY" not in environment - with ( - patch.dict(os.environ, new_env, clear=True), - patch("charm.GarmApiClient"), - pytest.raises(StopIteration), - ): - GarmCharm.restart(charm) - charm.unit.get_container.return_value.add_layer.assert_called_once() - service_env = _layer_service_env(charm) - assert service_env["http_proxy"] == "http://proxy-b.example.com:3128" - assert service_env["HTTP_PROXY"] == "http://proxy-b.example.com:3128" +# --- garm-configurator relation gating ---------------------------------------------------- -def test_restart_clearing_proxy_removes_it_from_layer(): +def test_missing_configurator_relation_prunes_orphaned_scalesets( + ctx: Context, garm_api: _GarmApiMocks +): """ - arrange: The container's current plan carries a proxy value; the model proxy - is now cleared (empty). - act: Run restart(). - assert: The rewritten layer's service environment no longer contains the - proxy keys. + arrange: A ready charm whose garm-configurator relation has been removed. + act: Run update-status. + assert: Scalesets are reconciled against an empty desired state, deleting the ones the + removed relation orphaned, while the workload is left unconfigured and the unit + reports what it is waiting for. """ - old_env = {"JUJU_CHARM_HTTP_PROXY": "http://proxy-a.example.com:3128"} - with patch.dict(os.environ, old_env, clear=True): - old_proxy = _proxy_environment() - old_hash = _expected_config_hash(old_proxy) + out = ctx.run(ctx.on.update_status(), _state(configurator_related=False)) - charm = _make_restart_charm(previous_plan_env={"config_hash": old_hash, **old_proxy}) - charm._maybe_first_run.side_effect = StopIteration(_SENTINEL) + garm_api.scaleset.return_value.reconcile.assert_called_once_with([]) + assert SERVICE_NAME not in out.get_container(CONTAINER_NAME).plan.services + assert out.unit_status == ops.WaitingStatus("Waiting for garm-configurator relation") - with ( - patch.dict(os.environ, {}, clear=True), - patch("charm.GarmApiClient"), - pytest.raises(StopIteration), - ): - GarmCharm.restart(charm) - charm.unit.get_container.return_value.add_layer.assert_called_once() - service_env = _layer_service_env(charm) - assert "http_proxy" not in service_env - assert "HTTP_PROXY" not in service_env +def test_unpopulated_configurator_relation_does_not_prune(ctx: Context, garm_api: _GarmApiMocks): + """ + arrange: A garm-configurator relation that is present but has not published its + secret-dependent fields yet. + act: Run update-status. + assert: Scalesets are NOT reconciled — pruning against the empty desired state would + delete live scalesets — and the unit waits for the relation to finish publishing. + """ + out = ctx.run(ctx.on.update_status(), _state(configurator_units_data={0: {}})) + + garm_api.scaleset.assert_not_called() + assert out.unit_status == ops.WaitingStatus("Waiting for garm-configurator relation") -def test_restart_unchanged_config_skips_replan(): +def test_missing_configurator_relation_refreshes_stale_app_status( + ctx: Context, garm_api: _GarmApiMocks +): """ - arrange: The container's current plan already stores the config_hash that - the current state (same proxy, same config) renders to. - act: Run restart(). - assert: The early-return path is taken — no new layer is added and the - service is not replanned — while runners are still reconciled. + arrange: A stale app status and no garm-configurator relation. + act: Run update-status. + assert: Both unit and app status report the wait, so the app status cannot freeze stale. """ - proxy_env = {"JUJU_CHARM_HTTP_PROXY": "http://proxy.example.com:3128"} - with patch.dict(os.environ, proxy_env, clear=True): - current_proxy = _proxy_environment() - current_hash = _expected_config_hash(current_proxy) + stale = ops.WaitingStatus("Waiting for pebble ready") + out = ctx.run( + ctx.on.update_status(), + _state(configurator_related=False, unit_status=stale, app_status=stale), + ) - charm = _make_restart_charm(previous_plan_env={"config_hash": current_hash, **current_proxy}) + assert out.unit_status == ops.WaitingStatus("Waiting for garm-configurator relation") + assert out.app_status == ops.WaitingStatus("Waiting for garm-configurator relation") - with ( - patch.dict(os.environ, proxy_env, clear=True), - patch("charm.GarmApiClient"), - ): - GarmCharm.restart(charm) - charm.unit.get_container.return_value.add_layer.assert_not_called() - charm.unit.get_container.return_value.replan.assert_not_called() - charm._reconcile_runners.assert_called_once() +# --- First-run initialisation ------------------------------------------------------------- -def test_restart_without_configurator_relation_still_prunes_orphaned_scalesets(): +def test_first_run_initialises_garm_with_the_stored_credentials( + ctx: Context, garm_api: _GarmApiMocks +): """ - arrange: The charm is ready (postgres + secrets available) but the - garm-configurator relation is gone, so there are no provider configs - and CharmState reports the relation absent. - act: Run restart(). - assert: _reconcile_runners() still runs so ScalesetReconciler can delete the - now-orphaned scalesets, the workload is not replanned, and the unit - reports that it is waiting for the garm-configurator relation. + arrange: A leader whose GARM reports itself uninitialised. + act: Run update-status. + assert: first-run is called with the credentials the charm actually stored in the secret, + so the operator can log in with what `juju secret-get` returns. """ - charm = _make_restart_charm() - charm._get_configurator_provider_configs.return_value = [] + garm_api.client.return_value.is_initialized.return_value = False - with patch.dict(os.environ, {}, clear=True), patch("charm.CharmState") as mock_state: - mock_state.from_charm.return_value.configurator_related = False - GarmCharm.restart(charm) + out = ctx.run(ctx.on.update_status(), _state()) - charm._reconcile_runners.assert_called_once() - charm.unit.get_container.return_value.add_layer.assert_not_called() - charm.update_app_and_unit_status.assert_called_once_with( - ops.WaitingStatus("Waiting for garm-configurator relation") + credentials = _secret_content(out, GARM_ADMIN_CREDENTIALS_LABEL) + garm_api.client.return_value.first_run.assert_called_once_with( + username=credentials["username"], + password=credentials["password"], + email=credentials["email"], + full_name=credentials["full-name"], ) -def test_restart_configurator_relation_present_but_unpopulated_does_not_prune(): +def test_first_run_skipped_when_garm_is_already_initialised(ctx: Context, garm_api: _GarmApiMocks): """ - arrange: The charm is ready but the garm-configurator relation, though still - present, has not yet published its secret-dependent fields, so there - are no provider configs while CharmState reports the relation present. - act: Run restart(). - assert: _reconcile_runners() is NOT called — pruning against the empty desired - state would delete live scalesets — the workload is not replanned, and - the unit waits for the relation to finish publishing. + arrange: A leader whose GARM reports itself initialised. + act: Run update-status. + assert: first-run is not called again. """ - charm = _make_restart_charm() - charm._get_configurator_provider_configs.return_value = [] + garm_api.client.return_value.is_initialized.return_value = True - with patch.dict(os.environ, {}, clear=True), patch("charm.CharmState") as mock_state: - mock_state.from_charm.return_value.configurator_related = True - GarmCharm.restart(charm) + ctx.run(ctx.on.update_status(), _state()) - charm._reconcile_runners.assert_not_called() - charm.unit.get_container.return_value.add_layer.assert_not_called() - charm.update_app_and_unit_status.assert_called_once_with( - ops.WaitingStatus("Waiting for garm-configurator relation") - ) + garm_api.client.return_value.first_run.assert_not_called() -def test_render_garm_toml_default_provider_applies_proxy_var_names(): +def test_first_run_skipped_when_not_leader(ctx: Context, garm_api: _GarmApiMocks): """ - arrange: No configurator providers but a non-empty proxy_var_names list. - act: Call render_garm_toml() with providers=None and proxy_var_names. - assert: The default openstack provider forwards the proxy var names via - environment_variables (consistent with the per-provider path). + arrange: A non-leader unit that can already read the GARM secrets. + act: Run update-status. + assert: No GARM API call is made — only the leader initialises the shared controller. """ - proxy_var_names = ["http_proxy", "HTTP_PROXY"] - toml_content, _ = render_garm_toml( - jwt_secret="test-secret", - db_passphrase="a" * 32, - postgresql_config=_DEFAULT_PG_CONFIG, - proxy_var_names=proxy_var_names, - ) - parsed = tomllib.loads(toml_content) + ctx.run(ctx.on.update_status(), _state(leader=False, secrets=_owned_secrets())) - assert parsed["provider"][0]["external"]["environment_variables"] == proxy_var_names + garm_api.client.assert_not_called() -def test_reconcile_runners_skips_when_no_admin_credentials(): +def test_first_run_skipped_when_credentials_are_incomplete(ctx: Context, garm_api: _GarmApiMocks): """ - arrange: Admin credentials secret is unavailable. - act: Call _reconcile_runners(). - assert: No GARM API connection is attempted. + arrange: A leader whose existing admin-credentials secret is missing the password. Secret + creation skips labels that already exist, so an incomplete secret survives. + act: Run update-status. + assert: GARM is never contacted — a half-built admin account would lock the operator out. """ - charm = object.__new__(GarmCharm) - charm._get_admin_credentials = MagicMock(return_value=None) + secrets = _owned_secrets(admin_content={"username": "admin"}) - with patch("charm.GarmAuthenticatedClient") as mock_auth_cls: - charm._reconcile_runners() + with patch.object(GarmCharm, "_reconcile_runners"): + ctx.run(ctx.on.update_status(), _state(secrets=secrets)) - mock_auth_cls.from_login.assert_not_called() + garm_api.client.assert_not_called() -def test_reconcile_runners_reconciles_credentials_entities_then_scalesets(): +@pytest.mark.parametrize( + "error_message", + ["refused", "GARM did not become ready within 30s"], + ids=["connection-refused", "timeout"], +) +def test_first_run_connection_error_errors_out_for_retry( + ctx: Context, garm_api: _GarmApiMocks, error_message: str +): """ - arrange: Admin credentials are available and the desired credential/entity/scaleset specs are - stubbed. - act: Call _reconcile_runners(). - assert: On a single authenticated client it configures controller URLs, then reconciles - credentials, entities, and scalesets in that dependency order, reports ActiveStatus, and - never restarts. + arrange: A GARM that never becomes reachable. + act: Run update-status. + assert: The error propagates so Juju retries the hook, and initialisation is not attempted + against an unreachable GARM. """ - charm = object.__new__(GarmCharm) - charm._get_admin_credentials = MagicMock( - return_value={"username": "admin", "password": "TestPass-123!"} - ) - credentials = [object()] - entities = [object()] - scalesets = [object()] - charm._build_desired_credentials = MagicMock(return_value=credentials) - charm._build_desired_scalesets = MagicMock(return_value=scalesets) - charm._ensure_controller_urls = MagicMock() - charm.restart = MagicMock() + garm_api.client.return_value.wait_for_ready.side_effect = GarmConnectionError(error_message) - with ( - patch("charm.GarmAuthenticatedClient") as mock_auth_cls, - patch("charm.GithubReconciler") as mock_github_cls, - patch("charm.EntityReconciler") as mock_entity_cls, - patch("charm.ScalesetReconciler") as mock_scaleset_cls, - patch("charm._apply_garm_template", return_value=99) as mock_apply, - patch("charm.CharmState") as mock_charm_state_cls, - patch.object(GarmCharm, "unit", new_callable=PropertyMock) as mock_unit, - patch.object(GarmCharm, "app", new_callable=PropertyMock) as mock_app, - ): - mock_charm_state_cls.from_charm.return_value.ssh_debug_connections = [] - mock_charm_state_cls.from_charm.return_value.desired_entities = entities - mock_unit.return_value = MagicMock() - mock_app.return_value = MagicMock() - order = MagicMock() - order.attach_mock(mock_github_cls.return_value.reconcile, "github") - order.attach_mock(mock_entity_cls.return_value.reconcile, "entity") - order.attach_mock(mock_scaleset_cls.return_value.reconcile, "scaleset") - charm._reconcile_runners() - - expected_url = f"http://127.0.0.1:{GARM_PORT}/api/v1" - mock_auth_cls.from_login.assert_called_once_with(expected_url, "admin", "TestPass-123!") - auth_client = mock_auth_cls.from_login.return_value - # Controller URLs must be configured before any operational call, or GARM returns 409. - charm._ensure_controller_urls.assert_called_once_with(auth_client) - # Each reconciler must be built against the same authenticated client. - mock_github_cls.assert_called_once_with(auth_client) - mock_entity_cls.assert_called_once_with(auth_client) - mock_apply.assert_called_once() - charm._build_desired_scalesets.assert_called_once_with(99) - mock_scaleset_cls.assert_called_once_with(auth_client) - mock_github_cls.return_value.reconcile.assert_called_once_with(credentials) - mock_entity_cls.return_value.reconcile.assert_called_once_with(entities) - mock_scaleset_cls.return_value.reconcile.assert_called_once_with(scalesets) - # Entities depend on credentials and scalesets depend on entities, so order matters. - assert [name for name, _, _ in order.mock_calls] == ["github", "entity", "scaleset"] - charm.restart.assert_not_called() - - -def test_reconcile_runners_charmed_template_error_sets_waiting_status(): - """ - arrange: Admin credentials are available but _apply_garm_template raises CharmedTemplateError. - act: Call _reconcile_runners(). - assert: The unit degrades to WaitingStatus carrying the error message (not an error state), - and scalesets are never reconciled. - """ - charm = object.__new__(GarmCharm) - charm._get_admin_credentials = MagicMock( - return_value={"username": "admin", "password": "TestPass-123!"} - ) - charm._build_desired_credentials = MagicMock(return_value=[]) - charm._build_desired_scalesets = MagicMock(return_value=[]) - charm._ensure_controller_urls = MagicMock() + with pytest.raises(UncaughtCharmError) as exc_info: + ctx.run(ctx.on.update_status(), _state()) - with ( - patch("charm.GarmAuthenticatedClient"), - patch("charm.GithubReconciler"), - patch("charm.ScalesetReconciler") as mock_scaleset_cls, - patch( - "charm._apply_garm_template", - side_effect=garm_template.CharmedTemplateError("base template missing"), - ), - patch("charm.CharmState") as mock_state, - patch.object(GarmCharm, "unit", new_callable=PropertyMock) as mock_unit, - patch.object(GarmCharm, "app", new_callable=PropertyMock) as mock_app, - ): - mock_state.from_charm.return_value.ssh_debug_connections = [] - mock_unit.return_value = MagicMock() - mock_app.return_value = MagicMock() - charm._reconcile_runners() # must not raise + assert isinstance(exc_info.value.__cause__, GarmConnectionError) + garm_api.client.return_value.is_initialized.assert_not_called() - status = mock_unit.return_value.status - mock_scaleset_cls.return_value.reconcile.assert_not_called() - assert isinstance(status, ops.WaitingStatus) - assert status.message == "base template missing" +# --- Reconciling GARM --------------------------------------------------------------------- -def test_reconcile_runners_success_refreshes_stale_app_status(): +def test_reconcilers_run_in_dependency_order(ctx: Context, garm_api: _GarmApiMocks): """ - arrange: A stale app status and a reconcile that will succeed. - act: Call _reconcile_runners(). - assert: Both unit and app status refresh to ActiveStatus. + arrange: A ready charm. + act: Run update-status. + assert: Controller URLs are configured first — GARM 409s every operational call until they + are set — then credentials, entities and scalesets reconcile in that order, since + entities reference a credential and scalesets are created under an entity. All of them + run against the one authenticated client. """ - charm = object.__new__(GarmCharm) - charm._get_admin_credentials = MagicMock( - return_value={"username": "admin", "password": "TestPass-123!"} - ) - charm._build_desired_credentials = MagicMock(return_value=[]) - charm._build_desired_scalesets = MagicMock(return_value=[]) - charm._ensure_controller_urls = MagicMock() - - with ( - patch("charm.GarmAuthenticatedClient"), - patch("charm.GithubReconciler"), - patch("charm.EntityReconciler"), - patch("charm.ScalesetReconciler"), - patch("charm._apply_garm_template", return_value=99), - patch("charm.CharmState") as mock_charm_state_cls, - patch.object(GarmCharm, "unit", new_callable=PropertyMock) as mock_unit, - patch.object(GarmCharm, "app", new_callable=PropertyMock) as mock_app, - ): - mock_charm_state_cls.from_charm.return_value.ssh_debug_connections = [] - mock_charm_state_cls.from_charm.return_value.desired_entities = [] - mock_unit.return_value = MagicMock() - mock_unit.return_value.is_leader.return_value = True - mock_unit.return_value.status = ops.WaitingStatus("Waiting for pebble ready") - mock_app.return_value = MagicMock() - mock_app.return_value.status = ops.WaitingStatus("Waiting for pebble ready") + ctx.run(ctx.on.update_status(), _state()) - charm._reconcile_runners() - - unit_status = mock_unit.return_value.status - app_status = mock_app.return_value.status - - assert unit_status == ops.ActiveStatus() - assert app_status == ops.ActiveStatus() + assert [name for name, _, _ in garm_api.calls.mock_calls] == [ + "controller", + "github", + "entity", + "scaleset", + ] + garm_api.github.assert_called_once_with(garm_api.auth_client) + garm_api.entity.assert_called_once_with(garm_api.auth_client) + garm_api.scaleset.assert_called_once_with(garm_api.auth_client) -def test_restart_missing_configurator_relation_refreshes_stale_app_status(): +def test_reconcile_authenticates_against_the_local_garm_listener( + ctx: Context, garm_api: _GarmApiMocks +): """ - arrange: A stale app status and a missing garm-configurator relation. - act: Call restart(). - assert: Both unit and app status become "Waiting for garm-configurator relation". + arrange: A ready charm. + act: Run update-status. + assert: The charm logs in over GARM's fixed local listener with the stored admin + credentials, rather than a config-derived URL that is unset for the local API. """ - charm = object.__new__(GarmCharm) - charm._ensure_secrets = MagicMock() - charm.is_ready = MagicMock(return_value=True) - charm._get_postgresql_config = MagicMock(return_value=_DEFAULT_PG_CONFIG) - charm._get_secrets = MagicMock( - return_value={"jwt-secret": "test-jwt-secret", "db-passphrase": "a" * 32} - ) - charm._get_configurator_provider_configs = MagicMock(return_value=[]) - # restart() now still prunes orphaned scalesets when the relation is gone; - # stub it out since this test only cares about the status it reports. - charm._reconcile_runners = MagicMock() + out = ctx.run(ctx.on.update_status(), _state()) - with ( - patch.object(GarmCharm, "config", new_callable=PropertyMock) as mock_config, - patch.object(GarmCharm, "unit", new_callable=PropertyMock) as mock_unit, - patch.object(GarmCharm, "app", new_callable=PropertyMock) as mock_app, - patch.object(GarmCharm, "model", new_callable=PropertyMock) as mock_model, - patch("charm.CharmState") as mock_state, - ): - mock_config.return_value = {} - mock_unit.return_value = MagicMock() - mock_unit.return_value.is_leader.return_value = True - mock_unit.return_value.status = ops.WaitingStatus("Waiting for pebble ready") - mock_app.return_value = MagicMock() - mock_app.return_value.status = ops.WaitingStatus("Waiting for pebble ready") - mock_state.from_charm.return_value.configurator_related = False + credentials = _secret_content(out, GARM_ADMIN_CREDENTIALS_LABEL) + garm_api.auth.from_login.assert_called_once_with( + f"http://127.0.0.1:{GARM_PORT}/api/v1", + credentials["username"], + credentials["password"], + ) - charm.restart() - unit_status = mock_unit.return_value.status - app_status = mock_app.return_value.status +def test_reconcile_skipped_when_admin_credentials_are_unavailable( + ctx: Context, garm_api: _GarmApiMocks +): + """ + arrange: A charm that cannot read its admin-credentials secret. A leader recreates that + secret on every reconcile, so this models the secret store failing rather than a state + the charm reaches on its own — hence the patch. + act: Run update-status. + assert: No GARM connection is attempted with credentials the charm does not have. + """ + with patch.object(GarmCharm, "_get_admin_credentials", return_value=None): + ctx.run(ctx.on.update_status(), _state()) - assert unit_status == ops.WaitingStatus("Waiting for garm-configurator relation") - assert app_status == ops.WaitingStatus("Waiting for garm-configurator relation") + garm_api.auth.from_login.assert_not_called() -def test_restart_ensures_secrets_before_readiness_gate(): +def test_successful_reconcile_refreshes_stale_app_status(ctx: Context, garm_api: _GarmApiMocks): """ - arrange: A GarmCharm whose workload is not ready (pebble not up yet). - act: Call restart(). - assert: _ensure_secrets is invoked even though is_ready() returns False, - so secrets are created before pebble is up (install/leader_elected). + arrange: A stale unit and app status, and a reconcile that will succeed. + act: Run update-status. + assert: Both refresh to active, so the app status cannot freeze stale. """ - charm = object.__new__(GarmCharm) - charm._ensure_secrets = MagicMock() - charm.is_ready = MagicMock(return_value=False) + stale = ops.WaitingStatus("Waiting for pebble ready") - charm.restart() + out = ctx.run(ctx.on.update_status(), _state(unit_status=stale, app_status=stale)) - charm._ensure_secrets.assert_called_once() + assert out.unit_status == ops.ActiveStatus() + assert out.app_status == ops.ActiveStatus() -def test_reconcile_calls_restart(): +def test_charmed_template_error_degrades_to_waiting(ctx: Context, garm_api: _GarmApiMocks): """ - arrange: A bare GarmCharm instance with restart and _create_charm_state mocked - (the latter so the block_if_invalid_data decorator does not raise). - act: Call _reconcile. - assert: restart is called — every observed hook flows through _reconcile. + arrange: A configured, steady-state charm whose charmed runner template then fails to apply. + act: Run update-status. + assert: The unit waits carrying the error message rather than erroring, and scalesets are + not reconciled — each one references the template. """ - charm = object.__new__(GarmCharm) - charm.restart = MagicMock() - charm._create_charm_state = MagicMock() + with patch.dict(os.environ, {}, clear=True): + configured = ctx.run(ctx.on.update_status(), _state()) + garm_api.apply_template.side_effect = garm_template.CharmedTemplateError( + "base template missing" + ) + garm_api.scaleset.return_value.reconcile.reset_mock() + + out = ctx.run(ctx.on.update_status(), configured) - GarmCharm._reconcile(charm, MagicMock()) + assert out.unit_status == ops.WaitingStatus("base template missing") + garm_api.scaleset.return_value.reconcile.assert_not_called() - charm.restart.assert_called_once() +# --- Desired state from relation data ----------------------------------------------------- -def _github_charm(units_data, private_key="-----PEM-----"): - """Build a GarmCharm stub whose configurator relation exposes the given unit databags.""" - charm = MagicMock(spec=GarmCharm) - relation = MagicMock() - data_map = {} - for unit_data in units_data: - unit = MagicMock() - data_map[unit] = unit_data - relation.units = list(data_map) - relation.data = data_map - charm.model.relations.get.return_value = [relation] - charm._resolve_secret_value.return_value = private_key - return charm + +def _github_unit_data(secret_id: str, app_id: str = "12345", installation_id: str = "67890"): + """Build the GitHub App fields a configurator unit publishes.""" + return { + "github_app_id": app_id, + "github_installation_id": installation_id, + "github_private_key_secret_uri": secret_id, + } -def test_build_desired_credentials_builds_credential_from_relation(): +def test_github_credential_is_built_from_relation_data(ctx: Context, garm_api: _GarmApiMocks): """ - arrange: A charm whose configurator relation exposes one unit with app id, installation id - and a private-key secret URI. - act: Call _build_desired_credentials. - assert: One App credential is built (named app--) on the built-in - github.com endpoint, with the private key resolved from the secret. + arrange: A configurator unit publishing a GitHub App id, installation id and a secret URI + for the private key. + act: Run update-status. + assert: One credential is reconciled on GARM's built-in github.com endpoint, with the + private key resolved from the Juju secret rather than the databag. """ - charm = _github_charm( - [ - { - "github_app_id": "12345", - "github_installation_id": "67890", - "github_private_key_secret_uri": "secret:abc", - } - ], - private_key="PEMDATA", + key_secret = Secret(tracked_content={"value": "PEMDATA"}) + state = _state( + configurator_units_data={0: {**_PROVIDER_UNIT_DATA, **_github_unit_data(key_secret.id)}}, + secrets=[key_secret], ) - credentials = GarmCharm._build_desired_credentials(charm) + ctx.run(ctx.on.update_status(), state) - assert len(credentials) == 1 - cred = credentials[0] - assert cred.name == "app-12345-67890" - assert cred.endpoint == DEFAULT_GITHUB_ENDPOINT - assert cred.app_id == 12345 - assert cred.installation_id == 67890 - assert cred.private_key == "PEMDATA" - charm._resolve_secret_value.assert_called_once_with("secret:abc") + garm_api.github.return_value.reconcile.assert_called_once_with( + [ + CredentialSpec( + name="app-12345-67890", + endpoint=DEFAULT_GITHUB_ENDPOINT, + app_id=12345, + installation_id=67890, + private_key="PEMDATA", + description=MANAGED_CREDENTIAL_DESCRIPTION, + ) + ] + ) -def test_build_desired_credentials_dedupes_per_app_installation(): +def test_units_sharing_a_github_app_yield_one_credential(ctx: Context, garm_api: _GarmApiMocks): """ - arrange: A charm whose configurator relation exposes two units sharing one App/installation. - act: Call _build_desired_credentials. - assert: The two units collapse to a single credential. + arrange: Two configurator units sharing one GitHub App and installation. + act: Run update-status. + assert: They collapse to a single credential. """ - unit_data = { - "github_app_id": "1", - "github_installation_id": "2", - "github_private_key_secret_uri": "secret:k", - } - charm = _github_charm([dict(unit_data), dict(unit_data)]) + key_secret = Secret(tracked_content={"value": "PEMDATA"}) + unit_data = {**_PROVIDER_UNIT_DATA, **_github_unit_data(key_secret.id, "1", "2")} + state = _state( + configurator_units_data={0: dict(unit_data), 1: dict(unit_data)}, + secrets=[key_secret], + ) - credentials = GarmCharm._build_desired_credentials(charm) + ctx.run(ctx.on.update_status(), state) + (credentials,) = garm_api.github.return_value.reconcile.call_args.args assert len(credentials) == 1 -def test_build_desired_credentials_skips_incomplete_unit(): +@pytest.mark.parametrize( + "github_data", + [ + pytest.param({"github_app_id": "1"}, id="missing-fields"), + pytest.param( + _github_unit_data("secret:unavailable", app_id="not-a-number"), id="non-numeric-ids" + ), + pytest.param(_github_unit_data("secret:unavailable"), id="unreadable-key-secret"), + ], +) +def test_incomplete_github_config_yields_no_credential( + ctx: Context, garm_api: _GarmApiMocks, github_data: dict +): """ - arrange: A charm whose configurator relation exposes a unit missing required GitHub fields. - act: Call _build_desired_credentials. - assert: No credential is built. + arrange: A configurator unit whose GitHub App fields are incomplete, non-numeric, or name + a private-key secret this unit cannot read. + act: Run update-status. + assert: No credential is built — GARM rejects a partial one. """ - charm = _github_charm([{"github_app_id": "1"}]) + state = _state(configurator_units_data={0: {**_PROVIDER_UNIT_DATA, **github_data}}) - credentials = GarmCharm._build_desired_credentials(charm) + ctx.run(ctx.on.update_status(), state) - assert credentials == [] + garm_api.github.return_value.reconcile.assert_called_once_with([]) -def test_build_desired_credentials_skips_when_secret_unavailable(): +def test_scaleset_is_built_from_relation_data(ctx: Context, garm_api: _GarmApiMocks): """ - arrange: A charm whose configurator unit is complete but whose private-key secret resolves - to an empty string. - act: Call _build_desired_credentials. - assert: No credential is built. + arrange: A configurator unit publishing a full scaleset spec naming a repo. + act: Run update-status. + assert: The scaleset is reconciled against the repository entity and references the + charmed template that was ensured before it. """ - charm = _github_charm( - [ - { - "github_app_id": "1", - "github_installation_id": "2", - "github_private_key_secret_uri": "secret:k", - } - ], - private_key="", - ) + ctx.run(ctx.on.update_status(), _state()) + + (scalesets,) = garm_api.scaleset.return_value.reconcile.call_args.args + assert len(scalesets) == 1 + scaleset = scalesets[0] + assert scaleset.name == "my-scaleset" + assert scaleset.entity_type == "repository" + assert scaleset.entity_name == "myorg/myrepo" + assert scaleset.max_runners == 5 + assert scaleset.min_idle_runners == 1 + assert scaleset.template_id == _TEMPLATE_ID - credentials = GarmCharm._build_desired_credentials(charm) - assert credentials == [] +# --- Controller URLs ---------------------------------------------------------------------- -def test_build_desired_credentials_skips_non_numeric_ids(): +def test_controller_urls_default_to_the_kubernetes_service_url( + ctx: Context, garm_api: _GarmApiMocks +): """ - arrange: A charm whose configurator unit has a non-numeric app/installation id. - act: Call _build_desired_credentials. - assert: No credential is built. + arrange: A ready charm with no ingress. + act: Run update-status. + assert: The controller URLs point at the in-cluster service, which runners reach for + metadata and callbacks. """ - charm = _github_charm( - [ - { - "github_app_id": "not-a-number", - "github_installation_id": "2", - "github_private_key_secret_uri": "secret:k", - } - ] - ) - - credentials = GarmCharm._build_desired_credentials(charm) + ctx.run(ctx.on.update_status(), _state()) - assert credentials == [] + base = f"http://garm.{MODEL_NAME}:{GARM_PORT}" + garm_api.auth_client.update_controller.assert_called_once_with( + metadata_url=f"{base}/api/v1/metadata", + callback_url=f"{base}/api/v1/callbacks", + webhook_url=f"{base}/webhooks", + ) -def test_ensure_controller_urls_sets_urls_from_base_url(): +@pytest.mark.parametrize( + "base_url", + ["https://garm.example.com", "https://garm.example.com/"], + ids=["plain", "trailing-slash"], +) +def test_controller_urls_derive_from_the_ingress_url( + ctx: Context, garm_api: _GarmApiMocks, base_url: str +): """ - arrange: A charm whose _base_url is an ingress URL. - act: Call _ensure_controller_urls. - assert: update_controller is called once with metadata/callback/webhook URLs derived from it. + arrange: Ingress supplies the application's base URL, with and without a trailing slash. + act: Run update-status. + assert: The pushed URLs are the same either way, with no double slashes in their paths. """ - charm = MagicMock(spec=GarmCharm) - charm._base_url = "https://garm.example.com" - auth_client = MagicMock() - - GarmCharm._ensure_controller_urls(charm, auth_client) + with patch.object(GarmCharm, "_base_url", new_callable=PropertyMock, return_value=base_url): + ctx.run(ctx.on.update_status(), _state()) - auth_client.update_controller.assert_called_once_with( + garm_api.auth_client.update_controller.assert_called_once_with( metadata_url="https://garm.example.com/api/v1/metadata", callback_url="https://garm.example.com/api/v1/callbacks", webhook_url="https://garm.example.com/webhooks", ) -def test_ensure_controller_urls_strips_trailing_slash_from_base_url(): +# --- Event wiring ------------------------------------------------------------------------- + + +def _relation(state: State, endpoint: str) -> Relation: + """Return the state's relation on the given endpoint.""" + return next(relation for relation in state.relations if relation.endpoint == endpoint) # type: ignore[return-value] + + +_OBSERVED_EVENTS = [ + pytest.param(lambda ctx, _: ctx.on.install(), id="install"), + pytest.param(lambda ctx, _: ctx.on.leader_elected(), id="leader-elected"), + pytest.param(lambda ctx, _: ctx.on.update_status(), id="update-status"), +] +for _endpoint, _id in ( + (GARM_CONFIGURATOR_RELATION_NAME, "configurator"), + (DEBUG_SSH_INTEGRATION_NAME, "debug-ssh"), +): + _OBSERVED_EVENTS += [ + pytest.param( + lambda ctx, state, endpoint=_endpoint: ctx.on.relation_joined( + _relation(state, endpoint) + ), + id=f"{_id}-relation-joined", + ), + pytest.param( + lambda ctx, state, endpoint=_endpoint: ctx.on.relation_changed( + _relation(state, endpoint) + ), + id=f"{_id}-relation-changed", + ), + pytest.param( + lambda ctx, state, endpoint=_endpoint: ctx.on.relation_departed( + _relation(state, endpoint), remote_unit=0 + ), + id=f"{_id}-relation-departed", + ), + pytest.param( + lambda ctx, state, endpoint=_endpoint: ctx.on.relation_broken( + _relation(state, endpoint) + ), + id=f"{_id}-relation-broken", + ), + ] + + +@pytest.mark.parametrize("event", _OBSERVED_EVENTS) +def test_every_observed_event_reconciles( + ctx: Context, garm_api: _GarmApiMocks, event: typing.Callable +): """ - arrange: A charm whose _base_url is an ingress URL with a trailing slash. - act: Call _ensure_controller_urls. - assert: The pushed URLs have no double slashes in their paths. + arrange: A ready charm related to garm-configurator and debug-ssh. + act: Run each event the charm observes. + assert: Every one drives a full reconcile. The charm holds no per-event delta logic, so an + observer that is missed leaves GARM unsynced until the next hook happens to fire. """ - charm = MagicMock(spec=GarmCharm) - charm._base_url = "https://garm.example.com/" - auth_client = MagicMock() + state = _state(debug_ssh_related=True) - GarmCharm._ensure_controller_urls(charm, auth_client) + ctx.run(event(ctx, state), state) - auth_client.update_controller.assert_called_once_with( - metadata_url="https://garm.example.com/api/v1/metadata", - callback_url="https://garm.example.com/api/v1/callbacks", - webhook_url="https://garm.example.com/webhooks", - ) + garm_api.auth.from_login.assert_called_once() diff --git a/charms/garm/tests/unit/test_garm_toml.py b/charms/garm/tests/unit/test_garm_toml.py new file mode 100644 index 00000000..9284a94d --- /dev/null +++ b/charms/garm/tests/unit/test_garm_toml.py @@ -0,0 +1,439 @@ +# Copyright 2026 Canonical Ltd. +# See LICENSE file for licensing details. + +"""Unit tests for the config-rendering and secret-generation helpers in charm.py. + +These are the module-level functions GarmCharm.restart() composes: they take plain +arguments and return rendered content, so they are exercised directly. The charm's +event handling is tested through Scenario in test_charm.py. +""" + +import os +import string +from unittest.mock import patch + +import pytest +import yaml + +try: + import tomllib +except ImportError: + import tomli as tomllib # type: ignore[no-redef] + +from charm import ( + _build_provider_list, + _generate_admin_password, + _generate_garm_secrets, + _proxy_environment, + render_garm_toml, +) + +_DEFAULT_PG_CONFIG = { + "username": "u", + "password": "p", + "hostname": "h", + "port": 5432, + "database": "d", + "sslmode": "disable", +} + +_PROVIDERS = [ + { + "unit_name": "garm-configurator-0", + "auth_url": "https://ks1.example.com:5000/v3", + "username": "admin1", + "password": "pass1", + "project_name": "proj1", + "user_domain_name": "Default", + "project_domain_name": "Default", + "region_name": "RegionOne", + "network": "net1", + }, + { + "unit_name": "garm-configurator-1", + "auth_url": "https://ks2.example.com:5000/v3", + "username": "admin2", + "password": "pass2", + "project_name": "proj2", + "user_domain_name": "Default", + "project_domain_name": "Default", + "region_name": "RegionTwo", + "network": "net2", + }, +] + + +def _render(**overrides) -> dict: + """Helper: render TOML with defaults, return parsed dict.""" + kwargs = { + "jwt_secret": "test-secret", + "db_passphrase": "a" * 32, + "postgresql_config": _DEFAULT_PG_CONFIG, + } + kwargs.update(overrides) + toml_content, _ = render_garm_toml(**kwargs) + return tomllib.loads(toml_content) + + +def test_render_garm_toml_postgresql_backend(): + """The [database] section uses postgresql backend with correct fields.""" + parsed = _render( + postgresql_config={ + "username": "garm", + "password": "secret", + "hostname": "10.0.0.5", + "port": 5432, + "database": "garm_db", + "sslmode": "require", + }, + ) + assert parsed["database"]["backend"] == "postgresql" + assert parsed["database"]["postgresql"]["hostname"] == "10.0.0.5" + assert parsed["database"]["postgresql"]["username"] == "garm" + assert parsed["database"]["postgresql"]["password"] == "secret" + assert parsed["database"]["postgresql"]["port"] == 5432 + assert parsed["database"]["postgresql"]["database"] == "garm_db" + assert parsed["database"]["postgresql"]["sslmode"] == "require" + assert "sqlite3" not in parsed["database"] + + +def test_render_garm_toml_passphrase_in_database_section(): + """The passphrase appears in the [database] section.""" + passphrase = "b" * 32 + parsed = _render(db_passphrase=passphrase) + assert parsed["database"]["passphrase"] == passphrase + + +@pytest.mark.parametrize( + "sslmode", ["disable", "allow", "prefer", "require", "verify-ca", "verify-full"] +) +def test_render_garm_toml_sslmode_propagated(sslmode: str): + """The sslmode value is propagated to the postgresql section.""" + pg_config = {**_DEFAULT_PG_CONFIG, "sslmode": sslmode} + parsed = _render(postgresql_config=pg_config) + assert parsed["database"]["postgresql"]["sslmode"] == sslmode + + +@pytest.mark.parametrize( + "section,key,value,kwargs", + [ + ("apiserver", "bind", "0.0.0.0", {}), + ("apiserver", "port", 8080, {}), + ("apiserver", "use_tls", False, {}), + ("jwt_auth", "secret", "mysecret", {"jwt_secret": "mysecret"}), + ("jwt_auth", "time_to_live", "8760h", {}), + ("metrics", "disable_auth", True, {}), + ("metrics", "enable", True, {}), + ], + ids=[ + "apiserver-bind", + "apiserver-port", + "apiserver-use_tls", + "jwt_auth-secret", + "jwt_auth-time_to_live", + "metrics-disable_auth", + "metrics-enable", + ], +) +def test_render_garm_toml_section_fields(section: str, key: str, value, kwargs: dict): + """Config sections reflect the given parameters.""" + parsed = _render(**kwargs) + assert parsed[section][key] == value + + +def test_render_garm_toml_provider_section(): + """The [[provider]] section has the OpenStack provider binary.""" + parsed = _render() + assert len(parsed["provider"]) == 1 + provider = parsed["provider"][0] + assert provider["name"] == "openstack" + assert provider["provider_type"] == "external" + assert provider["external"]["provider_executable"] == "/usr/local/bin/garm-provider-openstack" + + +def test_generate_garm_secrets_returns_jwt_and_passphrase(): + """Returns a dict with jwt-secret (64-char hex) and db-passphrase (32-char alnum).""" + result = _generate_garm_secrets() + assert "jwt-secret" in result + assert "db-passphrase" in result + assert len(result["jwt-secret"]) == 64 + assert all(c in "0123456789abcdef" for c in result["jwt-secret"]) + assert len(result["db-passphrase"]) == 32 + valid_chars = string.ascii_letters + string.digits + assert all(c in valid_chars for c in result["db-passphrase"]) + + +def test_generate_garm_secrets_produces_unique_values(): + """Two calls return different secrets.""" + first = _generate_garm_secrets() + second = _generate_garm_secrets() + assert first["jwt-secret"] != second["jwt-secret"] + assert first["db-passphrase"] != second["db-passphrase"] + + +def test_render_garm_toml_with_configurator_providers(): + """ + arrange: Two Configurator units provided provider configs. + act: render_garm_toml is called with providers list. + assert: Two [[provider]] blocks are rendered with correct names and + config_file paths pointing to provider TOML files. The + returned provider_files dict contains the provider TOML and + clouds.yaml for each provider. + """ + toml_content, provider_files = render_garm_toml( + jwt_secret="test-secret", + db_passphrase="a" * 32, + postgresql_config=_DEFAULT_PG_CONFIG, + providers=_PROVIDERS, + ) + parsed = tomllib.loads(toml_content) + assert len(parsed["provider"]) == 2 + + p0 = parsed["provider"][0] + assert p0["name"] == "garm-configurator-0" + assert p0["external"]["provider_executable"] == "/usr/local/bin/garm-provider-openstack" + assert p0["external"]["config_file"] == "/etc/garm/provider-garm-configurator-0.toml" + assert "environment_variables" not in p0["external"] + + p1 = parsed["provider"][1] + assert p1["name"] == "garm-configurator-1" + assert p1["external"]["config_file"] == "/etc/garm/provider-garm-configurator-1.toml" + + # Verify provider_files contains the expected paths and content. + provider_toml_0 = provider_files["/etc/garm/provider-garm-configurator-0.toml"] + assert 'network_id = "net1"' in provider_toml_0 + assert 'cloud = "garm-configurator-0"' in provider_toml_0 + + clouds_yaml_0 = provider_files["/etc/garm/clouds-garm-configurator-0.yaml"] + clouds_0 = yaml.safe_load(clouds_yaml_0) + assert clouds_0["clouds"]["garm-configurator-0"]["auth"]["username"] == "admin1" + assert clouds_0["clouds"]["garm-configurator-0"]["auth"]["password"] == "pass1" + assert clouds_0["clouds"]["garm-configurator-0"]["region_name"] == "RegionOne" + + clouds_yaml_1 = provider_files["/etc/garm/clouds-garm-configurator-1.yaml"] + clouds_1 = yaml.safe_load(clouds_yaml_1) + assert clouds_1["clouds"]["garm-configurator-1"]["auth"]["username"] == "admin2" + assert clouds_1["clouds"]["garm-configurator-1"]["auth"]["password"] == "pass2" + + +def test_build_provider_list_returns_default_when_empty(): + """ + arrange: An empty list is passed. + act: _build_provider_list is called. + assert: Returns the default single-entry list with "openstack" provider. + """ + entries, files = _build_provider_list([]) + assert len(entries) == 1 + assert entries[0]["name"] == "openstack" + assert files == {} + + +def test_build_provider_list_password_in_clouds_yaml(): + """ + arrange: Two provider configs with passwords. + act: _build_provider_list is called. + assert: Both providers have their password rendered in clouds.yaml. + The password is resolved from Juju secrets in + _get_configurator_provider_configs before reaching + _build_provider_list, so it's always available as plaintext. + """ + entries, provider_files = _build_provider_list(_PROVIDERS) + assert len(entries) == 2 + + clouds_yaml_0 = provider_files["/etc/garm/clouds-garm-configurator-0.yaml"] + clouds_0 = yaml.safe_load(clouds_yaml_0) + assert clouds_0["clouds"]["garm-configurator-0"]["auth"]["password"] == "pass1" + assert clouds_0["clouds"]["garm-configurator-0"]["auth"]["username"] == "admin1" + assert clouds_0["clouds"]["garm-configurator-0"]["region_name"] == "RegionOne" + + clouds_yaml_1 = provider_files["/etc/garm/clouds-garm-configurator-1.yaml"] + clouds_1 = yaml.safe_load(clouds_yaml_1) + assert clouds_1["clouds"]["garm-configurator-1"]["auth"]["password"] == "pass2" + + assert entries[0]["external"]["config_file"] == "/etc/garm/provider-garm-configurator-0.toml" + assert entries[1]["external"]["config_file"] == "/etc/garm/provider-garm-configurator-1.toml" + + +def test_render_clouds_yaml_quotes_special_chars(): + """ + arrange: A password containing YAML-significant characters (colon, hash). + act: _render_clouds_yaml is called (via _build_provider_list). + assert: The resulting clouds.yaml is valid YAML and the password + parses correctly (not truncated or misinterpreted). + """ + providers = [ + { + "unit_name": "special-provider", + "auth_url": "https://keystone.example.com:5000/v3", + "username": "admin", + "password": "p@ss:w0rd#123", + "project_name": "proj", + "user_domain_name": "Default", + "project_domain_name": "Default", + "region_name": "RegionOne", + "network": "net1", + }, + ] + _, provider_files = _build_provider_list(providers) + clouds_yaml = provider_files["/etc/garm/clouds-special-provider.yaml"] + parsed = yaml.safe_load(clouds_yaml) + assert parsed["clouds"]["special-provider"]["auth"]["password"] == "p@ss:w0rd#123" + + +def test_generate_admin_password_meets_garm_policy(): + """ + arrange: No setup required. + act: Call _generate_admin_password(). + assert: The result satisfies GARM's strong-password requirements. + """ + password = _generate_admin_password() + assert len(password) >= 12 + assert any(c.isupper() for c in password) + assert any(c.islower() for c in password) + assert any(c.isdigit() for c in password) + symbols = set(password) - set(string.ascii_letters + string.digits) + assert len(symbols) > 0 + + +def test_generate_admin_password_produces_unique_values(): + """ + arrange: No setup required. + act: Call _generate_admin_password() twice. + assert: The two passwords are different. + """ + assert _generate_admin_password() != _generate_admin_password() + + +def test_proxy_environment_happy_path(): + """ + arrange: All three JUJU_CHARM_* proxy vars are set in the environment. + act: Call _proxy_environment(). + assert: Returns both lower- and upper-case variants for each variable + with the expected values. + """ + env_vars = { + "JUJU_CHARM_HTTP_PROXY": "http://proxy.example.com:3128", + "JUJU_CHARM_HTTPS_PROXY": "https://proxy.example.com:3129", + "JUJU_CHARM_NO_PROXY": "localhost,127.0.0.1", + } + with patch.dict(os.environ, env_vars, clear=True): + result = _proxy_environment() + + assert result["http_proxy"] == "http://proxy.example.com:3128" + assert result["HTTP_PROXY"] == "http://proxy.example.com:3128" + assert result["https_proxy"] == "https://proxy.example.com:3129" + assert result["HTTPS_PROXY"] == "https://proxy.example.com:3129" + assert result["no_proxy"] == "localhost,127.0.0.1" + assert result["NO_PROXY"] == "localhost,127.0.0.1" + assert len(result) == 6 + + +@pytest.mark.parametrize( + "env_vars,expected_keys", + [ + # Empty string values are dropped entirely. + ( + { + "JUJU_CHARM_HTTP_PROXY": "", + "JUJU_CHARM_HTTPS_PROXY": "", + "JUJU_CHARM_NO_PROXY": "", + }, + [], + ), + # Whitespace-only values are stripped and treated as empty. + ( + { + "JUJU_CHARM_HTTP_PROXY": " ", + "JUJU_CHARM_HTTPS_PROXY": "\t", + "JUJU_CHARM_NO_PROXY": " ", + }, + [], + ), + # Nothing set → empty result. + ({}, []), + # Only http_proxy set → only that pair is present. + ( + {"JUJU_CHARM_HTTP_PROXY": "http://proxy.example.com:3128"}, + ["http_proxy", "HTTP_PROXY"], + ), + ], + ids=["all-empty", "all-whitespace", "nothing-set", "only-http"], +) +def test_proxy_environment_edge_cases(env_vars: dict, expected_keys: list): + """ + arrange: Various incomplete or empty JUJU_CHARM_* configurations. + act: Call _proxy_environment(). + assert: Only keys for non-empty values appear; empty/whitespace values + are omitted. + """ + with patch.dict(os.environ, env_vars, clear=True): + result = _proxy_environment() + + assert set(result.keys()) == set(expected_keys) + + +def test_render_garm_toml_with_proxy_var_names(): + """ + arrange: Two provider configs and a non-empty proxy_var_names list. + act: Call render_garm_toml() with proxy_var_names. + assert: Every provider's external dict contains an environment_variables + key equal to the supplied list. + """ + proxy_var_names = [ + "HTTP_PROXY", + "HTTPS_PROXY", + "NO_PROXY", + "http_proxy", + "https_proxy", + "no_proxy", + ] + toml_content, _ = render_garm_toml( + jwt_secret="test-secret", + db_passphrase="a" * 32, + postgresql_config=_DEFAULT_PG_CONFIG, + providers=_PROVIDERS, + proxy_var_names=proxy_var_names, + ) + parsed = tomllib.loads(toml_content) + + for provider in parsed["provider"]: + assert provider["external"]["environment_variables"] == proxy_var_names + + +def test_render_garm_toml_no_proxy_var_names_omits_key(): + """ + arrange: Provider configs but no proxy_var_names (default None). + act: Call render_garm_toml() without proxy_var_names. + assert: environment_variables is absent from every provider external dict + (preserves existing behaviour; the existing assertion in + test_render_garm_toml_with_configurator_providers also covers this). + """ + toml_content, _ = render_garm_toml( + jwt_secret="test-secret", + db_passphrase="a" * 32, + postgresql_config=_DEFAULT_PG_CONFIG, + providers=_PROVIDERS[:1], + ) + parsed = tomllib.loads(toml_content) + + for provider in parsed["provider"]: + assert "environment_variables" not in provider["external"] + + +def test_render_garm_toml_default_provider_applies_proxy_var_names(): + """ + arrange: No configurator providers but a non-empty proxy_var_names list. + act: Call render_garm_toml() with providers=None and proxy_var_names. + assert: The default openstack provider forwards the proxy var names via + environment_variables (consistent with the per-provider path). + """ + proxy_var_names = ["http_proxy", "HTTP_PROXY"] + toml_content, _ = render_garm_toml( + jwt_secret="test-secret", + db_passphrase="a" * 32, + postgresql_config=_DEFAULT_PG_CONFIG, + proxy_var_names=proxy_var_names, + ) + parsed = tomllib.loads(toml_content) + + assert parsed["provider"][0]["external"]["environment_variables"] == proxy_var_names diff --git a/charms/garm/tox.toml b/charms/garm/tox.toml index 69cb5161..13902aa0 100644 --- a/charms/garm/tox.toml +++ b/charms/garm/tox.toml @@ -34,7 +34,13 @@ commands = [["pyright"]] [env.unit] description = "Run unit tests" -deps = ["pytest", "coverage[toml]", "tomli; python_version < '3.11'", "-r requirements.txt"] +deps = [ + "pytest", + "coverage[toml]", + "ops-scenario==8.8.0", + "tomli; python_version < '3.11'", + "-r requirements.txt", +] commands = [ [ "coverage", From c2537adf64d4c72263710c1fd78974b7c8499471 Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Wed, 15 Jul 2026 05:32:20 +0000 Subject: [PATCH 2/3] test(garm): add arrange/act/assert docstrings to the toml tests The seven helper tests moved out of test_charm.py predate the AAA docstring convention and carried single-line summaries across unchanged. State the why in the assert line where the requirement isn't obvious from the code, rather than restating the assertion. --- charms/garm/tests/unit/test_garm_toml.py | 45 ++++++++++++++++++++---- 1 file changed, 38 insertions(+), 7 deletions(-) diff --git a/charms/garm/tests/unit/test_garm_toml.py b/charms/garm/tests/unit/test_garm_toml.py index 9284a94d..5865d73b 100644 --- a/charms/garm/tests/unit/test_garm_toml.py +++ b/charms/garm/tests/unit/test_garm_toml.py @@ -76,7 +76,12 @@ def _render(**overrides) -> dict: def test_render_garm_toml_postgresql_backend(): - """The [database] section uses postgresql backend with correct fields.""" + """ + arrange: A postgresql connection config. + act: Call render_garm_toml(). + assert: The [database] section selects the postgresql backend and carries every + connection field, with no sqlite3 section left behind. + """ parsed = _render( postgresql_config={ "username": "garm", @@ -98,7 +103,12 @@ def test_render_garm_toml_postgresql_backend(): def test_render_garm_toml_passphrase_in_database_section(): - """The passphrase appears in the [database] section.""" + """ + arrange: A 32-character database passphrase. + act: Call render_garm_toml(). + assert: The passphrase is rendered into the [database] section, where GARM reads it + to encrypt provider credentials at rest. + """ passphrase = "b" * 32 parsed = _render(db_passphrase=passphrase) assert parsed["database"]["passphrase"] == passphrase @@ -108,7 +118,11 @@ def test_render_garm_toml_passphrase_in_database_section(): "sslmode", ["disable", "allow", "prefer", "require", "verify-ca", "verify-full"] ) def test_render_garm_toml_sslmode_propagated(sslmode: str): - """The sslmode value is propagated to the postgresql section.""" + """ + arrange: Each sslmode libpq accepts. + act: Call render_garm_toml(). + assert: The value reaches the postgresql section verbatim, unvalidated by the charm. + """ pg_config = {**_DEFAULT_PG_CONFIG, "sslmode": sslmode} parsed = _render(postgresql_config=pg_config) assert parsed["database"]["postgresql"]["sslmode"] == sslmode @@ -136,13 +150,21 @@ def test_render_garm_toml_sslmode_propagated(sslmode: str): ], ) def test_render_garm_toml_section_fields(section: str, key: str, value, kwargs: dict): - """Config sections reflect the given parameters.""" + """ + arrange: A field in each of the apiserver, jwt_auth and metrics sections. + act: Call render_garm_toml(). + assert: The rendered value matches, pinning the defaults GARM is served with. + """ parsed = _render(**kwargs) assert parsed[section][key] == value def test_render_garm_toml_provider_section(): - """The [[provider]] section has the OpenStack provider binary.""" + """ + arrange: No configurator-supplied providers. + act: Call render_garm_toml(). + assert: A single external provider points at the OpenStack binary shipped in the rock. + """ parsed = _render() assert len(parsed["provider"]) == 1 provider = parsed["provider"][0] @@ -152,7 +174,12 @@ def test_render_garm_toml_provider_section(): def test_generate_garm_secrets_returns_jwt_and_passphrase(): - """Returns a dict with jwt-secret (64-char hex) and db-passphrase (32-char alnum).""" + """ + arrange: No setup required. + act: Call _generate_garm_secrets(). + assert: The JWT secret is 64 hex characters and the passphrase 32 alphanumerics — + GARM rejects a passphrase of any other length. + """ result = _generate_garm_secrets() assert "jwt-secret" in result assert "db-passphrase" in result @@ -164,7 +191,11 @@ def test_generate_garm_secrets_returns_jwt_and_passphrase(): def test_generate_garm_secrets_produces_unique_values(): - """Two calls return different secrets.""" + """ + arrange: No setup required. + act: Call _generate_garm_secrets() twice. + assert: The two results differ, so the values are drawn randomly rather than fixed. + """ first = _generate_garm_secrets() second = _generate_garm_secrets() assert first["jwt-secret"] != second["jwt-secret"] From 281aec2e3960215058eba31157286af1e603c6ec Mon Sep 17 00:00:00 2001 From: Christopher Bartz Date: Wed, 15 Jul 2026 05:32:20 +0000 Subject: [PATCH 3/3] docs(agents): document the AAA docstring convention The convention was only written down in STYLE.md, which is scoped to Go and spells it as comments, so Python tests had nothing to point at and the nearest neighbour is an unreliable guide: planner-operator and garm follow it, garm-configurator and webhook-gateway-operator do not. Also drop the paas_charm fixture details from the garm AGENTS.md. They described framework internals at a pinned version (the derived go_secret_key databag key, its literal status strings), which the AGENTS.md checker cannot validate and a dependency bump would silently invalidate. The test file already carries them as comments at the point of use; keep the pointer and the guidance that outlives the version. --- AGENTS.md | 16 ++++++++++++++++ charms/garm/AGENTS.md | 15 +++++---------- 2 files changed, 21 insertions(+), 10 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 6eacb6b3..7a505949 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -72,6 +72,22 @@ For **`garm-configurator`** (plain `ops`): - New unit tests use **Scenario** (`scenario.Context` / `State` / `Relation` / `Secret`), not `Harness` — see `charms/garm-configurator/tests/unit/`. +- **DO** give every Python test an arrange/act/assert docstring — the convention `STYLE.md` + documents for Go (`// Arrange:` comments), spelled as a docstring: + ```python + def test_leader_creates_garm_secrets(): + """ + arrange: A leader unit with no GARM secrets yet. + act: Run install. + assert: Both labelled secrets are created, so other units can fetch them by label. + """ + ``` + State the *why* in the assert line where it isn't obvious ("so other units can fetch them + by label"), not just the mechanical result. For a `parametrize`d test, write one docstring + covering the cases — **DON'T** write one per case. +- **DO** fix a missing AAA docstring on any test you move or edit. It's followed unevenly + (`planner-operator` and `garm` yes; `garm-configurator` and `webhook-gateway-operator` not + yet), so imitating the nearest neighbour is not a reliable guide. - Integration tests live in the shared `charms/tests/integration/`. ## 12-factor divergences from the canonical charm-engineer guidance diff --git a/charms/garm/AGENTS.md b/charms/garm/AGENTS.md index 5fa5dbf0..929fe916 100644 --- a/charms/garm/AGENTS.md +++ b/charms/garm/AGENTS.md @@ -35,16 +35,11 @@ charm conventions; this file lists only what's specific to `garm`. rather than blocking). The port is pinned in the `_workload_config` property. - Tests: unit in `tests/unit/`; integration via `tox -e garm-integration` (`charms/tests/integration/test_garm.py`). -- **Scenario tests of this paas charm need three fixtures** that a plain `ops` charm doesn't - (all provided by the state builder in `tests/unit/test_charm.py`) — `ops-scenario` expands the - `go-framework` extension itself, so the `app` container and `secret-storage` peer exist - without a packed charm: - - **DO** give the `app` container a base layer declaring the rock's `go` service; the - framework's layer overrides that service and can't find it otherwise. - - **DO** put `go_secret_key` in the `secret-storage` peer app databag — `is_ready()` gates - on it and reports `Waiting for peer integration` without it. - - **DO** publish postgresql connection data; `postgresql` is `optional: false`, so the - framework blocks with `missing integrations: postgresql` before `restart()` runs. +- **Scenario tests** — `ops-scenario` expands the `go-framework` extension itself, so this charm + is testable from source with no packed charm. Reaching `ActiveStatus` needs a few fixtures a + plain `ops` charm doesn't; the state builder in `tests/unit/test_charm.py` assembles them, each + annotated with the `paas_charm` behaviour that demands it. **DO** start from it rather than + building a `State` from scratch. - **DON'T** rebuild the mock-`self` pattern (`GarmCharm.restart(MagicMock())`) these tests replaced: it can't see what the charm pushed or replanned, so it hid real bugs. Stub only GARM's HTTP surface (the `garm_api` fixture) and assert on the output `State`.