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

RunnerClient name=value parsing does not work as intended #57

Closed
whiteinge opened this issue Oct 28, 2015 · 5 comments
Closed

RunnerClient name=value parsing does not work as intended #57

whiteinge opened this issue Oct 28, 2015 · 5 comments

Comments

@whiteinge
Copy link
Contributor

Bug identified in #55 by @jfroche.

The problem can be clearly seen via the following command. In addition to the name=value parsing, the arguments in RunnerClient are not being run through the YAML parser like in LocalClient.

pepper --client=runner test.arg foo bar=Bar baz=true qux='{qux: Qux, quux: false}'
@whiteinge
Copy link
Contributor Author

This appears to be a deficiency in RunnerClient.cmd_sync() and RunnerClient.cmd_async() which are the methods that salt-api calls. RunnerClient.cmd() works correctly which is what the salt-run CLI command uses.

>>> import salt.config
>>> import salt.runner
>>> opts = salt.config.master_config('/etc/salt/master')
>>> runner = salt.runner.RunnerClient(opts)
>>> runner.cmd('test.arg', ['foo', 'bar=Bar', 'baz=True'])
{'args': ('foo',), 'kwargs': {'bar': 'Bar', 'baz': True}}
>>> runner.cmd_sync({'fun': 'test.arg', 'args': ['foo', 'bar=Bar', 'baz=True'], 'username': 'saltd
ev', 'password': 'saltdev', 'eauth': 'auto'})
{'args': ['foo', 'bar=Bar', 'baz=True'], 'kwargs': {}}

It looks like the two methods above are not processing the input through salt.utils.args.parse_input() which both LocalClient and RunnerClient.cmd() use.

@jfroche does this bug sound like something you can tackle from here? My available Salt time is limited at the moment. If not we can tag the core Salt team on the issue and they can add it to their sprint backlog.

@whiteinge
Copy link
Contributor Author

Er. That shell session may not be 100% accurate. Pepper may be sending slightly differently low data when using --client=runner. Must verify what Pepper is sending first, but parse_input() is still the likely fix.

@ashald
Copy link
Contributor

ashald commented Dec 17, 2015

+1

@ashald
Copy link
Contributor

ashald commented Mar 21, 2016

So the PR with the fix is merged into Salt's develop branch. Let's wait for its release. ;)

@whiteinge
Copy link
Contributor Author

@ashald thank you very much for fixing that!

mattp- added a commit to bloomberg/pepper that referenced this issue Oct 3, 2018
rather than attempting to deconstruct args locally to fully reproduce
the proper low state to pass to the api, we instead leave the kwargs as
is. The reason for this is the RunnerClient will deconstruct and
deserialize nested yaml args. We could equivalently do that locally but
it seems pepper tries to be a no-dependency dist so we just hand off the
work instead.

Fixes saltstack#57
mattp- added a commit to bloomberg/pepper that referenced this issue Oct 19, 2018
rather than attempting to deconstruct args locally to fully reproduce
the proper low state to pass to the api, we instead leave the kwargs as
is. The reason for this is the RunnerClient will deconstruct and
deserialize nested yaml args. We could equivalently do that locally but
it seems pepper tries to be a no-dependency dist so we just hand off the
work instead.

This PR partially depends on release of saltstack/salt#50124
to determine if a release is neon or newer; however, we can safely assume that if
the salt_version header is not provided in a request it is a salt-api
older than that.

Fixes saltstack#57
mattp- added a commit to bloomberg/pepper that referenced this issue Oct 19, 2018
rather than attempting to deconstruct args locally to fully reproduce
the proper low state to pass to the api, we instead leave the kwargs as
is. The reason for this is the RunnerClient will deconstruct and
deserialize nested yaml args. We could equivalently do that locally but
it seems pepper tries to be a no-dependency dist so we just hand off the
work instead.

This PR partially depends on release of saltstack/salt#50124
to determine if a release is neon or newer; however, we can safely assume that if
the salt_version header is not provided in a request it is a salt-api
older than that.

Fixes saltstack#57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants