From b914fb9789dddf252d8f5ef88ca9bd5018a1a898 Mon Sep 17 00:00:00 2001 From: Kevin Veen-Birkenbach Date: Wed, 3 Dec 2025 19:18:39 +0100 Subject: [PATCH] Refactor CLI filters: rename --ignore to --exclude and update all related logic and tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Updated CLI argument parsing to use --exclude instead of --ignore. - Adjusted help texts, comments, and error messages accordingly. - Updated role filtering logic and references (include → exclude). - Added new unit tests for parse_roles_list(), filter_inventory_by_include(), and filter_inventory_by_ignore(). - Improved wording and consistency in docstrings. This change is part of the refactoring required for the Ansible 2.18 → 2.20 upgrade, ensuring naming clarity and avoiding confusion with Python's 'ignore' semantics. Conversation reference: https://chatgpt.com/share/69307ef2-1fb4-800f-a2ec-d56020019269 --- cli/create/inventory.py | 24 ++++----- tests/unit/cli/create/test_inventory.py | 70 +++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 12 deletions(-) diff --git a/cli/create/inventory.py b/cli/create/inventory.py index 92c6129a..f7b2a587 100644 --- a/cli/create/inventory.py +++ b/cli/create/inventory.py @@ -11,8 +11,8 @@ This subcommand: host containing all invokable applications. 2. Optionally filters the resulting groups by: - --include: only listed application_ids are kept - - --ignore: listed application_ids are removed - - --roles: legacy include filter (used only if --include/--ignore are not set) + - --exclude: listed application_ids are removed + - --roles: legacy include filter (used only if --include/--exclude are not set) 3. Merges the generated inventory into an existing inventory file, without deleting or overwriting unrelated entries. 4. Ensures `host_vars/.yml` exists and stores base settings such as: @@ -192,7 +192,7 @@ def parse_roles_list(raw_roles: Optional[List[str]]) -> Optional[Set[str]]: Parse a list of IDs supplied on the CLI. Supports: --include web-app-nextcloud web-app-mastodon --include web-app-nextcloud,web-app-mastodon - Same logic is reused for --ignore and --roles. + Same logic is reused for --exclude and --roles. """ if not raw_roles: return None @@ -768,7 +768,7 @@ def main(argv: Optional[List[str]] = None) -> None: nargs="+", help=( "Optional legacy list of application_ids to include. " - "Used only if neither --include nor --ignore is specified. " + "Used only if neither --include nor --exclude is specified. " "Supports comma-separated values as well." ), ) @@ -777,11 +777,11 @@ def main(argv: Optional[List[str]] = None) -> None: nargs="+", help=( "Only include the listed application_ids in the inventory. " - "Mutually exclusive with --ignore." + "Mutually exclusive with --exclude." ), ) parser.add_argument( - "--ignore", + "--exclude", nargs="+", help=( "Exclude the listed application_ids from the inventory. " @@ -814,14 +814,14 @@ def main(argv: Optional[List[str]] = None) -> None: args = parser.parse_args(argv) - # Parse include/ignore/roles lists + # Parse include/exclude/roles lists include_filter = parse_roles_list(args.include) - ignore_filter = parse_roles_list(args.ignore) + ignore_filter = parse_roles_list(args.exclude) roles_filter = parse_roles_list(args.roles) - # Enforce mutual exclusivity: only one of --include / --ignore may be used + # Enforce mutual exclusivity: only one of --include / --exclude may be used if include_filter and ignore_filter: - fatal("Options --include and --ignore are mutually exclusive. Please use only one of them.") + fatal("Options --include and --exclude are mutually exclusive. Please use only one of them.") project_root = detect_project_root() roles_dir = Path(args.roles_dir) if args.roles_dir else (project_root / "roles") @@ -849,7 +849,7 @@ def main(argv: Optional[List[str]] = None) -> None: try: vault_password_file.chmod(0o600) except PermissionError: - # Best-effort; ignore if chmod is not allowed + # Best-effort; exclude if chmod is not allowed pass else: print(f"[INFO] Using existing vault password file: {vault_password_file}") @@ -866,7 +866,7 @@ def main(argv: Optional[List[str]] = None) -> None: project_root=project_root, ) - # 2) Apply filters: include → ignore → legacy roles + # 2) Apply filters: include → exclude → legacy roles if include_filter: print(f"[INFO] Including only application_ids: {', '.join(sorted(include_filter))}") dyn_inv = filter_inventory_by_include(dyn_inv, include_filter) diff --git a/tests/unit/cli/create/test_inventory.py b/tests/unit/cli/create/test_inventory.py index 951aef63..8fefd97a 100644 --- a/tests/unit/cli/create/test_inventory.py +++ b/tests/unit/cli/create/test_inventory.py @@ -14,6 +14,9 @@ from cli.create.inventory import ( # type: ignore merge_inventories, ensure_host_vars_file, ensure_become_password, + parse_roles_list, + filter_inventory_by_include, + filter_inventory_by_ignore, ) from ruamel.yaml import YAML @@ -21,6 +24,73 @@ from ruamel.yaml.comments import CommentedMap class TestCreateInventory(unittest.TestCase): + + def test_parse_roles_list_supports_commas_and_spaces(self): + """ + parse_roles_list() should: + - return None for None or empty input, + - split comma separated values, + - strip whitespace, + - deduplicate values. + """ + self.assertIsNone(parse_roles_list(None)) + self.assertIsNone(parse_roles_list([])) + + roles = parse_roles_list([ + "web-app-nextcloud, web-app-matomo", + "web-app-phpmyadmin", + "web-app-nextcloud", # duplicate + ]) + + self.assertIsInstance(roles, set) + self.assertEqual( + roles, + {"web-app-nextcloud", "web-app-matomo", "web-app-phpmyadmin"}, + ) + + def test_filter_inventory_by_include_keeps_only_selected_groups(self): + """ + filter_inventory_by_include() must: + - keep only groups whose names are in the include set, + - preserve the original group data for kept groups, + - remove groups not listed in the include set. + """ + original_inventory = { + "all": { + "children": { + "web-app-nextcloud": { + "hosts": {"localhost": {"ansible_host": "127.0.0.1"}}, + }, + "web-app-matomo": { + "hosts": {"localhost": {"ansible_host": "127.0.0.2"}}, + }, + "web-app-phpmyadmin": { + "hosts": {"localhost": {"ansible_host": "127.0.0.3"}}, + }, + } + } + } + + include_set = {"web-app-nextcloud", "web-app-phpmyadmin"} + + filtered = filter_inventory_by_include(original_inventory, include_set) + children = filtered["all"]["children"] + + # Only the included groups must remain + self.assertIn("web-app-nextcloud", children) + self.assertIn("web-app-phpmyadmin", children) + self.assertNotIn("web-app-matomo", children) + + # The content of the kept groups must be identical to the original + self.assertEqual( + children["web-app-nextcloud"], + original_inventory["all"]["children"]["web-app-nextcloud"], + ) + self.assertEqual( + children["web-app-phpmyadmin"], + original_inventory["all"]["children"]["web-app-phpmyadmin"], + ) + def test_merge_inventories_adds_host_and_preserves_existing(self): """ merge_inventories() must: