Update ansible-test collection inventory handling. (#61031)

* Update ansible-test collection inventory handling.

- The `windows-integration` command now supports the `--inventory` option.
- The incomplete support for host_vars and group_vars directories has been removed.
- The incomplete support for an inventory directory has been removed.
- The inventory specified by `--inventory` can now reside outside the install and content roots.
- Using `ansible_ssh_private_key_file` with `--docker` or `--remote` results in a warning about the combination being unsupported and likely to fail.

* Fix config handling.

* Fix payload handling of ssh keys.

* Disable pylint no-self-use rule for ansible-test.

* De-duplicate payload paths.

test/lib/ansible_test/_data/sanity/pylint/config/ansible-test

@@ -10,6 +10,7 @@ disable=
     too-many-nested-blocks,
     too-many-return-statements,
     too-many-statements,
+    no-self-use,
     unused-import,  # pylint does not understand PEP 484 type hints
     consider-using-dict-comprehension,  # requires Python 2.6, which we still support
     consider-using-set-comprehension,  # requires Python 2.6, which we still support

test/lib/ansible_test/_internal/ansible_util.py

@@ -26,6 +26,7 @@ from .util_common import (
 from .config import (
     PosixIntegrationConfig,
     EnvironmentConfig,
+    CommonConfig,
 )
 
 from .data import (
@@ -50,7 +51,7 @@ def ansible_environment(args, color=True, ansible_config=None):
 
     if not ansible_config:
         # use the default empty configuration unless one has been provided
-        ansible_config = os.path.join(ANSIBLE_TEST_DATA_ROOT, 'ansible.cfg')
+        ansible_config = args.get_ansible_config()
 
     if not args.explain and not os.path.exists(ansible_config):
         raise ApplicationError('Configuration not found: %s' % ansible_config)

test/lib/ansible_test/_internal/cli.py

@@ -390,6 +390,10 @@ def parse_args():
                                      action='append',
                                      help='windows version').completer = complete_windows
 
+    windows_integration.add_argument('--inventory',
+                                     metavar='PATH',
+                                     help='path to inventory used for tests')
+
     units = subparsers.add_parser('units',
                                   parents=[test],
                                   help='unit tests')

test/lib/ansible_test/_internal/cloud/__init__.py

@@ -314,14 +314,12 @@ class CloudProvider(CloudBase):
 
         atexit.register(self.cleanup)
 
-    # pylint: disable=locally-disabled, no-self-use
     def get_remote_ssh_options(self):
         """Get any additional options needed when delegating tests to a remote instance via SSH.
         :rtype: list[str]
         """
         return []
 
-    # pylint: disable=locally-disabled, no-self-use
     def get_docker_run_options(self):
         """Get any additional options needed when delegating tests to a docker container.
         :rtype: list[str]

test/lib/ansible_test/_internal/config.py

@@ -14,6 +14,7 @@ from .util import (
     generate_pip_command,
     get_docker_completion,
     ApplicationError,
+    INTEGRATION_DIR_RELATIVE,
 )
 
 from .util_common import (
@@ -244,6 +245,17 @@ class IntegrationConfig(TestConfig):
         if self.list_targets:
             self.explain = True
 
+    def get_ansible_config(self):  # type: () -> str
+        """Return the path to the Ansible config for the given config."""
+        ansible_config_relative_path = os.path.join(INTEGRATION_DIR_RELATIVE, '%s.cfg' % self.command)
+        ansible_config_path = os.path.join(data_context().content.root, ansible_config_relative_path)
+
+        if not os.path.exists(ansible_config_path):
+            # use the default empty configuration unless one has been provided
+            ansible_config_path = super(IntegrationConfig, self).get_ansible_config()
+
+        return ansible_config_path
+
 
 class PosixIntegrationConfig(IntegrationConfig):
     """Configuration for the posix integration command."""
@@ -265,6 +277,7 @@ class WindowsIntegrationConfig(IntegrationConfig):
         super(WindowsIntegrationConfig, self).__init__(args, 'windows-integration')
 
         self.windows = args.windows  # type: t.List[str]
+        self.inventory = args.inventory  # type: str
 
         if self.windows:
             self.allow_destructive = True

test/lib/ansible_test/_internal/core_ci.py

@@ -578,8 +578,13 @@ class SshKey:
 
             def ssh_key_callback(files):  # type: (t.List[t.Tuple[str, str]]) -> None
                 """Add the SSH keys to the payload file list."""
-                files.append((key, key_dst))
-                files.append((pub, pub_dst))
+                if data_context().content.collection:
+                    working_path = data_context().content.collection.directory
+                else:
+                    working_path = ''
+
+                files.append((key, os.path.join(working_path, key_dst)))
+                files.append((pub, os.path.join(working_path, pub_dst)))
 
             data_context().register_payload_callback(ssh_key_callback)
 

test/lib/ansible_test/_internal/delegation.py

@@ -23,6 +23,8 @@ from .config import (
     TestConfig,
     EnvironmentConfig,
     IntegrationConfig,
+    WindowsIntegrationConfig,
+    NetworkIntegrationConfig,
     ShellConfig,
     SanityConfig,
     UnitsConfig,
@@ -558,6 +560,11 @@ def filter_options(args, argv, options, exclude, require):
             '--base-branch': 1,
         })
 
+    if isinstance(args, (NetworkIntegrationConfig, WindowsIntegrationConfig)):
+        options.update({
+            '--inventory': 1,
+        })
+
     remaining = 0
 
     for arg in argv:

test/lib/ansible_test/_internal/executor.py

@@ -129,7 +129,11 @@ from .integration import (
     integration_test_environment,
     integration_test_config_file,
     setup_common_temp_dir,
-    VARS_FILE_RELATIVE,
+    INTEGRATION_VARS_FILE_RELATIVE,
+    get_inventory_relative_path,
+    INTEGRATION_DIR_RELATIVE,
+    check_inventory,
+    delegate_inventory,
 )
 
 from .data import (
@@ -375,36 +379,37 @@ def command_posix_integration(args):
     """
     :type args: PosixIntegrationConfig
     """
-    filename = 'test/integration/inventory'
+    inventory_relative_path = get_inventory_relative_path(args)
+    inventory_path = os.path.join(ANSIBLE_TEST_DATA_ROOT, os.path.basename(inventory_relative_path))
 
     all_targets = tuple(walk_posix_integration_targets(include_hidden=True))
     internal_targets = command_integration_filter(args, all_targets)
-    command_integration_filtered(args, internal_targets, all_targets, filename)
+    command_integration_filtered(args, internal_targets, all_targets, inventory_path)
 
 
 def command_network_integration(args):
     """
     :type args: NetworkIntegrationConfig
     """
-    default_filename = 'test/integration/inventory.networking'
-    template_path = os.path.join(ANSIBLE_TEST_CONFIG_ROOT, os.path.basename(default_filename)) + '.template'
+    inventory_relative_path = get_inventory_relative_path(args)
+    template_path = os.path.join(ANSIBLE_TEST_CONFIG_ROOT, os.path.basename(inventory_relative_path)) + '.template'
 
     if args.inventory:
-        filename = os.path.join('test/integration', args.inventory)
+        inventory_path = os.path.join(data_context().content.root, INTEGRATION_DIR_RELATIVE, args.inventory)
     else:
-        filename = default_filename
-
-    if not args.explain and not args.platform and not os.path.exists(filename):
-        if args.inventory:
-            filename = os.path.abspath(filename)
+        inventory_path = os.path.join(data_context().content.root, inventory_relative_path)
 
+    if not args.explain and not args.platform and not os.path.isfile(inventory_path):
         raise ApplicationError(
             'Inventory not found: %s\n'
             'Use --inventory to specify the inventory path.\n'
             'Use --platform to provision resources and generate an inventory file.\n'
-            'See also inventory template: %s' % (filename, template_path)
+            'See also inventory template: %s' % (inventory_path, template_path)
         )
 
+    check_inventory(args, inventory_path)
+    delegate_inventory(args, inventory_path)
+
     all_targets = tuple(walk_network_integration_targets(include_hidden=True))
     internal_targets = command_integration_filter(args, all_targets, init_callback=network_init)
     instances = []  # type: t.List[WrappedThread]
@@ -432,16 +437,16 @@ def command_network_integration(args):
         remotes = [instance.wait_for_result() for instance in instances]
         inventory = network_inventory(remotes)
 
-        display.info('>>> Inventory: %s\n%s' % (filename, inventory.strip()), verbosity=3)
+        display.info('>>> Inventory: %s\n%s' % (inventory_path, inventory.strip()), verbosity=3)
 
         if not args.explain:
-            with open(filename, 'w') as inventory_fd:
+            with open(inventory_path, 'w') as inventory_fd:
                 inventory_fd.write(inventory)
 
     success = False
 
     try:
-        command_integration_filtered(args, internal_targets, all_targets, filename)
+        command_integration_filtered(args, internal_targets, all_targets, inventory_path)
         success = True
     finally:
         if args.remote_terminate == 'always' or (args.remote_terminate == 'success' and success):
@@ -562,11 +567,24 @@ def command_windows_integration(args):
     """
     :type args: WindowsIntegrationConfig
     """
-    filename = 'test/integration/inventory.winrm'
-    template_path = os.path.join(ANSIBLE_TEST_CONFIG_ROOT, os.path.basename(filename)) + '.template'
+    inventory_relative_path = get_inventory_relative_path(args)
+    template_path = os.path.join(ANSIBLE_TEST_CONFIG_ROOT, os.path.basename(inventory_relative_path)) + '.template'
+
+    if args.inventory:
+        inventory_path = os.path.join(data_context().content.root, INTEGRATION_DIR_RELATIVE, args.inventory)
+    else:
+        inventory_path = os.path.join(data_context().content.root, inventory_relative_path)
+
+    if not args.explain and not args.windows and not os.path.isfile(inventory_path):
+        raise ApplicationError(
+            'Inventory not found: %s\n'
+            'Use --inventory to specify the inventory path.\n'
+            'Use --windows to provision resources and generate an inventory file.\n'
+            'See also inventory template: %s' % (inventory_path, template_path)
+        )
 
-    if not args.explain and not args.windows and not os.path.isfile(filename):
-        raise ApplicationError('Use the --windows option or provide an inventory file (see %s).' % template_path)
+    check_inventory(args, inventory_path)
+    delegate_inventory(args, inventory_path)
 
     all_targets = tuple(walk_windows_integration_targets(include_hidden=True))
     internal_targets = command_integration_filter(args, all_targets, init_callback=windows_init)
@@ -594,10 +612,10 @@ def command_windows_integration(args):
         remotes = [instance.wait_for_result() for instance in instances]
         inventory = windows_inventory(remotes)
 
-        display.info('>>> Inventory: %s\n%s' % (filename, inventory.strip()), verbosity=3)
+        display.info('>>> Inventory: %s\n%s' % (inventory_path, inventory.strip()), verbosity=3)
 
         if not args.explain:
-            with open(filename, 'w') as inventory_fd:
+            with open(inventory_path, 'w') as inventory_fd:
                 inventory_fd.write(inventory)
 
         use_httptester = args.httptester and any('needs/httptester/' in target.aliases for target in internal_targets)
@@ -661,7 +679,7 @@ def command_windows_integration(args):
     success = False
 
     try:
-        command_integration_filtered(args, internal_targets, all_targets, filename, pre_target=pre_target,
+        command_integration_filtered(args, internal_targets, all_targets, inventory_path, pre_target=pre_target,
                                      post_target=post_target)
         success = True
     finally:
@@ -832,7 +850,7 @@ def command_integration_filter(args,  # type: TIntegrationConfig
 
     cloud_init(args, internal_targets)
 
-    vars_file_src = os.path.join(data_context().content.root, VARS_FILE_RELATIVE)
+    vars_file_src = os.path.join(data_context().content.root, INTEGRATION_VARS_FILE_RELATIVE)
 
     if os.path.exists(vars_file_src):
         def integration_config_callback(files):  # type: (t.List[t.Tuple[str, str]]) -> None
@@ -845,7 +863,7 @@ def command_integration_filter(args,  # type: TIntegrationConfig
             else:
                 working_path = ''
 
-            files.append((vars_file_src, os.path.join(working_path, VARS_FILE_RELATIVE)))
+            files.append((vars_file_src, os.path.join(working_path, INTEGRATION_VARS_FILE_RELATIVE)))
 
         data_context().register_payload_callback(integration_config_callback)
 

test/lib/ansible_test/_internal/integration/__init__.py

@@ -8,12 +8,15 @@ import os
 import shutil
 import tempfile
 
+from .. import types as t
+
 from ..target import (
     analyze_integration_target_dependencies,
     walk_integration_targets,
 )
 
 from ..config import (
+    IntegrationConfig,
     NetworkIntegrationConfig,
     PosixIntegrationConfig,
     WindowsIntegrationConfig,
@@ -28,8 +31,8 @@ from ..util import (
     MODE_DIRECTORY,
     MODE_DIRECTORY_WRITE,
     MODE_FILE,
-    ANSIBLE_ROOT,
-    ANSIBLE_TEST_DATA_ROOT,
+    INTEGRATION_DIR_RELATIVE,
+    INTEGRATION_VARS_FILE_RELATIVE,
     to_bytes,
 )
 
@@ -53,9 +56,6 @@ from ..data import (
     data_context,
 )
 
-INTEGRATION_DIR_RELATIVE = 'test/integration'
-VARS_FILE_RELATIVE = os.path.join(INTEGRATION_DIR_RELATIVE, 'integration_config.yml')
-
 
 def setup_common_temp_dir(args, path):
     """
@@ -134,27 +134,78 @@ def get_files_needed(target_dependencies):
     return files_needed
 
 
+def check_inventory(args, inventory_path):  # type: (IntegrationConfig, str) -> None
+    """Check the given inventory for issues."""
+    if args.docker or args.remote:
+        if os.path.exists(inventory_path):
+            with open(inventory_path) as inventory_file:
+                inventory = inventory_file.read()
+
+            if 'ansible_ssh_private_key_file' in inventory:
+                display.warning('Use of "ansible_ssh_private_key_file" in inventory with the --docker or --remote option is unsupported and will likely fail.')
+
+
+def get_inventory_relative_path(args):  # type: (IntegrationConfig) -> str
+    """Return the inventory path used for the given integration configuration relative to the content root."""
+    inventory_names = {
+        PosixIntegrationConfig: 'inventory',
+        WindowsIntegrationConfig: 'inventory.winrm',
+        NetworkIntegrationConfig: 'inventory.networking',
+    }  # type: t.Dict[t.Type[IntegrationConfig], str]
+
+    return os.path.join(INTEGRATION_DIR_RELATIVE, inventory_names[type(args)])
+
+
+def delegate_inventory(args, inventory_path_src):  # type: (IntegrationConfig, str) -> None
+    """Make the given inventory available during delegation."""
+    if isinstance(args, PosixIntegrationConfig):
+        return
+
+    def inventory_callback(files):  # type: (t.List[t.Tuple[str, str]]) -> None
+        """
+        Add the inventory file to the payload file list.
+        This will preserve the file during delegation even if it is ignored or is outside the content and install roots.
+        """
+        if data_context().content.collection:
+            working_path = data_context().content.collection.directory
+        else:
+            working_path = ''
+
+        inventory_path = os.path.join(working_path, get_inventory_relative_path(args))
+
+        if os.path.isfile(inventory_path_src) and os.path.relpath(inventory_path_src, data_context().content.root) != inventory_path:
+            originals = [item for item in files if item[1] == inventory_path]
+
+            if originals:
+                for original in originals:
+                    files.remove(original)
+
+                display.warning('Overriding inventory file "%s" with "%s".' % (inventory_path, inventory_path_src))
+            else:
+                display.notice('Sourcing inventory file "%s" from "%s".' % (inventory_path, inventory_path_src))
+
+            files.append((inventory_path_src, inventory_path))
+
+    data_context().register_payload_callback(inventory_callback)
+
+
 @contextlib.contextmanager
-def integration_test_environment(args, target, inventory_path):
+def integration_test_environment(args, target, inventory_path_src):
     """
     :type args: IntegrationConfig
     :type target: IntegrationTarget
-    :type inventory_path: str
+    :type inventory_path_src: str
     """
-    ansible_config_relative = os.path.join(INTEGRATION_DIR_RELATIVE, '%s.cfg' % args.command)
-    ansible_config_src = os.path.join(data_context().content.root, ansible_config_relative)
-
-    if not os.path.exists(ansible_config_src):
-        # use the default empty configuration unless one has been provided
-        ansible_config_src = os.path.join(ANSIBLE_TEST_DATA_ROOT, 'ansible.cfg')
+    ansible_config_src = args.get_ansible_config()
+    ansible_config_relative = os.path.relpath(ansible_config_src, data_context().content.root)
 
     if args.no_temp_workdir or 'no/temp_workdir/' in target.aliases:
         display.warning('Disabling the temp work dir is a temporary debugging feature that may be removed in the future without notice.')
 
         integration_dir = os.path.join(data_context().content.root, INTEGRATION_DIR_RELATIVE)
-        inventory_path = os.path.join(data_context().content.root, inventory_path)
+        inventory_path = inventory_path_src
         ansible_config = ansible_config_src
-        vars_file = os.path.join(data_context().content.root, VARS_FILE_RELATIVE)
+        vars_file = os.path.join(data_context().content.root, INTEGRATION_VARS_FILE_RELATIVE)
 
         yield IntegrationEnvironment(integration_dir, inventory_path, ansible_config, vars_file)
         return
@@ -177,13 +228,8 @@ def integration_test_environment(args, target, inventory_path):
     try:
         display.info('Preparing temporary directory: %s' % temp_dir, verbosity=2)
 
-        inventory_names = {
-            PosixIntegrationConfig: 'inventory',
-            WindowsIntegrationConfig: 'inventory.winrm',
-            NetworkIntegrationConfig: 'inventory.networking',
-        }
-
-        inventory_name = inventory_names[type(args)]
+        inventory_relative_path = get_inventory_relative_path(args)
+        inventory_path = os.path.join(temp_dir, inventory_relative_path)
 
         cache = IntegrationCache(args)
 
@@ -194,12 +240,12 @@ def integration_test_environment(args, target, inventory_path):
         integration_dir = os.path.join(temp_dir, INTEGRATION_DIR_RELATIVE)
         ansible_config = os.path.join(temp_dir, ansible_config_relative)
 
-        vars_file_src = os.path.join(data_context().content.root, VARS_FILE_RELATIVE)
-        vars_file = os.path.join(temp_dir, VARS_FILE_RELATIVE)
+        vars_file_src = os.path.join(data_context().content.root, INTEGRATION_VARS_FILE_RELATIVE)
+        vars_file = os.path.join(temp_dir, INTEGRATION_VARS_FILE_RELATIVE)
 
         file_copies = [
             (ansible_config_src, ansible_config),
-            (os.path.join(ANSIBLE_ROOT, inventory_path), os.path.join(integration_dir, inventory_name)),
+            (inventory_path_src, inventory_path),
         ]
 
         if os.path.exists(vars_file_src):
@@ -212,17 +258,6 @@ def integration_test_environment(args, target, inventory_path):
             for target in target_dependencies
         ]
 
-        inventory_dir = os.path.dirname(inventory_path)
-
-        host_vars_dir = os.path.join(inventory_dir, 'host_vars')
-        group_vars_dir = os.path.join(inventory_dir, 'group_vars')
-
-        if os.path.isdir(host_vars_dir):
-            directory_copies.append((host_vars_dir, os.path.join(integration_dir, os.path.basename(host_vars_dir))))
-
-        if os.path.isdir(group_vars_dir):
-            directory_copies.append((group_vars_dir, os.path.join(integration_dir, os.path.basename(group_vars_dir))))
-
         directory_copies = sorted(set(directory_copies))
         file_copies = sorted(set(file_copies))
 
@@ -242,8 +277,6 @@ def integration_test_environment(args, target, inventory_path):
                 make_dirs(os.path.dirname(file_dst))
                 shutil.copy2(file_src, file_dst)
 
-        inventory_path = os.path.join(integration_dir, inventory_name)
-
         yield IntegrationEnvironment(integration_dir, inventory_path, ansible_config, vars_file)
     finally:
         if not args.explain:

test/lib/ansible_test/_internal/payload.py

@@ -17,7 +17,6 @@ from .config import (
 
 from .util import (
     display,
-    ANSIBLE_ROOT,
     ANSIBLE_SOURCE_ROOT,
     remove_tree,
     is_subdir,
@@ -79,19 +78,11 @@ def create_payload(args, dst_path):  # type: (CommonConfig, str) -> None
             files.extend((os.path.join(data_context().content.root, path), os.path.join(data_context().content.collection.directory, path))
                          for path in data_context().content.all_files())
 
-    # these files need to be migrated to the ansible-test data directory
-    hack_files_to_keep = (
-        'test/integration/inventory',
-    )
-
-    # temporary solution to include files not yet present in the ansible-test data directory
-    files.extend([(os.path.join(ANSIBLE_ROOT, path), path) for path in hack_files_to_keep])
-
     for callback in data_context().payload_callbacks:
         callback(files)
 
     # maintain predictable file order
-    files = sorted(files)
+    files = sorted(set(files))
 
     display.info('Creating a payload archive containing %d files...' % len(files), verbosity=1)
 

test/lib/ansible_test/_internal/util.py

@@ -80,6 +80,9 @@ if not os.path.exists(ANSIBLE_LIB_ROOT):
 ANSIBLE_TEST_DATA_ROOT = os.path.join(ANSIBLE_TEST_ROOT, '_data')
 ANSIBLE_TEST_CONFIG_ROOT = os.path.join(ANSIBLE_TEST_ROOT, 'config')
 
+INTEGRATION_DIR_RELATIVE = 'test/integration'
+INTEGRATION_VARS_FILE_RELATIVE = os.path.join(INTEGRATION_DIR_RELATIVE, 'integration_config.yml')
+
 # Modes are set to allow all users the same level of access.
 # This permits files to be used in tests that change users.
 # The only exception is write access to directories for the user creating them.

test/lib/ansible_test/_internal/util_common.py

@@ -50,6 +50,10 @@ class CommonConfig:
 
         self.cache = {}
 
+    def get_ansible_config(self):  # type: () -> str
+        """Return the path to the Ansible config for the given config."""
+        return os.path.join(ANSIBLE_TEST_DATA_ROOT, 'ansible.cfg')
+
 
 @contextlib.contextmanager
 def named_temporary_file(args, prefix, suffix, directory, content):