From 6e011d0809fd24940a57e2e51d61ef4fd7090623 Mon Sep 17 00:00:00 2001 From: Brian Coca Date: Fri, 3 Sep 2021 18:53:55 -0400 Subject: [PATCH] various fixes to command (#74212) (#74257) * 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) --- .../fragments/command_deliver_promisses.yml | 6 + lib/ansible/modules/command.py | 118 ++++++++++-------- lib/ansible/parsing/splitter.py | 5 +- .../targets/command_shell/tasks/main.yml | 4 +- 4 files changed, 76 insertions(+), 57 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 00000000000..c2f61ba90e5 --- /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 96d460eca3f..1ccd3388b26 100644 --- a/lib/ansible/modules/command.py +++ b/lib/ansible/modules/command.py @@ -47,10 +47,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 @@ -254,6 +256,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(), @@ -283,92 +286,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: - chdir = to_bytes(chdir, errors='surrogate_or_strict') + try: + chdir = to_bytes(chdir, errors='surrogate_or_strict') + except ValueError as 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 b5209b01ff6..ab7df04db7b 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 3e7edcf1cd8..653b00594be 100644 --- a/test/integration/targets/command_shell/tasks/main.yml +++ b/test/integration/targets/command_shell/tasks/main.yml @@ -449,7 +449,7 @@ - name: assert check message exists assert: that: - - "'skipped, running in check mode' in result.msg" + - "'Command would have run if not in check mode' in result.msg" - name: test check mode creates/removes message command: @@ -461,7 +461,7 @@ - name: assert check message exists assert: that: - - "'Command would have run if not in check mode' in result.stderr" + - "'Command would have run if not in check mode' in result.msg" - name: command symlink handling block: