Skip to content
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

Support proxy per-connection rather than just environment variables #320

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jakewski
Copy link

@jakewski jakewski commented Jan 16, 2024

Problem

I opened this issue: #318. My use case involves using the Databricks SQL connector in a multi-threaded web application where we may not want to set the proxy for all traffic. The current method of using environment variables would not allow that.

Solution

Proxy specification works same way as the Python requests module - it will default to whatever is set in the environment variable, but it can manually be overridden for a single request / connection if desired.

@jakewski
Copy link
Author

I would like to add a test for this in client tests - but seems they don't actually run in CI. Any guidance would be much appreciated! #317

@susodapop
Copy link
Contributor

Thanks for this contribution. We have rather poor testing facilities for proxies at present (we need proper CI for this repository -- that's coming soon). Will review and perform some manual testing in the next couple weeks.

@susodapop susodapop self-assigned this Jan 25, 2024
@jakewski
Copy link
Author

Thanks for this contribution. We have rather poor testing facilities for proxies at present (we need proper CI for this repository -- that's coming soon). Will review and perform some manual testing in the next couple weeks.

sounds good, appreciate the review!

@kravets-levko
Copy link
Contributor

Hi @jakewski! Sorry for not getting back for so long. First of all, thank you for your effort - this indeed is a useful feature to have in this library. The changes you made look good. However, there are two more places that need to be updated as well:

  • OAuth manager. OAuth endpoints are located on the same host as Thrift backend, so if you need proxy to execute query - you definitely need the same proxy for OAuth
  • CloudFetch handler - same reasoning as above.

Indeed, we can merge this as is, but in this case the feature will be limited, and eventually other users will complain about it

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.

3 participants