-
Notifications
You must be signed in to change notification settings - Fork 0
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
Remove urlwait in favor of waitfor #35
Conversation
c022861
to
79685a6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not currently set up to test obs-common, but I figured I'd do this comment and then test tomorrow.
obs_common/waitfor.py
Outdated
@@ -28,6 +46,9 @@ def main(args=None): | |||
) | |||
parser.add_argument("--verbose", action="store_true") | |||
parser.add_argument("--timeout", type=int, default=15, help="Wait timeout") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I documented this poorly--it's very confusing. What's really going on is that waitfor will keep attempting with a 5-second timeout until this timeout parameter is exceeded.
At some point, we should either rename this argument or fix the documentation. We don't have to do that in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed the --timeout
help string to "Seconds after which to stop retrying. This is separate from the timeout for individual attempts, which is 5 seconds."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggested a couple of minor changes, but otherwise this is good.
At some point, we should change the name of the timeout
argument and docs so that is clearer, but we can do that later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
default=15, | ||
help=( | ||
"Seconds after which to stop retrying. This is separate from the timeout " | ||
"for individual attempts, which is 5 seconds." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
as suggested in #28 (comment)
adds support for non-http services based on https://github.com/pmac/urlwait/blob/master/urlwait.py#L89 so that socorro and tecken can also drop urlwait
also adds missing
make updatereqs
command to match other obs team reposfixes #38