Code review comment for ~thibf/uvtool:thibf/uvt-kvm_wait_system_booting

Revision history for this message
Robie Basak (racb) wrote :

Thank you for working on this so promptly! Your changes look like they will work, but there are a few things that I think need cleanup up please.

The main issue is that I think that this inappropriately overloads the ssh function. Its definition should not be to ssh but also and also handle its own retries matching a particular string, especially when it's only the wait mechanism that would be using it. And `capture_output` and `retries` are also inappropriately being coupled together. Consider what a description of the function arguments would look like!

Instead, please add support for capture_output to the `ssh` function, let CalledProcessError bubble up to `main_wait_remote`, and handle retry delay there. You will have direct access to args.timeout and args.interval as required.

Some further comments inline.

review: Needs Fixing

« Back to merge proposal