Add check_mode tests for shell/command/script and make skipped=True/changed=True mutually exclusive (#76429)

* Add additional tests for check_mode with shell/command/script
* update check_mode documentation: if a module/action is not skipped in check mode and accurately reflects whether a change is made to the remote support should be 'full'
* Make reporting skipped in check mode mutually exclusive with 'changed: True'
* Add missing documented attributes
* Fix tests to expect skipped=True and changed=True do not occur together
* Fix script check_mode support documentation
* Fix earlier changelog
* document platforms attribute
* Use tasks's check mode since the value from PlayContext does not reflect loop items

Co-authored-by: Brian Coca <bcoca@users.noreply.github.com>

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).

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.

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:

lib/ansible/modules/fail.py

@@ -39,7 +39,7 @@ attributes:
     connection:
         support: none
     check_mode:
-        support: none
+        support: full
     diff_mode:
         support: none
     delegation:

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)
 '''

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:

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:

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:

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:

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)
 

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:

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