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

Revision history for this message
Thibf (thibf) 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.

Indeed, this makes sense.
See the update, I think it match what you shared.

I enabled retries only when we find the matching string which is the best in the case of the linked issue.
But it may make sense in the future to always retries for generic errors until timeout occur.

« Back to merge proposal