From ef554d03785dff43a03a1121e921eb55ff5c0bb4 Mon Sep 17 00:00:00 2001 From: Rick Elrod Date: Fri, 9 Apr 2021 09:28:24 -0500 Subject: [PATCH] [playbook] error on empty, error on 'include' (remove two deprecations) (#74172) Change: - Remove two deprecated features - We now error if a playbook is an empty list instead of just skipping - We now error if using 'include' instead of 'import_playbook' Test Plan: - Added new tests for new errors Tickets: - Fixes #74133 Signed-off-by: Rick Elrod * sanity & changelog Signed-off-by: Rick Elrod --- changelogs/fragments/playbook-deprecations.yml | 3 +++ lib/ansible/constants.py | 1 - lib/ansible/playbook/__init__.py | 13 +++++-------- lib/ansible/playbook/playbook_include.py | 2 +- .../includes/include_on_playbook_should_fail.yml | 1 + test/integration/targets/includes/runme.sh | 6 ++++++ test/integration/targets/includes/test_includes.yml | 8 ++++---- test/integration/targets/path_lookups/play.yml | 10 +++++----- test/integration/targets/playbook/empty.yml | 1 + test/integration/targets/playbook/runme.sh | 6 ++++++ test/sanity/ignore.txt | 1 - 11 files changed, 32 insertions(+), 20 deletions(-) create mode 100644 changelogs/fragments/playbook-deprecations.yml create mode 100644 test/integration/targets/includes/include_on_playbook_should_fail.yml create mode 100644 test/integration/targets/playbook/empty.yml diff --git a/changelogs/fragments/playbook-deprecations.yml b/changelogs/fragments/playbook-deprecations.yml new file mode 100644 index 00000000000..155f2f36e52 --- /dev/null +++ b/changelogs/fragments/playbook-deprecations.yml @@ -0,0 +1,3 @@ +minor_changes: + - playbook - Error if a playbook is an empty list instead of just skipping + - playbook - Error if using ``include`` instead of ``import_playbook`` diff --git a/lib/ansible/constants.py b/lib/ansible/constants.py index d4cee0c84ed..8ef5c41d76e 100644 --- a/lib/ansible/constants.py +++ b/lib/ansible/constants.py @@ -77,7 +77,6 @@ _ACTION_SETUP = add_internal_fqcns(('setup', )) _ACTION_HAS_CMD = add_internal_fqcns(('command', 'shell', 'script')) _ACTION_ALLOWS_RAW_ARGS = _ACTION_HAS_CMD + add_internal_fqcns(('raw', )) _ACTION_ALL_INCLUDES = _ACTION_INCLUDE + _ACTION_INCLUDE_TASKS + _ACTION_INCLUDE_ROLE -_ACTION_ALL_IMPORT_PLAYBOOKS = _ACTION_INCLUDE + _ACTION_IMPORT_PLAYBOOK _ACTION_ALL_INCLUDE_IMPORT_TASKS = _ACTION_INCLUDE + _ACTION_INCLUDE_TASKS + _ACTION_IMPORT_TASKS _ACTION_ALL_PROPER_INCLUDE_IMPORT_ROLES = _ACTION_INCLUDE_ROLE + _ACTION_IMPORT_ROLE _ACTION_ALL_PROPER_INCLUDE_IMPORT_TASKS = _ACTION_INCLUDE_TASKS + _ACTION_IMPORT_TASKS diff --git a/lib/ansible/playbook/__init__.py b/lib/ansible/playbook/__init__.py index 8c4ed65f0b3..0dbb38e43bd 100644 --- a/lib/ansible/playbook/__init__.py +++ b/lib/ansible/playbook/__init__.py @@ -79,8 +79,8 @@ class Playbook: self._loader.set_basedir(cur_basedir) raise AnsibleParserError("A playbook must be a list of plays, got a %s instead" % type(ds), obj=ds) elif not ds: - display.deprecated("Empty plays will currently be skipped, in the future they will cause a syntax error", - version='2.12', collection_name='ansible.builtin') + self._loader.set_basedir(cur_basedir) + raise AnsibleParserError("A playbook must contain at least one play") # Parse the playbook entries. For plays, we simply parse them # using the Play() object, and includes are parsed using the @@ -89,18 +89,15 @@ class Playbook: if not isinstance(entry, dict): # restore the basedir in case this error is caught and handled self._loader.set_basedir(cur_basedir) - raise AnsibleParserError("playbook entries must be either a valid play or an include statement", obj=entry) + raise AnsibleParserError("playbook entries must be either valid plays or 'import_playbook' statements", obj=entry) - if any(action in entry for action in C._ACTION_ALL_IMPORT_PLAYBOOKS): - if any(action in entry for action in C._ACTION_INCLUDE): - display.deprecated("'include' for playbook includes. You should use 'import_playbook' instead", - version="2.12", collection_name='ansible.builtin') + if any(action in entry for action in C._ACTION_IMPORT_PLAYBOOK): pb = PlaybookInclude.load(entry, basedir=self._basedir, variable_manager=variable_manager, loader=self._loader) if pb is not None: self._entries.extend(pb._entries) else: which = entry - for k in C._ACTION_IMPORT_PLAYBOOK + C._ACTION_INCLUDE: + for k in C._ACTION_IMPORT_PLAYBOOK: if k in entry: which = entry[k] break diff --git a/lib/ansible/playbook/playbook_include.py b/lib/ansible/playbook/playbook_include.py index 5b966902550..d65f7966f36 100644 --- a/lib/ansible/playbook/playbook_include.py +++ b/lib/ansible/playbook/playbook_include.py @@ -140,7 +140,7 @@ class PlaybookInclude(Base, Conditional, Taggable): new_ds.ansible_pos = ds.ansible_pos for (k, v) in iteritems(ds): - if k in C._ACTION_ALL_IMPORT_PLAYBOOKS: + if k in C._ACTION_IMPORT_PLAYBOOK: self._preprocess_import(ds, new_ds, k, v) else: # some basic error checking, to make sure vars are properly diff --git a/test/integration/targets/includes/include_on_playbook_should_fail.yml b/test/integration/targets/includes/include_on_playbook_should_fail.yml new file mode 100644 index 00000000000..953459dcf8c --- /dev/null +++ b/test/integration/targets/includes/include_on_playbook_should_fail.yml @@ -0,0 +1 @@ +- include: test_includes3.yml diff --git a/test/integration/targets/includes/runme.sh b/test/integration/targets/includes/runme.sh index 70ff105bd1d..f4f0a0162f5 100755 --- a/test/integration/targets/includes/runme.sh +++ b/test/integration/targets/includes/runme.sh @@ -5,3 +5,9 @@ set -eux ansible-playbook test_includes.yml -i ../../inventory "$@" ansible-playbook inherit_notify.yml "$@" + +echo "EXPECTED ERROR: Ensure we fail if using 'include' to include a playbook." +set +e +result="$(ansible-playbook -i ../../inventory include_on_playbook_should_fail.yml -v "$@" 2>&1)" +set -e +grep -q "ERROR! 'include' is not a valid attribute for a Play" <<< "$result" diff --git a/test/integration/targets/includes/test_includes.yml b/test/integration/targets/includes/test_includes.yml index 0bcebd4f9d3..adeb80d2baa 100644 --- a/test/integration/targets/includes/test_includes.yml +++ b/test/integration/targets/includes/test_includes.yml @@ -1,7 +1,7 @@ -- include: test_includes2.yml parameter1=asdf parameter2=jkl +- import_playbook: test_includes2.yml parameter1=asdf parameter2=jkl -- include: test_includes3.yml +- import_playbook: test_includes3.yml -- include: test_include_free.yml +- import_playbook: test_include_free.yml -- include: test_include_host_pinned.yml +- import_playbook: test_include_host_pinned.yml diff --git a/test/integration/targets/path_lookups/play.yml b/test/integration/targets/path_lookups/play.yml index 7321589bf9a..233f972da37 100644 --- a/test/integration/targets/path_lookups/play.yml +++ b/test/integration/targets/path_lookups/play.yml @@ -10,31 +10,31 @@ - copy: dest={{playbook_dir}}/files/testfile content='in files' - copy: dest={{playbook_dir}}/testfile content='in local' -- include: testplay.yml +- import_playbook: testplay.yml vars: remove: nothing role_out: in role files play_out: in files -- include: testplay.yml +- import_playbook: testplay.yml vars: remove: roles/showfile/files/testfile role_out: in role play_out: in files -- include: testplay.yml +- import_playbook: testplay.yml vars: remove: roles/showfile/testfile role_out: in role tasks play_out: in files -- include: testplay.yml +- import_playbook: testplay.yml vars: remove: roles/showfile/tasks/testfile role_out: in files play_out: in files -- include: testplay.yml +- import_playbook: testplay.yml vars: remove: files/testfile role_out: in local diff --git a/test/integration/targets/playbook/empty.yml b/test/integration/targets/playbook/empty.yml new file mode 100644 index 00000000000..fe51488c706 --- /dev/null +++ b/test/integration/targets/playbook/empty.yml @@ -0,0 +1 @@ +[] diff --git a/test/integration/targets/playbook/runme.sh b/test/integration/targets/playbook/runme.sh index 25e2e5a64d3..ec646dd312d 100755 --- a/test/integration/targets/playbook/runme.sh +++ b/test/integration/targets/playbook/runme.sh @@ -7,3 +7,9 @@ ansible-playbook -i ../../inventory types.yml -v "$@" # test timeout ansible-playbook -i ../../inventory timeout.yml -v "$@" + +echo "EXPECTED ERROR: Ensure we fail properly if a playbook is an empty list." +set +e +result="$(ansible-playbook -i ../../inventory empty.yml -v "$@" 2>&1)" +set -e +grep -q "ERROR! A playbook must contain at least one play" <<< "$result" diff --git a/test/sanity/ignore.txt b/test/sanity/ignore.txt index d43bd159fbd..85e5991575a 100644 --- a/test/sanity/ignore.txt +++ b/test/sanity/ignore.txt @@ -142,7 +142,6 @@ lib/ansible/modules/yum_repository.py validate-modules:doc-default-does-not-matc lib/ansible/modules/yum_repository.py validate-modules:parameter-type-not-in-doc lib/ansible/modules/yum_repository.py validate-modules:undocumented-parameter lib/ansible/parsing/vault/__init__.py pylint:blacklisted-name -lib/ansible/playbook/__init__.py pylint:ansible-deprecated-version lib/ansible/playbook/base.py pylint:blacklisted-name lib/ansible/playbook/collectionsearch.py required-and-default-attributes # https://github.com/ansible/ansible/issues/61460 lib/ansible/playbook/conditional.py pylint:ansible-deprecated-version