Skip to content
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

tests: add --quiet switch to retry-tool #7397

Merged
merged 1 commit into from Sep 4, 2019

Conversation

zyga
Copy link
Collaborator

@zyga zyga commented Sep 3, 2019

Pawel made a case for using retry-tool in situations where we know and
anticipate the first few attempts of a command to fail, e.g. while
waiting for some asynchronous operation to finish. In that case the
default output doesn't help much, as we routinely fail on the road to
eventual success.

For this use case, add the --quiet switch, to silence all output from
the tool itself.

Signed-off-by: Zygmunt Krynicki me@zygoon.pl

Pawel made a case for using retry-tool in situations where we know and
anticipate the first few attempts of a command to fail, e.g. while
waiting for some asynchronous operation to finish. In that case the
default output doesn't help much, as we routinely fail on the road to
eventual success.

For this use case, add the --quiet switch, to silence all output from
the tool itself.

Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
@zyga zyga added the Simple 😃 A small PR which can be reviewed quickly label Sep 3, 2019
time.sleep(wait)
else:
if n > 1:
if verbose and n > 1:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We expected the command to succeed in n attempts, so reaching here is unexpected. Wonder if we should print the message anyway. Makes the switch named --quiet a bit akward, so maybe --no-log then?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about it as well, oh oh :) Such a hard choice.

So here's my proposal instead. I was thinking about this earlier but decided not to do it because YAGNI but this shows it may have been useful:

The -n switch can now be both a number and a range n-m. When it is a range it indicates that we expect the first n attempts to fail and those automatically become quiet. After those the remaining up-to-m attempts log as usual.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a followup material.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm happy to iterate on this some more. We can always respect --quiet even if we have the range syntax implemented.

Copy link
Collaborator

@bboozzoo bboozzoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

time.sleep(wait)
else:
if n > 1:
if verbose and n > 1:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a followup material.

Copy link
Contributor

@stolowski stolowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, +1

@zyga zyga merged commit bfe4456 into snapcore:master Sep 4, 2019
@zyga zyga deleted the tweak/retry-quiet-switch branch September 4, 2019 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Simple 😃 A small PR which can be reviewed quickly
Projects
None yet
3 participants