-
Notifications
You must be signed in to change notification settings - Fork 818
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
Fix wait for url #5024
Fix wait for url #5024
Conversation
@@ -476,8 +477,10 @@ def test_metadata_ipv4_404(self, dhcpv4, ds_detect): | |||
self.assertIsNone(self.datasource.get_userdata_raw()) | |||
self.assertIsNone(self.datasource.get_vendordata_raw()) | |||
|
|||
@mock.patch("cloudinit.url_helper.time.sleep", lambda x: None) | |||
@mock.patch("cloudinit.url_helper.time.time", side_effect=count()) |
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.
Do we need the time mock, the sleep mock looks like it'll suffice.
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.
LGTM
cloudinit/url_helper.py
Outdated
sleep_time: Amount of time to sleep between retries. If this and | ||
sleep_time_cb are None, the default sleep time | ||
defaults to 1 second and increases by 1 seconds every 5 | ||
tries. |
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.
nit: Let's document mutual exclusion with sleep_time_cb
here too.
cloudinit/url_helper.py
Outdated
|
||
def timeup(max_wait, start_time): | ||
def timeup(max_wait: float, start_time: float, sleep_time: float = 0): | ||
"""Check if time is up based on start time and max wait""" | ||
if max_wait is None: |
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.
Do we also want to check if max_wait in (None, float("inf"))
here to avoid extra calculations below?
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.
two minor nits. otherwise +1.
In test_scaleway.py, test_metadata_connection_errors_legacy_ipv4_url assumes that a call to wait_for_url() will retry the number of retries specified by the datasource, but that retry number is used for readurl() calls, not wait_for_url() calls. The fact that the assertion happened to work previously was entirely incidental. Additionally, mock time.time() and time.sleep() calls so that we're not needlessly waiting in unit tests.
4ace169
to
ca82a38
Compare
Proposed Commit Message
Merge type