diff --git a/cli/administration/inventory/validate/applications.py b/cli/administration/inventory/validate/applications.py index f3d6672b0a..8d3a3d1ed6 100644 --- a/cli/administration/inventory/validate/applications.py +++ b/cli/administration/inventory/validate/applications.py @@ -3,13 +3,10 @@ from __future__ import annotations from utils.manager.credential_key import OVERRIDE_SECTION, SECRETS_KEY -from utils.roles.applications.dashboard import DASHBOARD_SERVICE, DISPLAY_PATH from utils.roles.custom import CUSTOM_KEY, SOURCE_KEY, is_custom from .keys import recursive_keys -_DASHBOARD_PATH = f"services.{DASHBOARD_SERVICE}" - def _declares_custom_role(app_id: str, key: str) -> bool: return is_custom(app_id) and key.split(".", 1)[0] in (CUSTOM_KEY, SOURCE_KEY) @@ -25,11 +22,7 @@ def compare_application_keys(applications, application_defaults, source, variant ``custom`` and ``source`` of a custom role are the inventory's declaration of where the role comes from, not role settings, so no - role default carries them. - - ``services.dashboard.display`` is legal wherever the role contract - declares ``services.dashboard``: absent means shown, so no role - default carries it.""" + role default carries them.""" variants = variants or {} errs: list[str] = [] for app_id, conf in applications.items(): @@ -39,8 +32,6 @@ def compare_application_keys(applications, application_defaults, source, variant legal_keys = recursive_keys(application_defaults[app_id]) for variant in variants.get(app_id, []): legal_keys |= recursive_keys(variant) - if _DASHBOARD_PATH in legal_keys: - legal_keys.add(DISPLAY_PATH) for key in recursive_keys(conf): if key == SECRETS_KEY or key.startswith(OVERRIDE_SECTION): continue diff --git a/docs/contributing/design/role/services/base.md b/docs/contributing/design/role/services/base.md index e9f3351c7b..037f65dd4c 100644 --- a/docs/contributing/design/role/services/base.md +++ b/docs/contributing/design/role/services/base.md @@ -76,22 +76,21 @@ Rules: ## Dashboard Card Display 👀 -`display: false` on a consumer's `dashboard` binding drops its dashboard card, its Apps menu entry and its tile-reachability target. -The switch is read from the merged applications, so an inventory MAY set it per host without touching the role: +`enabled: false` on a consumer's `dashboard` binding takes the role out of the dashboard: no card, no Apps menu entry, no tile-reachability target and no dashboard origin in its CSP `frame-ancestors`. +The flag is read from the merged applications, so an inventory MAY set it per host without touching the role: ```yaml applications: web-app-gitea: services: dashboard: - display: false + enabled: false ``` Rules: -- `display:` MUST be a bool; any other value fails the card lookup. -- Without `display:` the card is shown, so a role does not declare `display: true`. -- The inventory validator accepts `services.dashboard.display` for every role whose contract declares `services.dashboard`. +- `enabled:` MUST render to a boolean; any other value fails the card lookup. +- A `web-app-*` role without a `dashboard` binding still gets a card. ## Service Discovery 🔍 diff --git a/docs/contributing/design/role/services/layout.md b/docs/contributing/design/role/services/layout.md index e24ead744f..2305c63c54 100644 --- a/docs/contributing/design/role/services/layout.md +++ b/docs/contributing/design/role/services/layout.md @@ -379,7 +379,7 @@ video: https://youtu.be/3jcYJGQgenI?si=FDmoMSrAb9_WvviC | `video` | string | Upstream demo / overview video URL. | The lint (`tests/lint/ansible/roles/meta/test_info.py`) rejects any other top-level key so the file does not become a dumping ground. -Whether the dashboard shows the role's card is not set here but in `services.dashboard.display`, see [Dashboard Card Display](base.md#dashboard-card-display-). +Whether the dashboard shows the role's card is not set here but in `services.dashboard.enabled`, see [Dashboard Card Display](base.md#dashboard-card-display-). ### Optionality diff --git a/plugins/lookup/roles_with_service.py b/plugins/lookup/roles_with_service.py index 980af57963..979c2877f0 100644 --- a/plugins/lookup/roles_with_service.py +++ b/plugins/lookup/roles_with_service.py @@ -9,9 +9,7 @@ can tell embeddable cards from those that must open in a new tab. For the ``dashboard`` service a role MAY name ``services.dashboard.card: ``; its entry then carries ``domains.canonical.`` and ``lookup('canonical_url', , )`` instead of the primary domain, so -the tile target matches the card ``docker_cards`` renders. A role whose -``services.dashboard.display`` is false renders no card, so it is no tile -target either. +the tile target matches the card ``docker_cards`` renders. A role keeps declaring the service for inventory completeness but can opt out of this consumer-target list by setting @@ -52,11 +50,7 @@ from ansible.plugins.loader import lookup_loader from ansible.plugins.lookup import LookupBase from utils.domains.primary_domain import get_primary_domain -from utils.roles.applications.dashboard import ( - DASHBOARD_SERVICE, - card_is_displayed, - card_target_key, -) +from utils.roles.applications.dashboard import DASHBOARD_SERVICE, card_target_key from utils.roles.applications.mcp import ( DEFAULT_MCP_TRANSPORT, derive_allowed_consumers, @@ -208,8 +202,6 @@ class LookupModule(LookupBase): continue if get_entity_name(str(role_id)) == service_name: continue - if lists_dashboard_cards and not card_is_displayed(str(role_id), block): - continue canonical = _resolve_canonical_domain(str(role_id), app_config) if not canonical and direction is None: continue diff --git a/roles/web-app-dashboard/lookup_plugins/docker_cards.py b/roles/web-app-dashboard/lookup_plugins/docker_cards.py index 2620bf3e8e..a0d9a83d36 100644 --- a/roles/web-app-dashboard/lookup_plugins/docker_cards.py +++ b/roles/web-app-dashboard/lookup_plugins/docker_cards.py @@ -31,7 +31,7 @@ class LookupModule(LookupBase): - Reads the title from the role's README.md (the first H1 line) - Retrieves the description from galaxy_info.description in meta/main.yml - Retrieves the icon class from meta/info.yml.logo.class - - Skips the role when services.dashboard.display in applications + - Skips the role when services.dashboard.enabled in applications is false - Retrieves the tags from galaxy_info.galaxy_tags - Builds the URL from the primary domain, or from the domains.canonical diff --git a/tests/unit/python/cli/administration/inventory/validate/test_applications.py b/tests/unit/python/cli/administration/inventory/validate/test_applications.py index 625357bab1..3f9ed263c2 100644 --- a/tests/unit/python/cli/administration/inventory/validate/test_applications.py +++ b/tests/unit/python/cli/administration/inventory/validate/test_applications.py @@ -105,33 +105,6 @@ class TestCompareApplicationKeys(unittest.TestCase): defaults = {"web-app-cst-demo": {"services": {"port": 8080}}} self.assertEqual(compare_application_keys(apps, defaults, "src"), []) - def test_dashboard_display_is_legal_where_dashboard_is_declared(self): - apps = {"app1": {"services": {"dashboard": {"display": False}}}} - defaults = {"app1": {"services": {"dashboard": {"enabled": False}}}} - self.assertEqual(compare_application_keys(apps, defaults, "src"), []) - - def test_dashboard_display_is_legal_where_a_variant_declares_dashboard(self): - apps = {"app1": {"services": {"dashboard": {"display": False}}}} - defaults = {"app1": {"services": {}}} - variants = {"app1": [{"services": {"dashboard": {"enabled": True}}}]} - self.assertEqual(compare_application_keys(apps, defaults, "src", variants), []) - - def test_dashboard_display_without_a_dashboard_binding_is_flagged(self): - apps = {"app1": {"services": {"dashboard": {"display": False}}}} - defaults = {"app1": {"services": {"port": 8080}}} - result = compare_application_keys(apps, defaults, "src") - self.assertEqual( - sorted(err.rsplit(": ", 1)[1] for err in result), - ["services.dashboard", "services.dashboard.display"], - ) - - def test_other_undeclared_dashboard_keys_stay_flagged(self): - apps = {"app1": {"services": {"dashboard": {"dispaly": False}}}} - defaults = {"app1": {"services": {"dashboard": {"enabled": False}}}} - result = compare_application_keys(apps, defaults, "src") - self.assertEqual(len(result), 1) - self.assertIn("services.dashboard.dispaly", result[0]) - def test_declaration_keys_on_an_in_tree_role_are_flagged(self): apps = {"app1": {"custom": True, "source": {"url": "https://x/y.git"}}} defaults = {"app1": {"services": {}}} diff --git a/tests/unit/python/plugins/lookup/test_docker_cards.py b/tests/unit/python/plugins/lookup/test_docker_cards.py index d28ee70528..f6620d8695 100644 --- a/tests/unit/python/plugins/lookup/test_docker_cards.py +++ b/tests/unit/python/plugins/lookup/test_docker_cards.py @@ -382,40 +382,34 @@ logo: fake_variables["group_names"] = ["portfolio", "other"] return fake_variables - def test_display_false_skips_the_card(self): + def test_enabled_false_skips_the_card(self): fake_variables = self._base_fake_variables() fake_variables["applications"]["portfolio"]["services"]["dashboard"][ - "display" + "enabled" ] = False result = self._run_lookup(LookupModule(), fake_variables) self.assertEqual(result, [[]]) - def test_absent_display_shows_the_card(self): + def test_a_role_without_a_dashboard_binding_shows_the_card(self): fake_variables = self._base_fake_variables() - self.assertNotIn( - "display", - fake_variables["applications"]["portfolio"]["services"]["dashboard"], - ) + fake_variables["applications"]["portfolio"]["services"] = {} result = self._run_lookup(LookupModule(), fake_variables) self.assertEqual([c["title"] for c in result[0]], ["Portfolio Application"]) - def test_display_true_shows_the_card(self): + def test_enabled_true_shows_the_card(self): fake_variables = self._base_fake_variables() - fake_variables["applications"]["portfolio"]["services"]["dashboard"][ - "display" - ] = True result = self._run_lookup(LookupModule(), fake_variables) self.assertEqual([c["title"] for c in result[0]], ["Portfolio Application"]) - def test_display_false_hides_only_its_own_card(self): + def test_enabled_false_hides_only_its_own_card(self): fake_variables = self._two_role_variables() - fake_variables["applications"]["other"]["services"]["dashboard"]["display"] = ( + fake_variables["applications"]["other"]["services"]["dashboard"]["enabled"] = ( False ) @@ -423,7 +417,7 @@ logo: self.assertEqual([c["title"] for c in result[0]], ["Portfolio Application"]) - def test_both_cards_render_without_display(self): + def test_both_cards_render_when_both_are_enabled(self): fake_variables = self._two_role_variables() result = self._run_lookup(LookupModule(), fake_variables) @@ -444,11 +438,11 @@ logo: self.assertEqual([c["title"] for c in result[0]], ["Portfolio Application"]) - def test_non_bool_display_fails_loudly(self): + def test_non_boolean_enabled_fails_loudly(self): fake_variables = self._base_fake_variables() fake_variables["applications"]["portfolio"]["services"]["dashboard"][ - "display" - ] = "false" + "enabled" + ] = "maybe" with self.assertRaisesRegex(AnsibleError, "portfolio"): self._run_lookup(LookupModule(), fake_variables) diff --git a/tests/unit/python/plugins/lookup/test_roles_with_service.py b/tests/unit/python/plugins/lookup/test_roles_with_service.py index c4ac664f07..d75c1fc3c0 100644 --- a/tests/unit/python/plugins/lookup/test_roles_with_service.py +++ b/tests/unit/python/plugins/lookup/test_roles_with_service.py @@ -613,37 +613,6 @@ class RolesWithServiceLookupTests(unittest.TestCase): ("matrix.example.com", "https://web-app-chat.example.com"), ) - def test_dashboard_display_false_drops_the_tile_target(self): - applications = { - "web-app-hidden": { - "services": { - "dashboard": {"enabled": True, "shared": True, "display": False} - }, - "domains": {"canonical": ["hidden.example.com"]}, - }, - "web-app-shown": { - "services": {"dashboard": {"enabled": True, "shared": True}}, - "domains": {"canonical": ["shown.example.com"]}, - }, - } - result = self._run(["dashboard"], applications)[0] - self.assertEqual([r["id"] for r in result], ["web-app-shown"]) - - def test_dashboard_display_true_keeps_the_tile_target(self): - applications = self._card_target_role("dashboard", display=True) - result = self._run(["dashboard"], applications)[0] - self.assertEqual([r["id"] for r in result], ["web-app-chat"]) - - def test_dashboard_non_bool_display_fails_loudly(self): - applications = self._card_target_role("dashboard", display="false") - with self.assertRaisesRegex(AnsibleError, "web-app-chat"): - self._run(["dashboard"], applications) - - def test_other_services_ignore_a_display_key(self): - applications = self._card_target_role("prometheus", display=False) - result = self._run(["prometheus"], applications)[0] - self.assertEqual([r["id"] for r in result], ["web-app-chat"]) - if __name__ == "__main__": unittest.main() diff --git a/tests/unit/python/utils/roles/applications/test_dashboard.py b/tests/unit/python/utils/roles/applications/test_dashboard.py index d397ea338f..4b5d942040 100644 --- a/tests/unit/python/utils/roles/applications/test_dashboard.py +++ b/tests/unit/python/utils/roles/applications/test_dashboard.py @@ -44,20 +44,23 @@ class TestCardTargetKey(unittest.TestCase): class TestCardIsDisplayed(unittest.TestCase): def test_a_binding_without_the_key_is_displayed(self): - self.assertTrue(card_is_displayed("web-app-foo", {"enabled": True})) + self.assertTrue(card_is_displayed("web-app-foo", {"shared": True})) def test_a_missing_binding_is_displayed(self): self.assertTrue(card_is_displayed("web-app-foo", None)) - def test_display_false_hides_the_card(self): - self.assertFalse(card_is_displayed("web-app-foo", {"display": False})) + def test_enabled_false_hides_the_card(self): + self.assertFalse(card_is_displayed("web-app-foo", {"enabled": False})) - def test_display_true_shows_the_card(self): - self.assertTrue(card_is_displayed("web-app-foo", {"display": True})) + def test_enabled_true_shows_the_card(self): + self.assertTrue(card_is_displayed("web-app-foo", {"enabled": True})) - def test_a_string_flag_fails_loudly(self): + def test_a_rendered_string_flag_is_read_as_a_boolean(self): + self.assertFalse(card_is_displayed("web-app-foo", {"enabled": "False"})) + + def test_a_non_boolean_flag_fails_loudly(self): with self.assertRaisesRegex(AnsibleError, "web-app-foo"): - card_is_displayed("web-app-foo", {"display": "false"}) + card_is_displayed("web-app-foo", {"enabled": "maybe"}) if __name__ == "__main__": diff --git a/utils/roles/applications/dashboard.py b/utils/roles/applications/dashboard.py index a18163d2b0..a2e31b01d8 100644 --- a/utils/roles/applications/dashboard.py +++ b/utils/roles/applications/dashboard.py @@ -5,8 +5,9 @@ A role's ``services.dashboard`` binding MAY name ``card: ``, a key of its tile-reachability target then point at that domain instead of the role's primary domain. -``display: false`` on the same binding drops the card, the Apps menu entry and -the tile-reachability target. +``enabled: false`` on the same binding takes the application out of the +dashboard: no card and no Apps menu entry, and the tile-reachability targets +and the CSP frame-ancestors already follow the same flag. """ from __future__ import annotations @@ -14,11 +15,12 @@ from __future__ import annotations from typing import Any from ansible.errors import AnsibleError +from ansible.module_utils.parsing.convert_bool import boolean DASHBOARD_SERVICE = "dashboard" CARD_TARGET_KEY = "card" -DISPLAY_KEY = "display" -DISPLAY_PATH = f"services.{DASHBOARD_SERVICE}.{DISPLAY_KEY}" +ENABLED_KEY = "enabled" +ENABLED_PATH = f"services.{DASHBOARD_SERVICE}.{ENABLED_KEY}" def card_is_displayed(application_id: str, dashboard: Any) -> bool: @@ -30,20 +32,22 @@ def card_is_displayed(application_id: str, dashboard: Any) -> bool: applications. Returns: - The ``display`` flag, or ``True`` when the binding sets none. + The ``enabled`` flag, or ``True`` when the role declares no binding or + the binding sets none. Raises: - AnsibleError: the binding sets ``display`` to something other than a - bool. + AnsibleError: the binding sets ``enabled`` to something that is not a + boolean. """ - if not isinstance(dashboard, dict) or DISPLAY_KEY not in dashboard: + if not isinstance(dashboard, dict) or ENABLED_KEY not in dashboard: return True - display = dashboard[DISPLAY_KEY] - if not isinstance(display, bool): + enabled = dashboard[ENABLED_KEY] + try: + return boolean(enabled, strict=True) + except TypeError as exc: raise AnsibleError( - f"{application_id}: {DISPLAY_PATH} must be a bool, got {display!r}" - ) - return display + f"{application_id}: {ENABLED_PATH} must be a boolean, got {enabled!r}" + ) from exc def card_target_key(application_id: str, dashboard: Any) -> str: