Remove requests dependency in favor of urllib3 #1653
Conversation
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
jku
left a comment
jku
left a comment
There was a problem hiding this comment.
Looks about right.
The changelog item should mention that this changes the root certificate source (for TLS, not for sigstore specific roots).
I feel like this does not help me that much if I want to see what timeouts and retries are currently used... Should the pool explicitly set the defaults for both of those so it would be clear (unless of course a specific component overrides e.g. the timeout value)?
| # Build fields for query parameters | ||
| fields = None | ||
| if params: | ||
| fields = params |
There was a problem hiding this comment.
fields seems like a useless variable here?
| # Create timeout object if specified | ||
| timeout_obj = None | ||
| if timeout is not None: | ||
| timeout_obj = urllib3.Timeout(connect=timeout, read=timeout) |
There was a problem hiding this comment.
I don't know if this changes anything in practice but in urllib3 the timeout argument default value is not None but a separate placeholder, yet we always set None here.
That said, maybe we should set an actual timeout default here to make it clear?
| session.headers.update( | ||
| { | ||
| "Content-Type": "application/json", | ||
| "Accept": "application/json", |
There was a problem hiding this comment.
Accept header is no longer set I think -- does this matter?
| headers = { | ||
| "Authorization": f"Basic {encoded_credentials}", | ||
| "Content-Type": "application/x-www-form-urlencoded", | ||
| } |
There was a problem hiding this comment.
urllib3.util.make_headers(basic_auth=f"{auth[0]}:{auth[1]}") might be a less involved way to do this
| resp: requests.Response = self.session.get(oidc_config_url, timeout=30) | ||
| except (requests.ConnectionError, requests.Timeout) as exc: | ||
| resp = http.get(oidc_config_url, timeout=30) | ||
| except (urllib3.exceptions.HTTPError, urllib3.exceptions.TimeoutError) as exc: |
There was a problem hiding this comment.
No need to explicitly handle urllib3.exceptions.TimeoutError here, it's an HTTPError.
Applies to all other uses
This drops our
requestsdependency in favor of directly usingurllib3, which we already depend on indirectly.This should be an internal change only, no public changes are expected.
Half me, half Claude.
CC @pnacht for viz; this will unblock us using
urllib3.util.Retry🙂Closes #1148.