Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add a function for the centralised list of shell/command options #81558

Open
wants to merge 3 commits into
base: devel
Choose a base branch
from
+43 −16
Open

add a function for the centralised list of shell/command options #81558

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6a642c7
add a function for the centralised list of shell/command options
nama1arpit Aug 22, 2023
64b5119
add changelog fragment for fixing interactive shell options
nama1arpit Aug 22, 2023
a9605e3
remove unnecessary .keys()
nama1arpit Aug 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions changelogs/fragments/81558-fix-interactive-shell-options.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
bugfixes:
- shell module - parameter-oriented invocation using 'cmd' was silently failing. Fix now adds 'cmd' argument in shell module and creates centralized list of options for usage in `splitter.py` (https://github.com/ansible/ansible/issues/73005).
25 changes: 25 additions & 0 deletions lib/ansible/module_utils/shell.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
# Simplified BSD License (see licenses/simplified_bsd.txt or https://opensource.org/licenses/BSD-2-Clause)

from __future__ import (absolute_import, division, print_function)
__metaclass__ = type


def get_command_args():
'''
The function for a centralised list for retrieval of shell module options
'''
return dict(
_raw_params=dict(),
_uses_shell=dict(type='bool', default=False),
argv=dict(type='list', elements='str'),
chdir=dict(type='path'),
executable=dict(),
expand_argument_vars=dict(type='bool', default=True),
creates=dict(type='path'),
removes=dict(type='path'),
# The default for this really comes from the action plugin
stdin=dict(required=False),
stdin_add_newline=dict(type='bool', default=True),
strip_empty_ends=dict(type='bool', default=True),
cmd=dict(),
)
16 changes: 2 additions & 14 deletions lib/ansible/modules/command.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,7 @@
from ansible.module_utils.basic import AnsibleModule
from ansible.module_utils.common.text.converters import to_native, to_bytes, to_text
from ansible.module_utils.common.collections import is_iterable
from ansible.module_utils.shell import get_command_args


def main():
Expand All @@ -248,20 +249,7 @@ def main():
# 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(),
_uses_shell=dict(type='bool', default=False),
argv=dict(type='list', elements='str'),
chdir=dict(type='path'),
executable=dict(),
expand_argument_vars=dict(type='bool', default=True),
creates=dict(type='path'),
removes=dict(type='path'),
# The default for this really comes from the action plugin
stdin=dict(required=False),
stdin_add_newline=dict(type='bool', default=True),
strip_empty_ends=dict(type='bool', default=True),
),
argument_spec=get_command_args(),
supports_check_mode=True,
)
shell = module.params['_uses_shell']
Expand Down
4 changes: 2 additions & 2 deletions lib/ansible/parsing/splitter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@

from ansible.errors import AnsibleParserError
from ansible.module_utils.common.text.converters import to_text
from ansible.module_utils.shell import get_command_args
from ansible.parsing.quoting import unquote


Expand Down Expand Up @@ -79,8 +80,7 @@ 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', 'stdin', 'stdin_add_newline', 'strip_empty_ends'):
if check_raw and k not in get_command_args():
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although the current list of key-value arguments is out-of-date, I don't think that's a good reason to move them into module_utils. The main reason is that the list of valid arguments varies by module. There are currently 6 different modules that make use of check_raw:

ansible/lib/ansible/constants.py

Lines 116 to 117 in 116948c

MODULE_REQUIRE_ARGS = tuple(add_internal_fqcns(('command', 'win_command', 'ansible.windows.win_command', 'shell', 'win_shell',
'ansible.windows.win_shell', 'raw', 'script')))

While there are arguments in common between those modules, they're not all the same. For example,
the win_command module does not accept strip_empty_ends.

It seems like we'd be better off checking the actual list of arguments accepted by the module we're parsing for, and use those when calling parse_kv, instead of using check_raw to activate a hard-coded list.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the approach in this PR was suggested by @sivel in #73005 (comment) -- although that seems to have been based on a ~9 year old code comment that predates the split to collections:

ansible/lib/ansible/parsing/splitter.py

Lines 90 to 91 in 29aea9f

# FIXME: make the retrieval of this list of shell/command
# options a function, so the list is centralized

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the concern and I would need some more guidance to resolve this as I am not very familiar with the codebase. I can see that parse_kv is used in a few places. Should I try to get the actual list of arguments accepted by the module here?

ansible/lib/ansible/cli/console.py

Lines 204 to 206 in 29aea9f

try:
check_raw = module in C._ACTION_ALLOWS_RAW_ARGS
task = dict(action=dict(module=module, args=parse_kv(module_args, check_raw=check_raw)), timeout=self.task_timeout)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently there isn't a good way to get the supported action/module args from the action name while parsing args.
Rather than implementing that (a non-trivial task), one option would be to add cmd to the existing hard-coded list used by the splitter when check_raw is enabled.

While that would allow resolving #73005, it would also have the unwanted affect of changing how the raw action is handled.

For example, a playbook with a task such as the following would be affected:

    - raw: some_command -o cmd=something

Currently this results in running: some_command -o cmd=something

With cmd in the list of args used by check_raw, it would instead become: some_command -o

This should be discussed further before we commit to a solution.

raw_params.append(orig_x)
else:
options[k.strip()] = unquote(v.strip())
Expand Down
12 changes: 12 additions & 0 deletions test/integration/targets/command_shell/tasks/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -480,6 +480,18 @@
- shell_result9 is failed or
shell_result9 is changed

- name: execute a shell command with parameter-oriented invocation using "cmd"
shell: 'cmd="echo test"'
register: shell_result10

- name: Assert the shell with parameter-oriented invocation ran as expected
assert:
that:
- shell_result10 is changed
- shell_result10.rc == 0
- shell_result10.cmd == "echo test"
- shell_result10.stdout == "test"

- name: remove the previously created file
file:
path: "{{ remote_tmp_dir_test }}/afile.txt"
Expand Down
Loading