From 29ac0273d4f421a0919c1e7d7608b2d7fd7a35b4 Mon Sep 17 00:00:00 2001 From: Matt Clay Date: Wed, 21 Aug 2019 12:12:37 -0700 Subject: [PATCH] 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. --- .../ansible_test/_data}/inventory | 0 .../_data/sanity/pylint/config/ansible-test | 1 + .../ansible_test/_internal/ansible_util.py | 3 +- test/lib/ansible_test/_internal/cli.py | 4 + .../ansible_test/_internal/cloud/__init__.py | 2 - test/lib/ansible_test/_internal/config.py | 13 +++ test/lib/ansible_test/_internal/core_ci.py | 9 +- test/lib/ansible_test/_internal/delegation.py | 7 ++ test/lib/ansible_test/_internal/executor.py | 66 +++++++---- .../_internal/integration/__init__.py | 109 ++++++++++++------ test/lib/ansible_test/_internal/payload.py | 11 +- test/lib/ansible_test/_internal/util.py | 3 + .../lib/ansible_test/_internal/util_common.py | 4 + 13 files changed, 155 insertions(+), 77 deletions(-) rename test/{integration => lib/ansible_test/_data}/inventory (100%) diff --git a/test/integration/inventory b/test/lib/ansible_test/_data/inventory similarity index 100% rename from test/integration/inventory rename to test/lib/ansible_test/_data/inventory diff --git a/test/lib/ansible_test/_data/sanity/pylint/config/ansible-test b/test/lib/ansible_test/_data/sanity/pylint/config/ansible-test index 96a19bb8d63..ea40c0bf556 100644 --- a/test/lib/ansible_test/_data/sanity/pylint/config/ansible-test +++ b/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 diff --git a/test/lib/ansible_test/_internal/ansible_util.py b/test/lib/ansible_test/_internal/ansible_util.py index 7b4c1e5470e..f3f3323a021 100644 --- a/test/lib/ansible_test/_internal/ansible_util.py +++ b/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) diff --git a/test/lib/ansible_test/_internal/cli.py b/test/lib/ansible_test/_internal/cli.py index 04d36f5f7a3..48d5affae36 100644 --- a/test/lib/ansible_test/_internal/cli.py +++ b/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') diff --git a/test/lib/ansible_test/_internal/cloud/__init__.py b/test/lib/ansible_test/_internal/cloud/__init__.py index 1e5e304e8f4..f46210b36fe 100644 --- a/test/lib/ansible_test/_internal/cloud/__init__.py +++ b/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] diff --git a/test/lib/ansible_test/_internal/config.py b/test/lib/ansible_test/_internal/config.py index 9390bf1bd8b..024b2890947 100644 --- a/test/lib/ansible_test/_internal/config.py +++ b/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 diff --git a/test/lib/ansible_test/_internal/core_ci.py b/test/lib/ansible_test/_internal/core_ci.py index 82a37eed30b..b623568ee84 100644 --- a/test/lib/ansible_test/_internal/core_ci.py +++ b/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) diff --git a/test/lib/ansible_test/_internal/delegation.py b/test/lib/ansible_test/_internal/delegation.py index 7fdee7c112c..c3c3a81434f 100644 --- a/test/lib/ansible_test/_internal/delegation.py +++ b/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: diff --git a/test/lib/ansible_test/_internal/executor.py b/test/lib/ansible_test/_internal/executor.py index adaa1b1fe70..33e43e6e692 100644 --- a/test/lib/ansible_test/_internal/executor.py +++ b/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 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) + 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) + ) + + 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) diff --git a/test/lib/ansible_test/_internal/integration/__init__.py b/test/lib/ansible_test/_internal/integration/__init__.py index a7054b330e2..a2268cc4d1d 100644 --- a/test/lib/ansible_test/_internal/integration/__init__.py +++ b/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: diff --git a/test/lib/ansible_test/_internal/payload.py b/test/lib/ansible_test/_internal/payload.py index 401b7f29c39..8003ea13ad5 100644 --- a/test/lib/ansible_test/_internal/payload.py +++ b/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) diff --git a/test/lib/ansible_test/_internal/util.py b/test/lib/ansible_test/_internal/util.py index f9873388220..6ad53f5f065 100644 --- a/test/lib/ansible_test/_internal/util.py +++ b/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. diff --git a/test/lib/ansible_test/_internal/util_common.py b/test/lib/ansible_test/_internal/util_common.py index b9f613b7c9c..7f3141fd252 100644 --- a/test/lib/ansible_test/_internal/util_common.py +++ b/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):