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

parsing: recognize 'stdin' as a module argument in splitter.py #55921

Open
wants to merge 1 commit into
base: devel
from

Conversation

Projects
None yet
5 participants
@larsks
Copy link
Contributor

commented Apr 30, 2019

SUMMARY

The parse_kv hardcodes a list of module parameter that it will
recognize. This has diverged from the parameters actually supported by
the command, shell, and win_command modules. This commits
introduces parse_kv to the stdin parameter.

Fixes: #55918

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

parsing

ADDITIONAL INFORMATION

See #55918 for details.

@nitzmahone

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

Yep, this was definitely an oversight when those new command/shell args were added... Any chance you'd be willing to take a pass over the other ones that have been added and make sure they're all in there since you're "in the neighborhood"?

@larsks

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

@nitzmahone Sure. I thought about submitted the PR with all of them but I figured there were good odds that someone would ask me to remove everything but stdin :).

@larsks larsks force-pushed the larsks:bug/55918 branch from a1de3fb to 25a0d1d Apr 30, 2019

@larsks

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

I've added support for stdin_add_newline and strip_empty_ends, although I note that:

  • win_command supports neither
  • shell supports stdin_add_newline
  • command supports both
  • ...and I haven't looked at any of the others.

Which suggests that there is room for making the various command modules consistent.

As far as I can tell, neither of these options has any impact on the output of the command module when used as an ad-hoc task.

@ansibot ansibot removed the small_patch label Apr 30, 2019

@@ -87,7 +98,7 @@ def parse_kv(args, check_raw=False):

# 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'):
if check_raw and k not in _COMMAND_ARGS:

This comment has been minimized.

Copy link
@bcoca

bcoca Apr 30, 2019

Member

what we really need at this point is a 'dictionary lookup' of the action since not all of them have the same options

This comment has been minimized.

Copy link
@larsks

larsks May 1, 2019

Author Contributor

@bcoca How about if we just go back to adding only stdin in this PR, which is common across all the plugins, and leave a more extensive refactoring for a future PR? Because ultimately trying to keep this list of keywords in sync with the modules seems like a losing battle, and I wonder if there is a better way...but I'd still like to see stdin support sooner rather than later.

This comment has been minimized.

Copy link
@bcoca

bcoca May 1, 2019

Member

better way is 'have no hardcoded exceptions', one way to do that is make it 'property based' on the modules, but that would require reading them on controller side.

parsing: recognize 'stdin' as a module argument in splitter.py
The `parse_kv` hardcodes a list of module parameter that it will
recognize. This has diverged from the parameters actually supported by
the `command`, `shell`, and `win_command` modules.  This commits
introduces `parse_kv` to the `stdin` parameter.

Fixes: #55918

@larsks larsks force-pushed the larsks:bug/55918 branch from 25a0d1d to 3fb13b6 May 13, 2019

@ansibot ansibot removed the stale_ci label May 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.