fix: machine scp & ssh#1020
fix: machine scp & ssh#1020jujubot merged 16 commits intojuju:masterfrom yanksyoon:fix/machine_ssh_cherry_picked
Conversation
1 similar comment
cderici
left a comment
cderici
left a comment
There was a problem hiding this comment.
Thanks for opening this ❤️ with just a couple changes in the tests and then we can land this 👍
tests/integration/test_machine.py
Outdated
| async def test_machine_ssh(): | ||
| async with base.CleanModel() as model: | ||
| machine: Machine = await model.add_machine() | ||
| out = await machine.ssh("echo hello world!") |
There was a problem hiding this comment.
Same redundancy here, see the test_ssh test in integration/test_unit.py, unit.ssh is just a wrapper around the machine.ssh.
There was a problem hiding this comment.
Added wait_for_active parameter here - this should no longer be redundant :)
|
Hi @yanksyoon, just pinging for this to let you know we're planning on making a release soon, so I'd love to roll this in if it's ready as well 👍 |
|
@cderici Sorry! I was on my break, i'll get on this asap! |
@yanksyoon np, hope you had a good break! Feel free to ping me whenever this is ready for review again 👍 |
|
@cderici The tests seem to have failed due to changes in how juju handles old charm-store ( |
cderici
left a comment
cderici
left a comment
There was a problem hiding this comment.
Yeah the CI failures are unrelated to this change. This can land.
One final request though, the wait_for_active should be an opt-in feature, so let's have False as the default value, as this would currently block any code that uses this without this parameter if the machine is not ready. Granted, it would be weird for them to rely on failure, but still, I'd steer away from changing default semantics on this. Hope this makes sense.
Thanks for working on this! <3
|
@cderici Changed! Thanks for the review! :D |
There's a bit of a gap between the time that the machine is assigned an IP and the ssh service is up and listening, which creates a race for the ssh command (i.e. we may run the ssh command after the machine gets the IP but before the ssh service is up). So we retry a couple of times to mitigate that effect until either the ssh command succeeds, or we run out of attempts (i.e. something else is going on).
|
@yanksyoon looks like the |
In a real world scenario, this will work in the first iteration (maybe the second after 2sec backoff), however the github workers are particularly slow so extending this to make the tests pass.
|
/build |
|
/build |
|
/build |
|
/merge |
3 participants
Description
This is a follow-up PR to #1016 , which contains cherry-picked commits without type imports.
Fİxes #997
QA Steps
<Commands / tests / steps to run to verify that the change works:>
All CI tests need to pass.
<Please note that most likely an additional test will be required by the reviewers for any change that's not a one liner to land.>
Notes & Discussion
<Additional notes for the reviewers if needed. Please delete section if not applicable.>