various fixes to command (#74212) (#74258)

* various fixes to command

  - Updated splitter to allow for all expected args in ad-hoc
  - Ensure we always return the returns we promissed to always return (i.e stderr/stdout)
  - Updated docs to clarify creates/removes precdence in checking
  - Removed abspath from chdir to allow reporting to handle symlinks correctly
  - Corrected tests to new output messages

Co-authored-by: Felix Fontein <felix@fontein.de>
(cherry picked from commit b3b1dde64887b1646633f0c30eecd03bbbe9218c)

4 files changed, 164 insertions, 56 deletions

diff --git a/changelogs/fragments/command_deliver_promisses.yml b/changelogs/fragments/command_deliver_promisses.yml
new file mode 100644
index 0000000000..c2f61ba90e
--- /dev/null
+++ b/changelogs/fragments/command_deliver_promisses.yml
@@ -0,0 +1,6 @@
+bugfixes:
+  - command module, now always returns what we documented as 'returns always'.
+  - command module, now all options work in ad-hoc execution.
+  - command module, clarify order of remove/creates checks.
+  - command module, correctly handles chdir to symlinks.
+  - command module, move to standarized messages in 'msg' vs abusing 'stdout'.
diff --git a/lib/ansible/modules/command.py b/lib/ansible/modules/command.py
index 1680543295..115b1cde51 100644
--- a/lib/ansible/modules/command.py
+++ b/lib/ansible/modules/command.py
@@ -46,10 +46,12 @@ options:
     type: path
     description:
       - A filename or (since 2.0) glob pattern. If a matching file already exists, this step B(will not) be run.
+      - This is checked before I(removes) is checked.
   removes:
     type: path
     description:
       - A filename or (since 2.0) glob pattern. If a matching file exists, this step B(will) be run.
+      - This is checked after I(creates) is checked.
     version_added: "0.8"
   chdir:
     type: path
@@ -250,6 +252,7 @@ def main():
 
     # the command module is the one ansible module that does not take key=value args
     # hence don't copy this one if you are looking to build others!
+    # NOTE: ensure splitter.py is kept in sync for exceptions
     module = AnsibleModule(
         argument_spec=dict(
             _raw_params=dict(),
@@ -279,95 +282,103 @@ def main():
     stdin_add_newline = module.params['stdin_add_newline']
     strip = module.params['strip_empty_ends']
 
+    # we promissed these in 'always' ( _lines get autoaded on action plugin)
+    r = {'changed': False, 'stdout': '', 'stderr': '', 'rc': None, 'cmd': None, 'start': None, 'end': None, 'delta': None, 'msg': ''}
+
     if not shell and executable:
         module.warn("As of Ansible 2.4, the parameter 'executable' is no longer supported with the 'command' module. Not using '%s'." % executable)
         executable = None
 
     if (not args or args.strip() == '') and not argv:
-        module.fail_json(rc=256, msg="no command given")
+        r['rc'] = 256
+        r['msg'] = "no command given"
+        module.fail_json(**r)
 
     if args and argv:
-        module.fail_json(rc=256, msg="only command or argv can be given, not both")
+        r['rc'] = 256
+        r['msg'] = "only command or argv can be given, not both"
+        module.fail_json(**r)
 
     if not shell and args:
         args = shlex.split(args)
 
     args = args or argv
-
     # All args must be strings
     if is_iterable(args, include_strings=False):
         args = [to_native(arg, errors='surrogate_or_strict', nonstring='simplerepr') for arg in args]
 
+    r['cmd'] = args
+    if warn:
+        # nany telling you to use module instead!
+        check_command(module, args)
+
     if chdir:
         try:
-            chdir = to_bytes(os.path.abspath(chdir), errors='surrogate_or_strict')
+            chdir = to_bytes(chdir, errors='surrogate_or_strict')
         except ValueError as e:
-            module.fail_json(msg='Unable to use supplied chdir: %s' % to_text(e))
+            r['msg'] = 'Unable to use supplied chdir from %s: %s ' % (os.getcwd(), to_text(e))
+            module.fail_json(**r)
 
         try:
             os.chdir(chdir)
         except (IOError, OSError) as e:
-            module.fail_json(msg='Unable to change directory before execution: %s' % to_text(e))
+            r['msg'] = 'Unable to change directory before execution: %s' % to_text(e)
+            module.fail_json(**r)
+
+    # check_mode partial support, since it only really works in checking creates/removes
+    if module.check_mode:
+        shoulda = "Would"
+    else:
+        shoulda = "Did"
 
+    # special skips for idempotence if file exists (assumes command creates)
     if creates:
-        # do not run the command if the line contains creates=filename
-        # and the filename already exists.  This allows idempotence
-        # of command executions.
         if glob.glob(creates):
-            module.exit_json(
-                cmd=args,
-                stdout="skipped, since %s exists" % creates,
-                changed=False,
-                rc=0
-            )
-
-    if removes:
-        # do not run the command if the line contains removes=filename
-        # and the filename does not exist.  This allows idempotence
-        # of command executions.
-        if not glob.glob(removes):
-            module.exit_json(
-                cmd=args,
-                stdout="skipped, since %s does not exist" % removes,
-                changed=False,
-                rc=0
-            )
+            r['msg'] = "%s not run command since '%s' exists" % (shoulda, creates)
+            r['stdout'] = "skipped, since %s exists" % creates  # TODO: deprecate
 
-    if warn:
-        check_command(module, args)
+            r['rc'] = 0
 
-    startd = datetime.datetime.now()
+    # special skips for idempotence if file does not exist (assumes command removes)
+    if not r['msg'] and removes:
+        if not glob.glob(removes):
+            r['msg'] = "%s not run command since '%s' does not exist" % (shoulda, removes)
+            r['stdout'] = "skipped, since %s does not exist" % removes  # TODO: deprecate
+            r['rc'] = 0
 
+    if r['msg']:
+        module.exit_json(**r)
+
+    # actually executes command (or not ...)
     if not module.check_mode:
-        rc, out, err = module.run_command(args, executable=executable, use_unsafe_shell=shell, encoding=None, data=stdin, binary_data=(not stdin_add_newline))
-    elif creates or removes:
-        rc = 0
-        out = err = b'Command would have run if not in check mode'
+        r['start'] = datetime.datetime.now()
+        r['rc'], r['stdout'], r['stderr'] = module.run_command(args, executable=executable, use_unsafe_shell=shell, encoding=None,
+                                                               data=stdin, binary_data=(not stdin_add_newline))
+        r['end'] = datetime.datetime.now()
     else:
-        module.exit_json(msg="skipped, running in check mode", skipped=True)
+        # this is partial check_mode support, since we end up skipping if we get here
+        r['rc'] = 0
+        r['msg'] = "Command would have run if not in check mode"
+        r['skipped'] = True
+
+    r['changed'] = True
 
-    endd = datetime.datetime.now()
-    delta = endd - startd
+    # convert to text for jsonization and usability
+    if r['start'] is not None and r['end'] is not None:
+        # these are datetime objects, but need them as strings to pass back
+        r['delta'] = to_text(r['end'] - r['start'])
+        r['end'] = to_text(r['end'])
+        r['start'] = to_text(r['start'])
 
     if strip:
-        out = out.rstrip(b"\r\n")
-        err = err.rstrip(b"\r\n")
-
-    result = dict(
-        cmd=args,
-        stdout=out,
-        stderr=err,
-        rc=rc,
-        start=str(startd),
-        end=str(endd),
-        delta=str(delta),
-        changed=True,
-    )
+        r['stdout'] = to_text(r['stdout']).rstrip("\r\n")
+        r['stderr'] = to_text(r['stderr']).rstrip("\r\n")
 
-    if rc != 0:
-        module.fail_json(msg='non-zero return code', **result)
+    if r['rc'] != 0:
+        r['msg'] = 'non-zero return code'
+        module.fail_json(**r)
 
-    module.exit_json(**result)
+    module.exit_json(**r)
 
 
 if __name__ == '__main__':
diff --git a/lib/ansible/parsing/splitter.py b/lib/ansible/parsing/splitter.py
index b5209b01ff..ab7df04db7 100644
--- a/lib/ansible/parsing/splitter.py
+++ b/lib/ansible/parsing/splitter.py
@@ -87,9 +87,8 @@ def parse_kv(args, check_raw=False):
                 k = x[:pos]
                 v = x[pos + 1:]
 
-                # FIXME: make the retrieval of this list of shell/command
-                #        options a function, so the list is centralized
-                if check_raw and k not in ('creates', 'removes', 'chdir', 'executable', 'warn'):
+                # FIXME: make the retrieval of this list of shell/command options a function, so the list is centralized
+                if check_raw and k not in ('creates', 'removes', 'chdir', 'executable', 'warn', 'stdin', 'stdin_add_newline', 'strip_empty_ends'):
                     raw_params.append(orig_x)
                 else:
                     options[k.strip()] = unquote(v.strip())
diff --git a/test/integration/targets/command_shell/tasks/main.yml b/test/integration/targets/command_shell/tasks/main.yml
index 1d614e4945..fd23dcd188 100644
--- a/test/integration/targets/command_shell/tasks/main.yml
+++ b/test/integration/targets/command_shell/tasks/main.yml
@@ -444,3 +444,95 @@
   file:
     path: "{{ output_dir_test }}/afile.txt"
     state: absent
+
+- name: test warning with command
+  command:
+    cmd: "rm -h"
+    warn: yes
+  ignore_errors: yes
+  register: warn_result
+
+- name: assert warning exists
+  assert:
+    that:
+      - warn_result.warnings | length == 1
+      - "'Consider using the file module with state=absent rather than running \\'rm\\'' in warn_result.warnings[0]"
+
+- name: test warning with shell
+  shell: "sed -h"
+  args:
+    warn: yes
+  ignore_errors: yes
+  register: warn_result
+
+- name: assert warning exists
+  assert:
+    that:
+      - warn_result.warnings | length == 1
+      - "'Consider using the replace, lineinfile or template module rather than running \\'sed\\'' in warn_result.warnings[0]"
+
+- name: test become warning
+  command:
+    cmd: "sudo true"
+    warn: yes
+  ignore_errors: yes
+  register: warn_result
+
+- name: assert warning exists
+  assert:
+    that:
+      - warn_result.warnings | length == 1
+      - "'Consider using \\'become\\', \\'become_method\\', and \\'become_user\\' rather than running sudo' in warn_result.warnings[0]"
+
+- name: test check mode skip message
+  command:
+    cmd: "true"
+  check_mode: yes
+  register: result
+
+- name: assert check message exists
+  assert:
+    that:
+      - "'Command would have run if not in check mode' in result.msg"
+
+- name: test check mode creates/removes message
+  command:
+    cmd: "true"
+    creates: yes
+  check_mode: yes
+  register: result
+
+- name: assert check message exists
+  assert:
+    that:
+      - "'Command would have run if not in check mode' in result.msg"
+
+- name: command symlink handling
+  block:
+  - name: Create target folders
+    file:
+      path: '{{output_dir}}/www_root/site'
+      state: directory
+
+  - name: Create symlink
+    file:
+      path: '{{output_dir}}/www'
+      state: link
+      src: '{{output_dir}}/www_root'
+
+  - name: check parent using chdir
+    shell: dirname "$PWD"
+    args:
+      chdir: '{{output_dir}}/www/site'
+    register: parent_dir_chdir
+
+  - name: check parent using cd
+    shell: cd "{{output_dir}}/www/site" && dirname "$PWD"
+    register: parent_dir_cd
+
+  - name: check expected outputs
+    assert:
+      that:
+        - parent_dir_chdir.stdout != parent_dir_cd.stdout
+        - 'parent_dir_cd.stdout == "{{output_dir}}/www"'
+        - 'parent_dir_chdir.stdout == "{{output_dir}}/www_root"'