-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 socks proxy #2541
Support socks proxy #2541
Conversation
8e193f0
to
c161478
Compare
Hi @stobrien89 |
Hi @zhan9san, Thanks for your patience. This will likely require a review by a few team members— I'll try to get some additional eyes on this. |
@stobrien89 any idea when those are coming? Maintaining private fork is painful. |
This patch has been forward ported a few times now. It includes a test case. At @aiven we have been running this in production for a very long time. It would be nice if we could stop manually patching botocore :) |
This is a crucial feature for us. |
nth-ing this for the same reason as adelsz: SSH jump servers via SSM work well with most of the other tools we use but anything boto-based requires periodically rebasing this patch. |
@stobrien89 I guess I can ask about reviews again - it has been year. |
@stobrien89 it would be great if this could be reviewed. |
Apologies for the delay. I don't work with the botocore maintainers anymore. @kdaily, @tim-finnigan, or @dlm6693 can probably help you get this looked at. |
@zhan9san are you still working on this? @kdaily, @tim-finnigan or @dlm6693 what needs to be done to get this reviewed |
If the maintainers have time to review it, I am glad to resolve the confilicts. I was a bit disappointed honestly. |
Pinging @kdaily, @tim-finnigan, or @dlm6693 one more time. If this fails to get traction again, I will give up on upstream and initiate work to patch this into fedora. But that would mean everyone else loses a chance to have this commonly requested feature in botocore :( But we have waited for almost two years now to get this merged in. |
Hi all, thanks for your patience here and thanks @zhan9san for creating this PR. The team is still tracking this feature request in the corresponding issue (#2540). Although folks have mentioned that this patch is working fine, the team wants to do a more comprehensive review before taking on a new dependency. Especially considering that PySocks has not been updated since 2019. We have a backlog item for further investigation but at this time cannot provide any guarantees on if or when this would be supported. |
Is there anything i can do to help speed up the review? |
tested fine, nice job , when can we merge it ? |
Thanks again for your patience here. We brought this PR up with the team again for further review and discussion, and decided to close it as not under consideration at this time. We will keep #2540 open to continue gathering community input and may revisit this PR at a later date. As mentioned earlier, adding a dependency that has not updates since 2019 is still a concern. |
Fix #2540 and #880
I got approval from the author of #2081 to continue.
Could you please review this PR and approve it?
Test code:
export https_proxy=socks5://127.0.0.1:50010 http_proxy=socks5://127.0.0.1:50010
BTW, it works only if proxies are set in environment while it doesn't work if proxies are set in Specifying proxy servers as http proxy doesn't work in configuration either.