-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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
[AIRFLOW-5936] Allow explicit get_pty in SSHOperator #6586
Conversation
Codecov Report
@@ Coverage Diff @@
## master #6586 +/- ##
==========================================
- Coverage 83.83% 83.49% -0.34%
==========================================
Files 647 647
Lines 37409 37407 -2
==========================================
- Hits 31360 31234 -126
- Misses 6049 6173 +124
Continue to review full report at Codecov.
|
Just want to make it clearer - by timeout I mean the execution_timeout of the task, not the channel timeout parameter in ssh_client.exec_command(). In many use cases, when the execution_timeout is reached, we want not only to close the ssh connection but also to kill the remote process. |
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.
Thanks for the first-time contribution @zzk-dev !
I think it would also be great if all the get_pty logic - even if it is simple - is also tested in TestSSHOperator. if you move the "if" to the constructor you will be able to have a set of tests where you will test the simple logic in the Test itself - you could pass a command with/without sudo and with different get_pty values and test if self.get_pty is set properly.
if self.command.startswith('sudo'): | ||
get_pty = True | ||
self.get_pty = True |
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 think it might make sense to move this to constructor. command is set only in the constructor so we can keep all the pty logic there and it could be one-liner
self.get_pty = self.start_command.starstwith('sudo') or self.get_pty
7d58f7e
to
b8ff440
Compare
Thanks a lot @potiuk for the review! I have made the suggested changes and added a set of tests. |
Good suggestion! But I got a question - how do I parameterize the case where |
b8ff440
to
b02a91b
Compare
I decided to remove the two cases where I also added two constants - |
b02a91b
to
661f782
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.
Great!. Waiting for Travis. It's ready to merge! Thank you!
Thanks @potiuk ! It's a great learning experience! |
Thanks @zzk-dev -> welcome as contributor! The code is merged. Happy that you've learned something and looking forward for future contributions :) |
Make sure you have checked all steps below.
Jira
Description
Currently when execution_timeout is reached for a SSHOperator task, the ssh connection will be closed but the remote process continues to run. In many scenarios, users might want the process to be killed upon task timeout. Giving users an explicit get_pty option achieves this goal.
Tests
Commits
Documentation