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

nama1arpit · 2023-08-22T01:23:20Z

SUMMARY

Fixes #73005

This PR

adds cmd argument to the shell/command options
moves the list of shell module options to a centralized function for usage in splitter.py
adds an integration test

ISSUE TYPE

Bugfix Pull Request

ADDITIONAL INFORMATION

Integration testing:

$ bin/ansible-test integration -v command_shell
...
TASK [command_shell : execute a shell command with parameter-oriented invocation using "cmd"] ***
changed: [testhost] => {"changed": true, "cmd": "echo test", "delta": "0:00:00.002359", "end": "2023-08-22 10:49:54.248467", "msg": "", "rc": 0, "start": "2023-08-22 10:49:54.246108", "stderr": "", "stderr_lines": [], "stdout": "test", "stdout_lines": ["test"]}

TASK [command_shell : Assert the shell with parameter-oriented invocation ran as expected] ***
ok: [testhost] => {
    "changed": false,
    "msg": "All assertions passed"
}
...
PLAY RECAP *********************************************************************
testhost                   : ok=91   changed=39   unreachable=0    failed=0    skipped=3    rescued=0    ignored=4

Sanity testing:

$ bin/ansible-test sanity
...
Running sanity test "yamllint"
WARNING: Reviewing previous 25 warning(s):
WARNING: The validate-modules sanity test cannot compare against the base commit because it was not detected.
WARNING: Skipping tests disabled by default without --allow-disabled: package-data
WARNING: Skipping sanity test "compile" on Python 2.7 because it could not be found.
WARNING: Skipping sanity test "compile" on Python 3.6 because it could not be found.
WARNING: Skipping sanity test "compile" on Python 3.7 because it could not be found.
WARNING: Skipping sanity test "compile" on Python 3.8 because it could not be found.
WARNING: Skipping sanity test "compile" on Python 3.9 because it could not be found.
WARNING: Skipping sanity test "compile" on Python 3.11 because it could not be found.
WARNING: Skipping sanity test "compile" on Python 3.12 because it could not be found.
WARNING: Skipping sanity test "import" on Python 2.7 because it could not be found.
WARNING: Skipping sanity test "import" on Python 3.6 because it could not be found.
WARNING: Skipping sanity test "import" on Python 3.7 because it could not be found.
WARNING: Skipping sanity test "import" on Python 3.8 because it could not be found.
WARNING: Skipping sanity test "import" on Python 3.9 because it could not be found.
WARNING: Skipping sanity test "import" on Python 3.11 because it could not be found.
WARNING: Skipping sanity test "import" on Python 3.12 because it could not be found.
WARNING: Skipping sanity test "mypy" on Python 2.7 because it is unsupported. Supported Python versions: 3.6, 3.7, 3.8, 3.9, 3.10, 3.11, 3.12
WARNING: Skipping sanity test "mypy" on Python 3.6 because it could not be found.
WARNING: Skipping sanity test "mypy" on Python 3.7 because it could not be found.
WARNING: Skipping sanity test "mypy" on Python 3.8 because it could not be found.
WARNING: Skipping sanity test "mypy" on Python 3.9 because it could not be found.
WARNING: Skipping sanity test "mypy" on Python 3.11 because it could not be found.
WARNING: Skipping sanity test "mypy" on Python 3.12 because it could not be found.
WARNING: Required program "pwsh" not found.
WARNING: Required program "shellcheck" not found.

nama1arpit · 2023-08-22T02:38:13Z

Added the changelog fragment

lib/ansible/parsing/splitter.py

mattclay · 2023-08-23T18:40:14Z

lib/ansible/parsing/splitter.py

@@ -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():


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.

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

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)

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.

nitzmahone · 2024-05-01T19:38:41Z

Some planned features may make this more generically possible in the not-too-distant future.

bcoca · 2024-05-01T20:14:55Z

depends on #79720

webknjaz · 2025-01-21T15:56:53Z

/azp run

azure-pipelines · 2025-01-21T15:57:05Z

Azure Pipelines successfully started running 1 pipeline(s).

webknjaz · 2025-01-21T16:04:03Z

@mattclay @sivel this is an example PR that #84375 / #84402 would've helped with.

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

6a642c7

ansibot added bug This issue/PR relates to a bug. needs_triage Needs a first human triage before being processed. module This issue/PR relates to a module. has_issue labels Aug 22, 2023

add changelog fragment for fixing interactive shell options

64b5119

s-hertel removed the needs_triage Needs a first human triage before being processed. label Aug 22, 2023

s-hertel reviewed Aug 22, 2023

View reviewed changes

lib/ansible/parsing/splitter.py Outdated Show resolved Hide resolved

remove unnecessary .keys()

a9605e3

ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Aug 22, 2023

This comment was marked as resolved.

Sign in to view

ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Aug 23, 2023

mattclay requested changes Aug 23, 2023

View reviewed changes

ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Aug 23, 2023

ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Sep 5, 2023

nitzmahone assigned nitzmahone and bcoca and unassigned nitzmahone May 1, 2024

ansibot added the needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. label Dec 16, 2024

ansibot removed the needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. label Jan 21, 2025

ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jan 21, 2025

webknjaz mentioned this pull request Jan 21, 2025

🧪 Let ansible-test diff ancient branches (granular approach) #84375

Open

webknjaz added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html ci_verified Changes made in this PR are causing tests to fail. labels Jan 21, 2025

ansibot removed the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Jan 21, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

nama1arpit commented Aug 22, 2023

nama1arpit commented Aug 22, 2023

This comment was marked as resolved.

mattclay Aug 23, 2023

mattclay Aug 23, 2023

nama1arpit Aug 25, 2023

mattclay Aug 29, 2023

nitzmahone commented May 1, 2024

bcoca commented May 1, 2024

webknjaz commented Jan 21, 2025

azure-pipelines bot commented Jan 21, 2025

webknjaz commented Jan 21, 2025

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

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

	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)

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

Are you sure you want to change the base?

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

Conversation

nama1arpit commented Aug 22, 2023

SUMMARY

ISSUE TYPE

ADDITIONAL INFORMATION

nama1arpit commented Aug 22, 2023

This comment was marked as resolved.

mattclay Aug 23, 2023

Choose a reason for hiding this comment

mattclay Aug 23, 2023

Choose a reason for hiding this comment

nama1arpit Aug 25, 2023

Choose a reason for hiding this comment

mattclay Aug 29, 2023

Choose a reason for hiding this comment

nitzmahone commented May 1, 2024

bcoca commented May 1, 2024

webknjaz commented Jan 21, 2025

azure-pipelines bot commented Jan 21, 2025

webknjaz commented Jan 21, 2025