-
Notifications
You must be signed in to change notification settings - Fork 68
Added merge environmental settings to prepared request #1811
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
Conversation
libs/labelbox/src/labelbox/client.py
Outdated
prepped: requests.PreparedRequest = request.prepare() | ||
|
||
response = self._connection.send(prepped, timeout=timeout) | ||
prepped: requests.PreparedRequest = self._connection.prepare_request( |
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 do not believe we should be using prepare_request
on the Session itself... unfortunately so. This is because we are modifying headers above and if you use prepare_request on the session it will prb merge those headers back.
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.
So just to test I printed out both the request headers and prepped headers
{
"User-Agent": "python-requests/2.32.3",
"Accept-Encoding": "gzip, deflate",
"Accept": "application/json",
"Connection": "keep-alive",
"Authorization": "Bearer <API_KEY>",
"Content-Type": "application/json",
"X-User-Agent": "python-sdk 4.0.0",
"X-Python-Version": "3.8.18-final",
}
{
"User-Agent": "python-requests/2.32.3",
"Accept-Encoding": "gzip, deflate",
"Accept": "application/json",
"Connection": "keep-alive",
"Authorization": "Bearer <API_KEY>",
"Content-Type": "application/json",
"X-User-Agent": "python-sdk 4.0.0",
"X-Python-Version": "3.8.18-final",
"Content-Length": "733",
}
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.
Ahh I see what your saying. I modified it to add the correct headers to the prepped response. I believe that is what the docs recommended to allow the merge_environment_settings
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.
Moved this back to just prepped but did the merge setting still
bb7724a
to
a3da2da
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.
@Gabefire we really need to confirm this via testing... how can we do that?
Description
REQUESTS_CA_BUNDLE