From e10a4d511dc8d9fc909b5709a52dd467447f5119 Mon Sep 17 00:00:00 2001 From: Brian Coca Date: Fri, 3 Sep 2021 18:53:29 -0400 Subject: 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 (cherry picked from commit b3b1dde64887b1646633f0c30eecd03bbbe9218c) --- changelogs/fragments/command_deliver_promisses.yml | 6 ++ lib/ansible/modules/command.py | 117 +++++++++++---------- lib/ansible/parsing/splitter.py | 5 +- .../targets/command_shell/tasks/main.yml | 92 ++++++++++++++++ 4 files changed, 164 insertions(+), 56 deletions(-) create mode 100644 changelogs/fragments/command_deliver_promisses.yml 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"' -- cgit v1.2.1