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

Regression in RunnerClient argument handling #25107

Closed
whiteinge opened this issue Jul 1, 2015 · 5 comments
Closed

Regression in RunnerClient argument handling #25107

whiteinge opened this issue Jul 1, 2015 · 5 comments
Assignees
Labels
Bug broken, incorrect, or confusing behavior Confirmed Salt engineer has confirmed bug/feature - often including a MCVE Core relates to code central or existential to Salt Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases P2 Priority 2 Regression The issue is a bug that breaks functionality known to work in previous releases. severity-critical top severity, seen by most users, serious issues severity-high 2nd top severity, seen by most users, causes major problems ZRELEASED - Beryllium
Milestone

Comments

@whiteinge
Copy link
Contributor

There is a regression in how RunnerClient handles args/kwargs in develop (tested on dd80bec). This will affect people calling runners via the Python API and salt-api.

@DmitryKuzmenko I apologize for not reading pull req #24831 more closely. The fix needs to be in RunnerClient and not in the rest_cherrypy module. We must maintain a backward compatible RunnerClient interface. Commit 954ba6e needs to be reverted.

@whiteinge whiteinge added Bug broken, incorrect, or confusing behavior Core relates to code central or existential to Salt severity-high 2nd top severity, seen by most users, causes major problems Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases severity-critical top severity, seen by most users, serious issues P2 Priority 2 labels Jul 1, 2015
@whiteinge whiteinge added this to the Be 3 milestone Jul 1, 2015
@whiteinge whiteinge added the Regression The issue is a bug that breaks functionality known to work in previous releases. label Jul 1, 2015
@DmitryKuzmenko DmitryKuzmenko added the Confirmed Salt engineer has confirmed bug/feature - often including a MCVE label Jul 6, 2015
@DmitryKuzmenko
Copy link
Contributor

As I can see there are 2 ways to provide fun args to RunnerClient:

  1. Explicitly via low['args'] and low['kwargs'], for example:
{
    'client': 'runner',
    'fun': 'jobs.list_job',
    'args': (jid,),
}

or

{
    'client': 'runner',
    'fun': 'jobs.list_job',
    'kwargs': {'jid': jid},
}
  1. Implicitly by not specifying 'args' and 'kwargs' in low and put kwargs directly into low:
{
    'client': 'runner',
    'fun': 'jobs.list_job',
    'jid': jid,
}

This worked as the 2nd one until #20161 added by @cachedout where default empty 'args' and 'kwargs' were forced. There are no references to related issues so I can't understood do we really need it?

@whiteinge
Copy link
Contributor Author

whiteinge commented Jul 6, 2015 via email

@DmitryKuzmenko
Copy link
Contributor

@whiteinge as I can see to fully support the old way we have to determine what arguments could be passed to the requested function and bring the only needed arguments from the top-level:
{'jid': 123} -> {'jid': 123, 'kwargs': {'jid': 123}} if function supports jid argument.
{'jid': 123} -> {'jid': 123, 'kwargs': {}} if not.
In this case we'll need to lookup the requested function on the runner client side (salt-api in this case)
If it's acceptable I can use salt.minion.load_args_and_kwargs()

@whiteinge
Copy link
Contributor Author

whiteinge commented Jul 6, 2015 via email

@DmitryKuzmenko
Copy link
Contributor

Ok, will do in this way. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug broken, incorrect, or confusing behavior Confirmed Salt engineer has confirmed bug/feature - often including a MCVE Core relates to code central or existential to Salt Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases P2 Priority 2 Regression The issue is a bug that breaks functionality known to work in previous releases. severity-critical top severity, seen by most users, serious issues severity-high 2nd top severity, seen by most users, causes major problems ZRELEASED - Beryllium
Projects
None yet
Development

No branches or pull requests

3 participants