Skip to content

Conversation

@zhu-tom
Copy link
Collaborator

@zhu-tom zhu-tom commented Apr 24, 2025

No description provided.

@zhu-tom zhu-tom marked this pull request as ready for review April 24, 2025 19:59
@elghali97
Copy link

Nice thanks for the PR @zhu-tom !

@moderakh moderakh self-requested a review April 25, 2025 16:45
Copy link
Collaborator

@moderakh moderakh left a comment

Choose a reason for hiding this comment

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

@zhu-tom The change looks good to me as a custom modification to support proxy usage when interacting with the server. However, please do not merge the PR.

Thank you

@moderakh moderakh self-requested a review April 25, 2025 16:49
@moderakh moderakh changed the title Add DeltaSharingClient Proxy Configuration [Do not merge] Add DeltaSharingClient Proxy Configuration Apr 25, 2025
Copy link

@bob-hodgeman-kr bob-hodgeman-kr left a comment

Choose a reason for hiding this comment

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

Upon testing this PR, I found that I was unable to make an https call via an http proxy. I've tested the proposed fix and basic testing shows that it is working.

new HttpRoute(target)
} else {
// Route via proxy
new HttpRoute(target, proxy)

Choose a reason for hiding this comment

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

This constructor always constructs an insecure (http) route through the proxy. This does not work if one is using an http proxy to make an https call to the server. Instead, consider using an alternate constructor and setting the secure flag appropriately based on the target:

Suggested change
new HttpRoute(target, proxy)
val isSecure = target.getSchemeName == "https"
new HttpRoute(target, null, proxy, isSecure)

@sevann71
Copy link

Hi team,
Do you know when this fix will be merged?
Thanks a lot

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.

5 participants