Add proper check mode support to the script module (#31852)

* Do not run script in check mode

Fixes #30676

* Reformat script integration test

* Add integration tests for check mode of script module

* Fix name on test

* Cleanup temp file

* win_script integration test syntaxt changes

* Add check mode tests for win_script

* Use proper variable in test

* Fail if source file does not exist

* Verify script is accessible and don't copy in check mode

Use shlex to properly split shell arguments, though a path with spaces in it still needs to be quoted in the playbook.
Add note to docs describing such.
Improve error message if file is not found indicating there may be a space in the path.

* Properly encode path now that path is split using shlex

* Allow for spaces in both path and script name

* Add unicode character test to Linux script tests

* Add Linux test for space in path to script

5 files changed, 248 insertions, 50 deletions

diff --git a/test/integration/targets/script/files/space path/test.sh b/test/integration/targets/script/files/space path/test.sh
new file mode 100755
index 0000000000..6f6334d71f
--- /dev/null
+++ b/test/integration/targets/script/files/space path/test.sh
@@ -0,0 +1,3 @@
+#!/usr/bin/env bash
+
+echo -n "Script with space in path"
\ No newline at end of file
diff --git a/test/integration/targets/script/files/test_with_args.sh b/test/integration/targets/script/files/test_with_args.sh
new file mode 100755
index 0000000000..13dce4f24c
--- /dev/null
+++ b/test/integration/targets/script/files/test_with_args.sh
@@ -0,0 +1,5 @@
+#!/usr/bin/env bash
+
+for i in "$@"; do
+    echo "$i"
+done
\ No newline at end of file
diff --git a/test/integration/targets/script/tasks/main.yml b/test/integration/targets/script/tasks/main.yml
index 7b31444190..85c9a8dcbc 100644
--- a/test/integration/targets/script/tasks/main.yml
+++ b/test/integration/targets/script/tasks/main.yml
@@ -20,13 +20,18 @@
 ## prep
 ##
 
-- set_fact: output_dir_test={{output_dir}}/test_script
+- set_fact:
+    output_dir_test: "{{ output_dir }}/test_script"
 
 - name: make sure our testing sub-directory does not exist
-  file: path="{{ output_dir_test }}" state=absent
+  file:
+    path: "{{ output_dir_test }}"
+    state: absent
 
 - name: create our testing sub-directory
-  file: path="{{ output_dir_test }}" state=directory
+  file:
+    path: "{{ output_dir_test }}"
+    state: directory
 
 ##
 ## script
@@ -43,36 +48,101 @@
       - "script_result0.stderr == ''"
       - "script_result0.stdout == 'win'"
 
-# creates
+- name: Execute a script with a space in the path
+  script: "'space path/test.sh'"
+  register: _space_path_test
+  tags:
+    - spacepath
+
+- name: Assert that script with space in path ran successfully
+  assert:
+    that:
+      - _space_path_test is success
+      - _space_path_test.stdout == 'Script with space in path'
+  tags:
+    - spacepath
+
+- name: Execute a script with arguments including a unicode character
+  script: test_with_args.sh -this -that -Ӧther
+  register: unicode_args
 
+- name: Assert that script with unicode character ran successfully
+  assert:
+    that:
+      - unicode_args is success
+      - unicode_args.stdout_lines[0] == '-this'
+      - unicode_args.stdout_lines[1] == '-that'
+      - unicode_args.stdout_lines[2] == '-Ӧther'
+
+# creates
 - name: verify that afile.txt is absent
-  file: path={{output_dir_test}}/afile.txt state=absent
+  file:
+    path: "{{ output_dir_test }}/afile.txt"
+    state: absent
 
 - name: create afile.txt with create_afile.sh via command
-  script: create_afile.sh {{output_dir_test | expanduser}}/afile.txt creates={{output_dir_test | expanduser}}/afile.txt
+  script: create_afile.sh {{ output_dir_test | expanduser }}/afile.txt
+  args:
+    creates: "{{ output_dir_test | expanduser }}/afile.txt"
+  register: _create_test1
+
+- name: Check state of created file
+  stat:
+    path: "{{ output_dir_test | expanduser }}/afile.txt"
+  register: _create_stat1
+
+- name: Run create_afile.sh again to ensure it is skipped
+  script: create_afile.sh {{ output_dir_test | expanduser }}/afile.txt
+  args:
+    creates: "{{ output_dir_test | expanduser }}/afile.txt"
+  register: _create_test2
+
+- name: Assert that script report a change, file was created, second run was skipped
+  assert:
+    that:
+      - _create_test1 | changed
+      - _create_stat1.stat.exists
+      - _create_test2 | skipped
 
-- name: verify that afile.txt is present
-  file: path={{output_dir_test}}/afile.txt state=file
 
 # removes
+- name: verify that afile.txt is present
+  file:
+    path: "{{ output_dir_test }}/afile.txt"
+    state: file
 
 - name: remove afile.txt with remote_afile.sh via command
-  script: remove_afile.sh {{output_dir_test | expanduser}}/afile.txt removes={{output_dir_test | expanduser}}/afile.txt
-  register: script_result1
-
-- name: verify that afile.txt is absent
-  file: path={{output_dir_test}}/afile.txt state=absent
-  register: script_result2
-
-- name: assert that the file was removed by the script
+  script: remove_afile.sh {{ output_dir_test | expanduser }}/afile.txt
+  args:
+    removes: "{{ output_dir_test | expanduser }}/afile.txt"
+  register: _remove_test1
+
+- name: Check state of removed file
+  stat:
+    path: "{{ output_dir_test | expanduser }}/afile.txt"
+  register: _remove_stat1
+
+- name: Run remote_afile.sh again to enure it is skipped
+  script: remove_afile.sh {{ output_dir_test | expanduser }}/afile.txt
+  args:
+    removes: "{{ output_dir_test | expanduser }}/afile.txt"
+  register: _remove_test2
+
+- name: Assert that script report a change, file was removed, second run was skipped
   assert:
     that:
-      - "script_result1|changed"
-      - "script_result2.state == 'absent'"
+      - _remove_test1 | changed
+      - not _remove_stat1.stat.exists
+      - _remove_test2 | skipped
+
 
 # async
-- name: test task failure with async param
+- name: verify that afile.txt is absent
+  file:
+    path: "{{ output_dir_test }}/afile.txt"
+    state: absent
 
+- name: test task failure with async param
   script: /some/script.sh
   async: 2
   ignore_errors: true
@@ -81,5 +151,73 @@
 - name: assert task with async param failed
   assert:
     that:
-      - script_result3|failed
+      - script_result3 | failed
       - script_result3.msg == "async is not supported for this task."
+
+
+# check mode
+- name: Run script to create a file in check mode
+  script: create_afile.sh {{ output_dir_test | expanduser }}/afile2.txt
+  check_mode: yes
+  register: _check_mode_test
+
+- debug:
+    var: _check_mode_test
+    verbosity: 2
+
+- name: Get state of file created by script
+  stat:
+    path: "{{ output_dir_test | expanduser }}/afile2.txt"
+  register: _afile_stat
+
+- debug:
+    var: _afile_stat
+    verbosity: 2
+
+- name: Assert that a change was reported but the script did not make changes
+  assert:
+    that:
+      - _check_mode_test | changed
+      - not _afile_stat.stat.exists
+
+- name: Run script to create a file
+  script: create_afile.sh {{ output_dir_test | expanduser }}/afile2.txt
+
+- name: Run script to create a file in check mode with 'creates' argument
+  script: create_afile.sh {{ output_dir_test | expanduser }}/afile2.txt
+  args:
+    creates: "{{ output_dir_test | expanduser }}/afile2.txt"
+  register: _check_mode_test2
+  check_mode: yes
+
+- debug:
+    var: _check_mode_test2
+    verbosity: 2
+
+- name: Assert that task was skipped and mesage was returned
+  assert:
+    that:
+      - _check_mode_test2 | skipped
+      - '_check_mode_test2.msg == "skipped, since {{ output_dir_test | expanduser }}/afile2.txt exists"'
+
+- name: Remove afile2.txt
+  file:
+    path: "{{ output_dir_test | expanduser }}/afile2.txt"
+    state: absent
+
+- name: Run script to remove a file in check mode with 'removes' argument
+  script: remove_afile.sh {{ output_dir_test | expanduser }}/afile2.txt
+  args:
+    removes: "{{ output_dir_test | expanduser }}/afile2.txt"
+  register: _check_mode_test3
+  check_mode: yes
+
+- debug:
+    var: _check_mode_test3
+    verbosity: 2
+
+- name: Assert that task was skipped and message was returned
+  assert:
+    that:
+      - _check_mode_test3 | skipped
+      - '_check_mode_test3.msg == "skipped, since {{ output_dir_test | expanduser }}/afile2.txt does not exist"'
diff --git a/test/integration/targets/win_script/files/space path/test_script.ps1 b/test/integration/targets/win_script/files/space path/test_script.ps1
new file mode 100644
index 0000000000..24e0ab2ed5
--- /dev/null
+++ b/test/integration/targets/win_script/files/space path/test_script.ps1
@@ -0,0 +1 @@
+Write-Host "Ansible supports spaces in the path to the script."
diff --git a/test/integration/targets/win_script/tasks/main.yml b/test/integration/targets/win_script/tasks/main.yml
index 1a9c25924f..32b637630a 100644
--- a/test/integration/targets/win_script/tasks/main.yml
+++ b/test/integration/targets/win_script/tasks/main.yml
@@ -35,8 +35,8 @@
       - "test_script_result.stdout"
       - "'Woohoo' in test_script_result.stdout"
       - "not test_script_result.stderr"
-      - "not test_script_result|failed"
-      - "test_script_result|changed"
+      - "not test_script_result | failed"
+      - "test_script_result | changed"
 
 - name: run test script that takes arguments including a unicode char
   script: test_script_with_args.ps1 /this /that /Ӧther
@@ -51,8 +51,8 @@
       - "test_script_with_args_result.stdout_lines[1] == '/that'"
       - "test_script_with_args_result.stdout_lines[2] == '/Ӧther'"
       - "not test_script_with_args_result.stderr"
-      - "not test_script_with_args_result|failed"
-      - "test_script_with_args_result|changed"
+      - "not test_script_with_args_result | failed"
+      - "test_script_with_args_result | changed"
 
 - name: run test script that takes parameters passed via splatting
   script: test_script_with_splatting.ps1 @{ This = 'this'; That = '{{ test_win_script_value }}'; Other = 'other'}
@@ -67,8 +67,8 @@
       - "test_script_with_splatting_result.stdout_lines[1] == test_win_script_value"
       - "test_script_with_splatting_result.stdout_lines[2] == 'other'"
       - "not test_script_with_splatting_result.stderr"
-      - "not test_script_with_splatting_result|failed"
-      - "test_script_with_splatting_result|changed"
+      - "not test_script_with_splatting_result | failed"
+      - "test_script_with_splatting_result | changed"
 
 - name: run test script that takes splatted parameters from a variable
   script: test_script_with_splatting.ps1 {{ test_win_script_splat }}
@@ -83,8 +83,8 @@
       - "test_script_with_splatting2_result.stdout_lines[1] == 'THAT'"
       - "test_script_with_splatting2_result.stdout_lines[2] == 'OTHER'"
       - "not test_script_with_splatting2_result.stderr"
-      - "not test_script_with_splatting2_result|failed"
-      - "test_script_with_splatting2_result|changed"
+      - "not test_script_with_splatting2_result | failed"
+      - "test_script_with_splatting2_result | changed"
 
 - name: run test script that has errors
   script: test_script_with_errors.ps1
@@ -97,15 +97,17 @@
       - "test_script_with_errors_result.rc != 0"
       - "not test_script_with_errors_result.stdout"
       - "test_script_with_errors_result.stderr"
-      - "test_script_with_errors_result|failed"
-      - "test_script_with_errors_result|changed"
+      - "test_script_with_errors_result | failed"
+      - "test_script_with_errors_result | changed"
 
 - name: cleanup test file if it exists
-  raw: Remove-Item "{{test_win_script_filename}}" -Force
+  raw: Remove-Item "{{ test_win_script_filename }}" -Force
   ignore_errors: true
 
 - name: run test script that creates a file
-  script: test_script_creates_file.ps1 "{{test_win_script_filename}}" creates="{{test_win_script_filename}}"
+  script: test_script_creates_file.ps1 {{ test_win_script_filename }}
+  args:
+    creates: "{{ test_win_script_filename }}"
   register: test_script_creates_file_result
 
 - name: check that script ran and indicated a change
@@ -114,22 +116,26 @@
       - "test_script_creates_file_result.rc == 0"
       - "not test_script_creates_file_result.stdout"
       - "not test_script_creates_file_result.stderr"
-      - "not test_script_creates_file_result|failed"
-      - "test_script_creates_file_result|changed"
+      - "not test_script_creates_file_result | failed"
+      - "test_script_creates_file_result | changed"
 
 - name: run test script that creates a file again
-  script: test_script_creates_file.ps1 "{{test_win_script_filename}}" creates="{{test_win_script_filename}}"
+  script: test_script_creates_file.ps1 {{ test_win_script_filename }}
+  args:
+    creates: "{{ test_win_script_filename }}"
   register: test_script_creates_file_again_result
 
 - name: check that the script did not run since the remote file exists
   assert:
     that:
-      - "not test_script_creates_file_again_result|failed"
-      - "not test_script_creates_file_again_result|changed"
-      - "test_script_creates_file_again_result|skipped"
+      - "not test_script_creates_file_again_result | failed"
+      - "not test_script_creates_file_again_result | changed"
+      - "test_script_creates_file_again_result | skipped"
 
 - name: run test script that removes a file
-  script: test_script_removes_file.ps1 "{{test_win_script_filename}}" removes="{{test_win_script_filename}}"
+  script: test_script_removes_file.ps1 {{ test_win_script_filename }}
+  args:
+    removes: "{{ test_win_script_filename }}"
   register: test_script_removes_file_result
 
 - name: check that the script ran since the remote file exists
@@ -138,19 +144,21 @@
       - "test_script_removes_file_result.rc == 0"
       - "not test_script_removes_file_result.stdout"
       - "not test_script_removes_file_result.stderr"
-      - "not test_script_removes_file_result|failed"
-      - "test_script_removes_file_result|changed"
+      - "not test_script_removes_file_result | failed"
+      - "test_script_removes_file_result | changed"
 
 - name: run test script that removes a file again
-  script: test_script_removes_file.ps1 "{{test_win_script_filename}}" removes="{{test_win_script_filename}}"
+  script: test_script_removes_file.ps1 {{ test_win_script_filename }}
+  args:
+    removes: "{{ test_win_script_filename }}"
   register: test_script_removes_file_again_result
 
 - name: check that the script did not run since the remote file does not exist
   assert:
     that:
-      - "not test_script_removes_file_again_result|failed"
-      - "not test_script_removes_file_again_result|changed"
-      - "test_script_removes_file_again_result|skipped"
+      - "not test_script_removes_file_again_result | failed"
+      - "not test_script_removes_file_again_result | changed"
+      - "test_script_removes_file_again_result | skipped"
 
 # TODO: these tests fail on 2008 (not even R2) with no output. It's related to the default codepage being UTF8- if we force it back to 437, it works fine.
 # Need to figure out a sane place to do that under the new exec wrapper.
@@ -165,8 +173,8 @@
 #      - "test_batch_result.stdout"
 #      - "'batch' in test_batch_result.stdout"
 #      - "not test_batch_result.stderr"
-#      - "not test_batch_result|failed"
-#      - "test_batch_result|changed"
+#      - "not test_batch_result | failed"
+#      - "test_batch_result | changed"
 
 
 #- name: run simple batch file with .cmd extension
@@ -180,8 +188,8 @@
 #      - "test_cmd_result.stdout"
 #      - "'cmd extension' in test_cmd_result.stdout"
 #      - "not test_cmd_result.stderr"
-#      - "not test_cmd_result|failed"
-#      - "test_cmd_result|changed"
+#      - "not test_cmd_result | failed"
+#      - "test_cmd_result | changed"
 
 - name: run test script that takes a boolean parameter
   script: test_script_bool.ps1 $true
@@ -205,4 +213,47 @@
 #    that:
 #    - test_script_env_result | succeeded
 #    - test_script_env_result.stdout_lines[0] == 'task'
-#
\ No newline at end of file
+#
+
+
+# check mode
+- name: Run test script that creates a file in check mode
+  script: test_script_creates_file.ps1 {{ test_win_script_filename }}
+  args:
+    creates: "{{ test_win_script_filename }}"
+  check_mode: yes
+  register: test_script_creates_file_check_mode
+
+- name: Get state of file created by script
+  win_stat:
+    path: "{{ test_win_script_filename }}"
+  register: create_file_stat
+
+- name: Assert that a change was reported but the script did not make changes
+  assert:
+    that:
+      - test_script_creates_file_check_mode | changed
+      - not create_file_stat.stat.exists
+
+- name: Run test script that creates a file
+  script: test_script_creates_file.ps1 {{ test_win_script_filename }}
+  args:
+    creates: "{{ test_win_script_filename }}"
+
+- name: Run test script that removes a file in check mode
+  script: test_script_removes_file.ps1 {{ test_win_script_filename }}
+  args:
+    removes: "{{ test_win_script_filename }}"
+  check_mode: yes
+  register: test_script_removes_file_check_mode
+
+- name: Get state of file removed by script
+  win_stat:
+    path: "{{ test_win_script_filename }}"
+  register: remove_file_stat
+
+- name: Assert that a change was reported but the script did not make changes
+  assert:
+    that:
+      - test_script_removes_file_check_mode | changed
+      - remove_file_stat.stat.exists