Skip to content

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

Merged
merged 7 commits into from
Sep 24, 2024

Conversation

Gabefire
Copy link
Collaborator

@Gabefire Gabefire commented Sep 12, 2024

Description

  • User mentions they are wanting to use self-signed SSL certs for the SDK but unable to do so since our SDK does not take account of the environmental variable REQUESTS_CA_BUNDLE
  • reference these docs for more details
  • But I essentially implemented the recommended solution

prepped: requests.PreparedRequest = request.prepare()

response = self._connection.send(prepped, timeout=timeout)
prepped: requests.PreparedRequest = self._connection.prepare_request(
Copy link
Contributor

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.

Copy link
Collaborator Author

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",
}

Copy link
Collaborator Author

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

Copy link
Collaborator Author

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

Copy link
Contributor

@vbrodsky vbrodsky left a 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?

@vbrodsky vbrodsky merged commit 929ca7f into develop Sep 24, 2024
18 of 26 checks passed
@vbrodsky vbrodsky deleted the gu/added_merge_setting branch September 24, 2024 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Labelbox Client usage of requests Library doesn't support the verify argument so it ignores REQUESTS_CA_BUNDLE Environment Variable
2 participants