Ensure that Fact#command does not take global arguments

pyinfra/api/arguments.py

@@ -9,6 +9,7 @@
     List,
     Mapping,
     Optional,
+    Type,
     TypeVar,
     Union,
     cast,
@@ -228,6 +229,11 @@ class AllArguments(ConnectorArguments, MetaArguments, ExecutionArguments):
     pass
 
 
+def all_global_arguments() -> List[tuple[str, Type]]:
+    """Return all global arguments and their types."""
+    return list(get_type_hints(AllArguments).items())
+
+
 all_argument_meta: dict[str, ArgumentMeta] = {
     **auth_argument_meta,
     **shell_argument_meta,
@@ -305,7 +311,7 @@ def pop_global_arguments(
     arguments: dict[str, Any] = {}
     found_keys: list[str] = []
 
-    for key, type_ in get_type_hints(AllArguments).items():
+    for key, type_ in all_global_arguments():
         if keys_to_check and key not in keys_to_check:
             continue
 

pyinfra/api/facts.py

@@ -10,6 +10,7 @@
 
 from __future__ import annotations
 
+import inspect
 import re
 from inspect import getcallargs
 from socket import error as socket_error, timeout as timeout_error
@@ -32,7 +33,7 @@
 
 from pyinfra import logger
 from pyinfra.api import StringCommand
-from pyinfra.api.arguments import pop_global_arguments
+from pyinfra.api.arguments import all_global_arguments, pop_global_arguments
 from pyinfra.api.util import (
     get_kwargs_str,
     log_error_or_warning,
@@ -76,6 +77,17 @@ def __init_subclass__(cls) -> None:
         module_name = cls.__module__.replace("pyinfra.facts.", "")
         cls.name = f"{module_name}.{cls.__name__}"
 
+        # Check that fact's `command` method does not inadvertently take a global
+        # argument, most commonly `name`.
+        if hasattr(cls, "command") and callable(cls.command):
+            command_args = set(inspect.signature(cls.command).parameters.keys())
+            global_args = set([name for name, _ in all_global_arguments()])
+            command_global_args = command_args & global_args
+
+            if len(command_global_args) > 0:
+                names = ", ".join(command_global_args)
+                raise TypeError(f"{cls.name}'s arguments {names} are reserved for global arguments")
+
     @staticmethod
     def default() -> T:
         """

pyinfra/facts/deb.py

@@ -1,4 +1,5 @@
 import re
+import shlex
 
 from pyinfra.api import FactBase
 
@@ -54,8 +55,10 @@ class DebPackage(FactBase):
 
     requires_command = "dpkg"
 
-    def command(self, name):
-        return "! test -e {0} && (dpkg -s {0} 2>/dev/null || true) || dpkg -I {0}".format(name)
+    def command(self, package):
+        return "! test -e {0} && (dpkg -s {0} 2>/dev/null || true) || dpkg -I {0}".format(
+            shlex.quote(package)
+        )
 
     def process(self, output):
         data = {}

pyinfra/facts/pacman.py

@@ -1,3 +1,5 @@
+import shlex
+
 from pyinfra.api import FactBase
 
 from .util.packaging import parse_packages
@@ -21,9 +23,9 @@ class PacmanUnpackGroup(FactBase):
 
     default = list
 
-    def command(self, name):
+    def command(self, package):
         # Accept failure here (|| true) for invalid/unknown packages
-        return 'pacman -S --print-format "%n" {0} || true'.format(name)
+        return 'pacman -S --print-format "%n" {0} || true'.format(shlex.quote(package))
 
     def process(self, output):
         return output

pyinfra/facts/rpm.py

@@ -1,4 +1,5 @@
 import re
+import shlex
 
 from pyinfra.api import FactBase
 
@@ -19,7 +20,7 @@ class RpmPackages(FactBase):
         }
     """
 
-    command = 'rpm --queryformat "{0}" -qa'.format(rpm_query_format)
+    command = "rpm --queryformat {0} -qa".format(shlex.quote(rpm_query_format))
     requires_command = "rpm"
 
     default = dict
@@ -42,12 +43,12 @@ class RpmPackage(FactBase):
 
     requires_command = "rpm"
 
-    def command(self, name):
+    def command(self, package):
         return (
-            'rpm --queryformat "{0}" -q {1} || '
+            "rpm --queryformat {0} -q {1} || "
             "! test -e {1} || "
-            'rpm --queryformat "{0}" -qp {1} 2> /dev/null'
-        ).format(rpm_query_format, name)
+            "rpm --queryformat {0} -qp {1} 2> /dev/null"
+        ).format(shlex.quote(rpm_query_format), shlex.quote(package))
 
     def process(self, output):
         for line in output:
@@ -69,11 +70,11 @@ class RpmPackageProvides(FactBase):
     requires_command = "repoquery"
 
     @staticmethod
-    def command(name):
+    def command(package):
         # Accept failure here (|| true) for invalid/unknown packages
-        return 'repoquery --queryformat "{0}" --whatprovides {1} || true'.format(
-            rpm_query_format,
-            name,
+        return "repoquery --queryformat {0} --whatprovides {1} || true".format(
+            shlex.quote(rpm_query_format),
+            shlex.quote(package),
         )
 
     @staticmethod

pyinfra/operations/apt.py

@@ -233,18 +233,14 @@ def deb(src, present=True, force=False):
         src = temp_filename
 
     # Check for file .deb information (if file is present)
-    info = host.get_fact(DebPackage, name=src)
+    info = host.get_fact(DebPackage, package=src)
     current_packages = host.get_fact(DebPackages)
 
     exists = False
 
     # We have deb info! Check against installed packages
-    if info:
-        if (
-            info["name"] in current_packages
-            and info.get("version") in current_packages[info["name"]]
-        ):
-            exists = True
+    if info and info.get("version") in current_packages.get(info.get("name"), {}):
+        exists = True
 
     # Package does not exist and we want?
     if present:

pyinfra/operations/dnf.py

@@ -207,8 +207,5 @@ def packages(
         upgrade_command="dnf update -y",
         version_join="=",
         latest=latest,
-        expand_package_fact=lambda package: host.get_fact(
-            RpmPackageProvides,
-            name=package,
-        ),
+        expand_package_fact=lambda package: host.get_fact(RpmPackageProvides, package=package),
     )

pyinfra/operations/pacman.py

@@ -75,8 +75,5 @@ def packages(
         present,
         install_command="pacman --noconfirm -S",
         uninstall_command="pacman --noconfirm -R",
-        expand_package_fact=lambda package: host.get_fact(
-            PacmanUnpackGroup,
-            name=package,
-        ),
+        expand_package_fact=lambda package: host.get_fact(PacmanUnpackGroup, package=package),
     )

pyinfra/operations/util/packaging.py

@@ -171,12 +171,12 @@ def ensure_rpm(state, host, files, source, present, package_manager_command):
         source = temp_filename
 
     # Check for file .rpm information
-    info = host.get_fact(RpmPackage, name=source)
+    info = host.get_fact(RpmPackage, package=source)
     exists = False
 
     # We have info!
     if info:
-        current_package = host.get_fact(RpmPackage, name=info["name"])
+        current_package = host.get_fact(RpmPackage, package=info["name"])
         if current_package and current_package["version"] == info["version"]:
             exists = True
 

pyinfra/operations/yum.py

@@ -207,8 +207,5 @@ def packages(
         upgrade_command="yum update -y",
         version_join="=",
         latest=latest,
-        expand_package_fact=lambda package: host.get_fact(
-            RpmPackageProvides,
-            name=package,
-        ),
+        expand_package_fact=lambda package: host.get_fact(RpmPackageProvides, package=package),
     )

tests/facts/rpm.RpmPackage/package.json

@@ -1,6 +1,6 @@
 {
     "arg": "somepackage",
-    "command": "rpm --queryformat \"%{NAME} %{VERSION}-%{RELEASE}\\n\" -q somepackage || ! test -e somepackage || rpm --queryformat \"%{NAME} %{VERSION}-%{RELEASE}\\n\" -qp somepackage 2> /dev/null",
+    "command": "rpm --queryformat '%{NAME} %{VERSION}-%{RELEASE}\\n' -q somepackage || ! test -e somepackage || rpm --queryformat '%{NAME} %{VERSION}-%{RELEASE}\\n' -qp somepackage 2> /dev/null",
     "requires_command": "rpm",
     "output": [
         "pydash 3.48.0"

tests/facts/rpm.RpmPackageProvides/package.json

@@ -1,6 +1,6 @@
 {
     "arg": "vim",
-    "command": "repoquery --queryformat \"%{NAME} %{VERSION}-%{RELEASE}\\n\" --whatprovides vim || true",
+    "command": "repoquery --queryformat '%{NAME} %{VERSION}-%{RELEASE}\\n' --whatprovides vim || true",
     "requires_command": "repoquery",
     "output": [
         "vim-enhanced 8.0.1763-15.el8"

tests/facts/rpm.RpmPackages/packages.json

@@ -1,5 +1,5 @@
 {
-    "command": "rpm --queryformat \"%{NAME} %{VERSION}-%{RELEASE}\\n\" -qa",
+    "command": "rpm --queryformat '%{NAME} %{VERSION}-%{RELEASE}\\n' -qa",
     "requires_command": "rpm",
     "output": [
         "pydash 3.48.0"

tests/operations/apt.deb/add_existing.json

@@ -6,7 +6,7 @@
             "path=_tempfile_": ""
         },
         "deb.DebPackage": {
-            "name=_tempfile_": {
+            "package=_tempfile_": {
                 "name": "test",
                 "version": 0
             }

tests/operations/apt.deb/remove_existing.json

@@ -9,7 +9,7 @@
             "path=_tempfile_": ""
         },
         "deb.DebPackage": {
-            "name=_tempfile_": {
+            "package=_tempfile_": {
                 "name": "test",
                 "version": 0
             }

tests/operations/dnf.packages/add_packages.json

@@ -9,9 +9,9 @@
             "something": [""]
         },
         "rpm.RpmPackageProvides": {
-            "name=git": null,
-            "name=python-devel": null,
-            "name=something": null
+            "package=git": null,
+            "package=python-devel": null,
+            "package=something": null
         }
     },
     "commands": [

tests/operations/dnf.packages/install_with_args.json

@@ -6,7 +6,7 @@
     "facts": {
         "rpm.RpmPackages": {},
         "rpm.RpmPackageProvides": {
-            "name=docker-ce": null
+            "package=docker-ce": null
         }
     },
     "commands": [

tests/operations/dnf.packages/install_with_nobest.json

@@ -6,7 +6,7 @@
     "facts": {
         "rpm.RpmPackages": {},
         "rpm.RpmPackageProvides": {
-            "name=docker-ce": null
+            "package=docker-ce": null
         }
     },
     "commands": [

tests/operations/dnf.packages/noop_add_package_alias.json

@@ -5,7 +5,7 @@
             "vim-enhanced": ["abc"]
         },
         "rpm.RpmPackageProvides": {
-            "name=vim": [["vim-enhanced", "abc"]]
+            "package=vim": [["vim-enhanced", "abc"]]
         }
     },
     "commands": [],

tests/operations/dnf.packages/remove_package.json

@@ -14,9 +14,9 @@
             "vim-common": ["8.2.1081-1.fc32"]
         },
         "rpm.RpmPackageProvides": {
-            "name=git": null,
-            "name=vim-enhanced": null,
-            "name=vim-common": null
+            "package=git": null,
+            "package=vim-enhanced": null,
+            "package=vim-common": null
         }
     },
     "commands": [

tests/operations/dnf.packages/uninstall_with_args.json

@@ -11,7 +11,7 @@
             "git": [""]
         },
         "rpm.RpmPackageProvides": {
-            "name=git": null
+            "package=git": null
         }
     },
     "commands": [

tests/operations/dnf.rpm/add.json

@@ -3,11 +3,11 @@
     "facts": {
         "rpm.RpmPackages": {},
         "rpm.RpmPackage": {
-            "name=something.rpm": {
+            "package=something.rpm": {
                 "name": "something",
                 "version": "abc"
             },
-            "name=something": null
+            "package=something": null
         }
     },
     "commands": [

tests/operations/dnf.rpm/add_existing.json

@@ -2,11 +2,11 @@
     "args": ["something.rpm"],
     "facts": {
         "rpm.RpmPackage": {
-            "name=something.rpm": {
+            "package=something.rpm": {
                 "name": "something",
                 "version": "1.1"
             },
-            "name=something": {
+            "package=something": {
                 "version": "1.1"
             }
         }

tests/operations/dnf.rpm/add_existing_upgrade.json

@@ -2,11 +2,11 @@
     "args": ["something.rpm"],
     "facts": {
         "rpm.RpmPackage": {
-            "name=something.rpm": {
+            "package=something.rpm": {
                 "name": "something",
                 "version": "1.1"
             },
-            "name=something": {
+            "package=something": {
                 "version": "1.0"
             }
         }

tests/operations/dnf.rpm/add_url.json

@@ -6,7 +6,7 @@
             "path=_tempfile_.rpm": null
         },
         "rpm.RpmPackage": {
-            "name=_tempfile_.rpm": null
+            "package=_tempfile_.rpm": null
         },
         "server.Which": {
             "command=curl": "yes"

tests/operations/dnf.rpm/remove.json

@@ -5,11 +5,11 @@
     },
     "facts": {
         "rpm.RpmPackage": {
-            "name=something.rpm": {
+            "package=something.rpm": {
                 "name": "something",
                 "version": "1.1"
             },
-            "name=something": {
+            "package=something": {
                 "version": "1.1"
             }
         }

tests/operations/pacman.packages/add_packages.json

@@ -2,8 +2,8 @@
     "args": [["curl", "xorg-fonts"]],
     "facts": {
         "pacman.PacmanUnpackGroup": {
-            "name=curl": ["curl"],
-            "name=xorg-fonts": ["xorg-font-util", "xorg-fonts-encodings"]
+            "package=curl": ["curl"],
+            "package=xorg-fonts": ["xorg-font-util", "xorg-fonts-encodings"]
         },
         "pacman.PacmanPackages": {}
     },