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

SNOW-830959: Refactor of custom http handler setup #668

Closed

Conversation

sfc-gh-knozderko
Copy link
Collaborator

Refactor of HttpUtil class to extract logic of creating http handler to external classes.
It improves the code by applying Single Responsibility Principle and Open-Closed Principle to the refactored code.
A test was added for creating or not a web proxy based on configuration. The need to add this test was inspired by this ticket: SNOW-828572.

@sfc-gh-knozderko sfc-gh-knozderko requested a review from a team as a code owner June 1, 2023 17:08
@github-actions
Copy link

github-actions bot commented Jun 2, 2023

CLA Assistant Lite bot:
Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.


I have read the CLA Document and I hereby sign the CLA


You can retrigger this bot by commenting recheck in this Pull Request

sfc-gh-igarish
sfc-gh-igarish previously approved these changes Jun 2, 2023
Copy link
Collaborator

@sfc-gh-igarish sfc-gh-igarish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sfc-gh-igarish sfc-gh-igarish self-requested a review June 2, 2023 17:51
@sfc-gh-igarish sfc-gh-igarish dismissed their stale review June 2, 2023 17:53

Simba is planning to remove WinHTTPHandler and they are in middle of changes. So please do it after that.

@ghost
Copy link

ghost commented Jun 2, 2023

Hi @sfc-gh-knozderko

I think better to hold on this change as we are considering removing the use of WinHttpHandler.
https://github.com/snowflakedb/snowflake-sdks-drivers-issues-teamwork/issues/365
It was introduced to the driver for fixing a proxy issue while it doesn't fully fix the issue (the proxy issue was reported on Linux as well while WinHttpHandler only available on Windows) so we might need to switch back to use HttpClientHandler in all cases.

@sfc-gh-knozderko
Copy link
Collaborator Author

Will not be merged

@github-actions github-actions bot locked and limited conversation to collaborators Sep 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants