-
Notifications
You must be signed in to change notification settings - Fork 372
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 callbacks to run_command #2098
Conversation
|
||
# W0108: Lambda may not be necessary (unnecessary-lambda) - The use of lambda is appropriate | ||
shellutil.set_on_command_started_callback(lambda pid: started_commands.append(pid)) # pylint:disable=W0108 | ||
shellutil.set_on_command_completed_callback(lambda pid: completed_commands.append(pid)) # pylint:disable=W0108 |
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.
My understanding of W0108
is that, instead of lambda pid: started_commands.append(pid)
, you can just do started_commands.append
.
See, e.g., this StackOverflow post.
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.
Yes, either way is correct functionally. For me, it is clearer to say in the code that I am passing a callable that takes a pid and appends it to the array, it is more explicit that just passing append.
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.
I personally agree about the readability. I think it would be beneficial to modify the comment to point out that benefit, but I don't think it's really that big of a deal. Your discretion!
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.
I would advocate for disabling all the style rules in Pylint; I find that most of the time they just get in the way.
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.
+1 to that, if not disable all then atleast disable most (like too many branches, not enough instances, etc etc)
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.
LGTM!
_on_command_completed_callback = None # pylint:disable=C0103 | ||
|
||
|
||
def set_on_command_started_callback(callback): |
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 is not used anywhere? Maybe I'm missing something, is this only the first PR of multiple that involves callback changes?
If I'm seeing this correctly, _invoke_on_command_started_callback
does nothing since the _on_command_started_callback
will always be None, correct?
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.
It will be used to check the processes in the agent cgroup
Added callbacks to run/run_get_output/run_command/run_pipe for commands started and completed.
These will be used to monitor the commands started by the agent.