Add proper check mode support to the script module

lib/ansible/modules/commands/script.py

@@ -26,7 +26,7 @@
 options:
   free_form:
     description:
-      - path to the local script file followed by optional arguments. There is no parameter actually named 'free form'; see the examples!
+      - Path to the local script file followed by optional arguments. There is no parameter actually named 'free form'; see the examples!
     required: true
     default: null
     aliases: []
@@ -53,6 +53,7 @@
   - The ssh connection plugin will force pseudo-tty allocation via -tt when scripts are executed. pseudo-ttys do not have a stderr channel and all
     stderr is sent to stdout. If you depend on separated stdout and stderr result keys, please switch to a copy+command set of tasks instead of using script.
   - This module is also supported for Windows targets.
+  - If the path to the local script contains spaces, it needs to be quoted.
 author:
     - Ansible Core Team
     - Michael DeHaan

lib/ansible/plugins/action/script.py

@@ -19,9 +19,10 @@
 
 import os
 import re
+import shlex
 
 from ansible.errors import AnsibleError
-from ansible.module_utils._text import to_native
+from ansible.module_utils._text import to_native, to_text
 from ansible.plugins.action import ActionBase
 
 
@@ -72,30 +73,46 @@ def run(self, tmp=None, task_vars=None):
             if self._connection._shell.SHELL_FAMILY != 'powershell' and not chdir.startswith('/'):
                 return dict(failed=True, msg='chdir %s must be an absolute path for a Unix-aware remote node' % chdir)
 
-        # the script name is the first item in the raw params, so we split it
-        # out now so we know the file name we need to transfer to the remote,
-        # and everything else is an argument to the script which we need later
-        # to append to the remote command
-        parts = self._task.args.get('_raw_params', '').strip().split()
+        # Split out the script as the first item in raw_params using
+        # shlex.split() in order to support paths and files with spaces in the name.
+        # Any arguments passed to the script will be added back later.
+        raw_params = to_native(self._task.args.get('_raw_params', ''), errors='surrogate_or_strict')
+        parts = [to_text(s, errors='surrogate_or_strict') for s in shlex.split(raw_params.strip())]
         source = parts[0]
-        args = ' '.join(parts[1:])
 
         try:
             source = self._loader.get_real_file(self._find_needle('files', source), decrypt=self._task.args.get('decrypt', True))
         except AnsibleError as e:
             return dict(failed=True, msg=to_native(e))
 
-        # transfer the file to a remote tmp location
-        tmp_src = self._connection._shell.join_path(tmp, os.path.basename(source))
-        self._transfer_file(source, tmp_src)
+        if not self._play_context.check_mode:
+            # transfer the file to a remote tmp location
+            tmp_src = self._connection._shell.join_path(tmp, os.path.basename(source))
 
-        # set file permissions, more permissive when the copy is done as a different user
-        self._fixup_perms2((tmp, tmp_src), execute=True)
+            # 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)
+
+            # set file permissions, more permissive when the copy is done as a different user
+            self._fixup_perms2((tmp, 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)
+            script_cmd = ' '.join([env_string, target_command])
+
+        if self._play_context.check_mode:
+            result['changed'] = True
+            self._remove_tmp_path(tmp)
+            return result
 
-        # add preparation steps to one ssh roundtrip executing the script
-        env_dict = dict()
-        env_string = self._compute_environment_string(env_dict)
-        script_cmd = ' '.join([env_string, tmp_src, args])
         script_cmd = self._connection._shell.wrap_for_exec(script_cmd)
 
         exec_data = None

test/integration/targets/script/files/space path/test.sh

@@ -0,0 +1,3 @@
+#!/usr/bin/env bash
+
+echo -n "Script with space in path"

test/integration/targets/script/files/test_with_args.sh

@@ -0,0 +1,5 @@
+#!/usr/bin/env bash
+
+for i in "$@"; do
+    echo "$i"
+done

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

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