-
Notifications
You must be signed in to change notification settings - Fork 95
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
support local async batching #247
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me apart form the comments that I left.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, looks good to me now. This feature needs a minimum required Salt version though that is not even released yet, right? Is this patch backwards compatible in a way that the new batch parameters are ignored when working with an older Salt, or would you rather see this SaltInvocationError
? I guess both should be acceptable.
@renner so far it looks from @chiaradiamarcelo tests that it is backwards compatible since salt just ignores the additional parameters. |
yes, I can confirm that |
@lucidd shouldn't this PR include a class to map the 'batch started' salt event ? |
The following is an example of a 'batch-start' event json: salt/batch/20190130132632115965/start {
"_stamp": "2019-01-30T12:26:32.178962",
"available_minions": [
"mch-minion_1.tf.local"
],
"down_minions": [
"mch-minion_2.tf.local"
],
"metadata": {
"suma-action-id": 3111,
"suma-batch-mode": true
}
} |
386cd24
to
e75031c
Compare
2607409
to
8804bd6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This patch looks still good to me. New unit tests might be a good idea though, or also a code example to understand how you would make use of the batching would be nice along with the other examples.
8804bd6
to
b600b89
Compare
b600b89
to
6fe39e9
Compare
adding the batch parameter to async calls as its gonna be supported in salt with saltstack/salt#50546