diff --git a/changelogs/fragments/76353-fix-check-mode-skipping.yaml b/changelogs/fragments/76353-fix-check-mode-skipping.yaml index 6b93fa8f6a9..8dc03978d37 100644 --- a/changelogs/fragments/76353-fix-check-mode-skipping.yaml +++ b/changelogs/fragments/76353-fix-check-mode-skipping.yaml @@ -1,3 +1,4 @@ bugfixes: - shell/command - only skip in check mode if the options `creates` and `removes` are both None. - - script - skip in check mode since the plugin cannot determine if a change will occur. + - shell/command - only return changed as True if the task has not been skipped. + - script - skip in check mode if the plugin cannot determine if a change will occur (i.e. neither `creates` or `removes` are provided). diff --git a/lib/ansible/modules/assert.py b/lib/ansible/modules/assert.py index 100dda2213d..7104d59c323 100644 --- a/lib/ansible/modules/assert.py +++ b/lib/ansible/modules/assert.py @@ -55,7 +55,7 @@ attributes: connection: support: none check_mode: - support: none + support: full delegation: support: none details: Aside from C(register) and/or in combination with C(delegate_facts), it has little effect. diff --git a/lib/ansible/modules/command.py b/lib/ansible/modules/command.py index 47672853740..ebde2a249fc 100644 --- a/lib/ansible/modules/command.py +++ b/lib/ansible/modules/command.py @@ -362,6 +362,8 @@ def main(): if r['msg']: module.exit_json(**r) + r['changed'] = True + # actually executes command (or not ...) if not module.check_mode: r['start'] = datetime.datetime.now() @@ -374,8 +376,8 @@ def main(): r['msg'] = "Command would have run if not in check mode" if creates is None and removes is None: r['skipped'] = True - - r['changed'] = True + # skipped=True and changed=True are mutually exclusive + r['changed'] = False # convert to text for jsonization and usability if r['start'] is not None and r['end'] is not None: diff --git a/lib/ansible/modules/fail.py b/lib/ansible/modules/fail.py index 882ced041ae..ffb701ac950 100644 --- a/lib/ansible/modules/fail.py +++ b/lib/ansible/modules/fail.py @@ -39,7 +39,7 @@ attributes: connection: support: none check_mode: - support: none + support: full diff_mode: support: none delegation: diff --git a/lib/ansible/modules/known_hosts.py b/lib/ansible/modules/known_hosts.py index 95ae95a824c..d1de60efae0 100644 --- a/lib/ansible/modules/known_hosts.py +++ b/lib/ansible/modules/known_hosts.py @@ -55,6 +55,15 @@ options: choices: [ "absent", "present" ] default: "present" type: str +attributes: + check_mode: + support: full + diff_mode: + support: full + platform: + platforms: posix +extends_documentation_fragment: + - action_common_attributes author: - Matthew Vernon (@mcv21) ''' diff --git a/lib/ansible/modules/pause.py b/lib/ansible/modules/pause.py index 440359c7a27..4bac6851432 100644 --- a/lib/ansible/modules/pause.py +++ b/lib/ansible/modules/pause.py @@ -51,7 +51,7 @@ attributes: bypass_host_loop: support: full check_mode: - support: none + support: full connection: support: none delegation: diff --git a/lib/ansible/modules/reboot.py b/lib/ansible/modules/reboot.py index f842454f4a5..79a810de6b9 100644 --- a/lib/ansible/modules/reboot.py +++ b/lib/ansible/modules/reboot.py @@ -90,7 +90,7 @@ attributes: bypass_host_loop: support: none check_mode: - support: none + support: full diff_mode: support: none platform: diff --git a/lib/ansible/modules/script.py b/lib/ansible/modules/script.py index 15782240fa0..596deb49929 100644 --- a/lib/ansible/modules/script.py +++ b/lib/ansible/modules/script.py @@ -60,7 +60,8 @@ extends_documentation_fragment: - decrypt attributes: check_mode: - support: none + support: partial + details: while the script itself is arbitrary and cannot be subject to the check mode semantics it adds C(creates)/C(removes) options as a workaround diff_mode: support: none platform: diff --git a/lib/ansible/modules/validate_argument_spec.py b/lib/ansible/modules/validate_argument_spec.py index e7b5f1ea075..a36d516fb3d 100644 --- a/lib/ansible/modules/validate_argument_spec.py +++ b/lib/ansible/modules/validate_argument_spec.py @@ -40,7 +40,7 @@ attributes: connection: support: none check_mode: - support: none + support: full delegation: support: none diff_mode: diff --git a/lib/ansible/plugins/action/script.py b/lib/ansible/plugins/action/script.py index ff7f2f3a76d..1bbb8001186 100644 --- a/lib/ansible/plugins/action/script.py +++ b/lib/ansible/plugins/action/script.py @@ -87,41 +87,47 @@ class ActionModule(ActionBase): except AnsibleError as e: raise AnsibleActionFail(to_native(e)) + if self._task.check_mode: + # check mode is supported if 'creates' or 'removes' are provided + # the task has already been skipped if a change would not occur + if self._task.args.get('creates') or self._task.args.get('removes'): + result['changed'] = True + raise _AnsibleActionDone(result=result) + # If the script doesn't return changed in the result, it defaults to True, + # but since the script may override 'changed', just skip instead of guessing. + else: + result['changed'] = False + raise AnsibleActionSkip('Check mode is not supported for this task.', result=result) + # now we execute script, always assume changed. result['changed'] = True - if not self._play_context.check_mode: - # transfer the file to a remote tmp location - tmp_src = self._connection._shell.join_path(self._connection._shell.tmpdir, - os.path.basename(source)) + # transfer the file to a remote tmp location + tmp_src = self._connection._shell.join_path(self._connection._shell.tmpdir, + os.path.basename(source)) - # Convert raw_params to text for the purpose of replacing the script since - # parts and tmp_src are both unicode strings and raw_params will be different - # depending on Python version. - # - # Once everything is encoded consistently, replace the script path on the remote - # system with the remainder of the raw_params. This preserves quoting in parameters - # that would have been removed by shlex.split(). - target_command = to_text(raw_params).strip().replace(parts[0], tmp_src) + # Convert raw_params to text for the purpose of replacing the script since + # parts and tmp_src are both unicode strings and raw_params will be different + # depending on Python version. + # + # Once everything is encoded consistently, replace the script path on the remote + # system with the remainder of the raw_params. This preserves quoting in parameters + # that would have been removed by shlex.split(). + target_command = to_text(raw_params).strip().replace(parts[0], tmp_src) - self._transfer_file(source, tmp_src) + self._transfer_file(source, tmp_src) - # set file permissions, more permissive when the copy is done as a different user - self._fixup_perms2((self._connection._shell.tmpdir, tmp_src), execute=True) + # set file permissions, more permissive when the copy is done as a different user + self._fixup_perms2((self._connection._shell.tmpdir, tmp_src), execute=True) - # add preparation steps to one ssh roundtrip executing the script - env_dict = dict() - env_string = self._compute_environment_string(env_dict) + # add preparation steps to one ssh roundtrip executing the script + env_dict = dict() + env_string = self._compute_environment_string(env_dict) - if executable: - script_cmd = ' '.join([env_string, executable, target_command]) - else: - script_cmd = ' '.join([env_string, target_command]) - - if self._play_context.check_mode: - # If the script doesn't return changed in the result, it defaults to True, - # but since the script may override 'changed', just skip instead of guessing. - raise AnsibleActionSkip('Check mode is not supported for this task.') + if executable: + script_cmd = ' '.join([env_string, executable, target_command]) + else: + script_cmd = ' '.join([env_string, target_command]) script_cmd = self._connection._shell.wrap_for_exec(script_cmd) diff --git a/test/integration/targets/command_shell/tasks/main.yml b/test/integration/targets/command_shell/tasks/main.yml index aad63c0dbd1..1d20522b45f 100644 --- a/test/integration/targets/command_shell/tasks/main.yml +++ b/test/integration/targets/command_shell/tasks/main.yml @@ -155,6 +155,24 @@ path: "{{ output_dir_test }}/afile.txt" state: absent +- name: create afile.txt with create_afile.sh via command (check mode) + command: "{{ output_dir_test }}/create_afile.sh {{output_dir_test }}/afile.txt" + args: + creates: "{{ output_dir_test }}/afile.txt" + register: check_mode_result + check_mode: yes + +- assert: + that: + - check_mode_result.changed + - "'skipped' not in check_mode_result" + +- name: verify that afile.txt still does not exist + stat: + path: "{{output_dir_test}}/afile.txt" + register: stat_result + failed_when: stat_result.stat.exists + - name: create afile.txt with create_afile.sh via command command: "{{ output_dir_test }}/create_afile.sh {{output_dir_test }}/afile.txt" args: @@ -165,6 +183,18 @@ path: "{{ output_dir_test }}/afile.txt" state: file +- name: re-run previous command using creates with globbing (check mode) + command: "{{ output_dir_test }}/create_afile.sh {{ output_dir_test }}/afile.txt" + args: + creates: "{{ output_dir_test }}/afile.*" + register: check_mode_result + check_mode: yes + +- assert: + that: + - not check_mode_result.changed + - "'skipped' not in check_mode_result" + - name: re-run previous command using creates with globbing command: "{{ output_dir_test }}/create_afile.sh {{ output_dir_test }}/afile.txt" args: @@ -178,6 +208,24 @@ # removes +- name: remove afile.txt with remote_afile.sh via command (check mode) + command: "{{ output_dir_test }}/remove_afile.sh {{ output_dir_test }}/afile.txt" + args: + removes: "{{ output_dir_test }}/afile.txt" + register: check_mode_result + check_mode: yes + +- assert: + that: + - check_mode_result.changed + - "'skipped' not in check_mode_result" + +- name: verify that afile.txt still exists + stat: + path: "{{output_dir_test}}/afile.txt" + register: stat_result + failed_when: not stat_result.stat.exists + - name: remove afile.txt with remote_afile.sh via command command: "{{ output_dir_test }}/remove_afile.sh {{ output_dir_test }}/afile.txt" args: @@ -186,6 +234,18 @@ - name: verify that afile.txt is absent file: path={{output_dir_test}}/afile.txt state=absent +- name: re-run previous command using removes with globbing (check mode) + command: "{{ output_dir_test }}/remove_afile.sh {{ output_dir_test }}/afile.txt" + args: + removes: "{{ output_dir_test }}/afile.*" + register: check_mode_result + check_mode: yes + +- assert: + that: + - not check_mode_result.changed + - "'skipped' not in check_mode_result" + - name: re-run previous command using removes with globbing command: "{{ output_dir_test }}/remove_afile.sh {{ output_dir_test }}/afile.txt" args: @@ -450,6 +510,8 @@ assert: that: - "'Command would have run if not in check mode' in result.msg" + - result.skipped + - not result.changed - name: test check mode creates/removes message command: @@ -462,6 +524,8 @@ assert: that: - "'Command would have run if not in check mode' in result.msg" + - "'skipped' not in result" + - result.changed - name: command symlink handling block: diff --git a/test/integration/targets/script/tasks/main.yml b/test/integration/targets/script/tasks/main.yml index f1746f7c485..2c85f7c5e74 100644 --- a/test/integration/targets/script/tasks/main.yml +++ b/test/integration/targets/script/tasks/main.yml @@ -176,7 +176,8 @@ - name: Assert that a change was reported but the script did not make changes assert: that: - - _check_mode_test is changed + - _check_mode_test is not changed + - _check_mode_test is skipped - not _afile_stat.stat.exists - name: Run script to create a file