Validate inventory files with warnings (#1182)

Co-authored-by: simonhammes <[email protected]>

docs/inventory-data.rst

@@ -8,7 +8,7 @@ By default, pyinfra assumes hosts are SSH servers and the name of the host is us
 Inventory Files
 ---------------
 
-Inventory files contain groups of hosts. Groups are defined as a ``list``. For example, this inventory creates two groups, ``app_servers`` and ``db_servers``:
+Inventory files contain groups of hosts. Groups are defined as a ``list`` or ``tuple``. For example, this inventory creates two groups, ``app_servers`` and ``db_servers``:
 
 .. code:: python
 
@@ -17,19 +17,17 @@ Inventory files contain groups of hosts. Groups are defined as a ``list``. For e
         "app-2.net"
     ]
 
-    db_servers = [
-        "db-1.net",
-        "db-2.net",
-        "db-3.net",
-    ]
+    db_servers = (["db-1.net", "db-2.net", "db-3.net",], {})
 
-If you save this file as ``inventory.py``, you can then use it in when executing pyinfra:
+If you save this file as ``inventory.py``, you can then use it when executing pyinfra:
 
 .. code:: shell
 
     pyinfra inventory.py OPERATIONS...
 
 .. Note::
+    Group names starting with a leading ``_`` are intentionally ignored.
+
     In addition to the groups defined in the inventory, all the hosts are added to a group with the name of the inventory file (eg ``production.py`` becomes ``production``).
 
 Limiting inventory at runtime

pyinfra_cli/inventory.py

@@ -1,7 +1,6 @@
 import socket
 from collections import defaultdict
 from os import listdir, path
-from types import GeneratorType
 from typing import Any, Callable, Dict, List, Optional, Tuple, TypeVar, Union
 
 from pyinfra import logger
@@ -23,18 +22,37 @@ def _is_inventory_group(key: str, value: Any):
     Verify that a module-level variable (key = value) is a valid inventory group.
     """
 
-    if key.startswith("_") or not isinstance(value, (list, tuple, GeneratorType)):
+    if key.startswith("__"):
+        # Ignore __builtins__/__file__
+        return False
+    elif key.startswith("_"):
+        logger.debug(
+            'Ignoring variable "%s" in inventory file since it starts with a leading underscore',
+            key,
+        )
         return False
 
-    # If the group is a tuple of (hosts, data), check the hosts
-    if isinstance(value, tuple):
+    if isinstance(value, list):
+        pass
+    elif isinstance(value, tuple):
+        # If the group is a tuple of (hosts, data), check the hosts
         value = value[0]
+    else:
+        logger.debug(
+            'Ignoring variable "%s" in inventory file since it is not a list or tuple',
+            key,
+        )
+        return False
 
-    # Expand any generators of hosts
-    if isinstance(value, GeneratorType):
-        value = list(value)
+    if not all(isinstance(item, ALLOWED_HOST_TYPES) for item in value):
+        logger.warning(
+            'Ignoring host group "%s". '
+            "Host groups may only contain strings (host) or tuples (host, data).",
+            key,
+        )
+        return False
 
-    return all(isinstance(item, ALLOWED_HOST_TYPES) for item in value)
+    return True
 
 
 def _get_group_data(dirname_or_filename: str):
@@ -258,7 +276,6 @@ def make_inventory_from_files(
         for hosts in groups.values():
             # Groups can be a list of hosts or tuple of (hosts, data)
             hosts = _get_any_tuple_first(hosts)
-
             for host in hosts:
                 # Hosts can be a hostname or tuple of (hostname, data)
                 hostname = _get_any_tuple_first(host)

tests/test_cli/inventories/invalid.py

@@ -0,0 +1,13 @@
+# Variables names with a leading underscore are ignored
+_hosts = ["host-1", "host-2", "host-3"]
+
+# Dictionaries are not supported
+dict_hosts = {
+    "host-1": "hostname",
+}
+
+# Generators are not supported
+generator_hosts = (host for host in ["host-1", "host-2"])
+
+# https://github.com/pyinfra-dev/pyinfra/issues/662
+issue_662 = ["a", "b", ({"foo": "bar"})]

tests/test_cli/test_cli_inventory.py

@@ -64,3 +64,56 @@ def test_load_group_data_file(self):
         assert "leftover_data" in inventory.group_data
         assert inventory.group_data["leftover_data"].get("still_parsed") == "never_used"
         assert inventory.group_data["leftover_data"].get("_global_arg") == "gets_parsed"
+
+    def test_ignores_variables_with_leading_underscore(self):
+        ctx_state.reset()
+        ctx_inventory.reset()
+
+        result = run_cli(
+            path.join("tests", "test_cli", "inventories", "invalid.py"),
+            "exec",
+            "--debug",
+            "--",
+            "echo hi",
+        )
+
+        assert result.exit_code == 0, result.stdout
+        assert (
+            'Ignoring variable "_hosts" in inventory file since it starts with a leading underscore'
+            in result.stdout
+        )
+        assert inventory.hosts == {}
+
+    def test_only_supports_list_and_tuples(self):
+        ctx_state.reset()
+        ctx_inventory.reset()
+
+        result = run_cli(
+            path.join("tests", "test_cli", "inventories", "invalid.py"),
+            "exec",
+            "--debug",
+            "--",
+            "echo hi",
+        )
+
+        assert result.exit_code == 0, result.stdout
+        assert 'Ignoring variable "dict_hosts" in inventory file' in result.stdout, result.stdout
+        assert (
+            'Ignoring variable "generator_hosts" in inventory file' in result.stdout
+        ), result.stdout
+        assert inventory.hosts == {}
+
+    def test_host_groups_may_only_contain_strings_or_tuples(self):
+        ctx_state.reset()
+        ctx_inventory.reset()
+
+        result = run_cli(
+            path.join("tests", "test_cli", "inventories", "invalid.py"),
+            "exec",
+            "--",
+            "echo hi",
+        )
+
+        assert result.exit_code == 0, result.stdout
+        assert 'Ignoring host group "issue_662"' in result.stdout, result.stdout
+        assert inventory.hosts == {}