[1LP][RFR] Prepare connect_ssh#10056
[1LP][RFR] Prepare connect_ssh#10056mshriver merged 7 commits intoManageIQ:masterfrom jarovo:connect_ssh
Conversation
|
Jaroslav, I'll take a look at this, but I am not an official approver. We'll need to get others involved in this review. I am still trying to understand all of the deficiencies this PR was meant to fix. I was just trying to use connect_ssh in PR#9952. I am very willing to help with this where I can, just want to get the proper support. |
The names of methods and places where they were used (related to being "private" and "public"). Also, I was asked to check why I cannot use wait_for. I found I can use it and do not have to reinvent the wheel. |
Having the rounds parameter may not be the best idea. Perhaps it is better to make it mandatory parameter. Setting it to math.inf will result in ininite ammount of attempts to connect.
|
I created a local copy of the branch for this PR and was trying to run test_snapshot.py::verify_revert_snapshot with the updates I had made earlier to make this work. I am getting a timeout here |
prichard77
left a comment
prichard77
left a comment
There was a problem hiding this comment.
Cool Jarsolav, I didn't know I had the permissions to do this. I knew I could leave comments, but not review. For clarity I am re-posting the comment I made to the general PR.
I created a local copy of the branch for this PR and was trying to run test_snapshot.py::verify_revert_snapshot with the updates I had made earlier to make this work. I am getting a timeout here connection, _ = wait_for(try_batch_of_ips, num_sec=num_sec, delay=delay) net.py line 123. I am still looking to see if I did something in the test to cause this, but wanted to let you know.
I'll be testing more and I'll post back when/if I find anything additional.
john-dupuy
left a comment
john-dupuy
left a comment
There was a problem hiding this comment.
2 small comments but LGTM, nice PR @JaryN!
Add more doc, accept only kwargs.
cfme/utils/ssh.py
Outdated
| * The SSHClient does not connect in the object creation time. | ||
| """ | ||
| if kwargs['hostname'] is None: | ||
| raise ValueError(f'The hostname argument cannot be None!') |
There was a problem hiding this comment.
This doesn't need to be an f-string anymore.
|
I detected some fixture changes in commit c225cd8 The global fixture
The global fixture
The global fixture
The global fixture
The local fixture
The local fixture
The local fixture
Please, consider creating a PRT run to make sure your fixture changes do not break existing usage 😃 |
6 participants
Prepare connect_ssh for the use in vm snapshot tests.
{{pytest: -v --use-provider complete --long-running cfme/utils/tests/test_connect_ssh.py cfme/tests/configure/test_proxy.py cfme/tests/cli/test_appliance_console_db_restore.py}}
I see no failures related to this PR. I see failures like: http://10.16.45.124/pr10056-r2183-downstream-510z-yfvrVf/artifacts/report.html
Connection reset by peer is bugging me long time.
Then there are timeouts caused by other than connect_ssh reslated things.