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.

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

Conversation

nama1arpit
Copy link

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.

@ansibot 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
@nama1arpit
Copy link
Author

Added the changelog fragment

@s-hertel s-hertel removed the needs_triage Needs a first human triage before being processed. label Aug 22, 2023
@ansibot 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
@nama1arpit

This comment was marked as resolved.

@ansibot 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
@@ -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:

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:

# 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?

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.

@ansibot 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 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 nitzmahone assigned nitzmahone and bcoca and unassigned nitzmahone May 1, 2024
@nitzmahone
Copy link
Member

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

@bcoca
Copy link
Member

bcoca commented May 1, 2024

depends on #79720

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue/PR relates to a bug. has_issue module This issue/PR relates to a module. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

builtin.shell: "cmd" not available for interactive use
6 participants