8000 Replace socks with socks5 in proxy protocol by wh201906 · Pull Request #1776 · arduino/arduino-ide · GitHub
[go: up one dir, main page]
Skip to content

Replace socks with socks5 in proxy protocol #1776

< 10000 details class="details-reset details-overlay details-overlay-dark float-right" > 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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Dec 21, 2022
Merged

Replace socks with socks5 in proxy protocol #1776

merged 1 commit into from
Dec 21, 2022

Conversation

wh201906
Copy link
Contributor

Motivation

Fixes #1769
The net/http package in arduino-cli supports socks5 as scheme rather than socks. This PR replaces socks with socks5 in proxy protocol to make socks proxy work.

Change description

  1. Replaces socks with socks5 in proxy protocol
  2. Show SOCKS5 in Preferences->Network

Other information

Reviewer checklist

  • PR addresses a single concern.
  • The PR has no duplicates (please search among the Pull Requests before creating one)
  • PR title and description are properly filled.
  • Docs have been added / updated (for bug fixes / features)

@wh201906 @per1234
Replace socks with socks5 in proxy protocol
c05d261 
The net/http package in arduino-cli supports socks5 as scheme rather than socks.

Co-authored-by: per1234 <accounts@perglass.com>
@CLAassistant
Copy link
CLAassistant commented Dec 16, 2022 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@per1234 per1234 added topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project topic: CLI Related to Arduino CLI labels Dec 16, 2022
@wh201906
Copy link
Contributor Author

I tested it on Windows. Both socks proxy and http proxy work

// after setting a socks5 proxy
2022/12/16 15:52:30 tcp:127.0.0.1:51372 accepted tcp:downloads.arduino.cc:443 [socks -> proxy]
2022/12/16 15:52:31 tcp:127.0.0.1:51374 accepted tcp:raw.githubusercontent.com:443 [socks -> proxy]
// after setting a http proxy
2022/12/16 15:52:54 127.0.0.1:51388 accepted //downloads.arduino.cc:443 [http -> proxy]
2022/12/16 15:52:55 127.0.0.1:51390 accepted //raw.githubusercontent.com:443 [http -> proxy]

I used Windows_X86-64_zip in the build artifacts for test
https://github.com/arduino/arduino-ide/actions/runs/3710694526

@kittaakos kittaakos assigned kittaakos and unassigned kittaakos Dec 16, 2022
@kittaakos kittaakos self-requested a review December 16, 2022 08:00
kittaakos
kittaakos reviewed Dec 16, 2022
View reviewed changes
Copy link
Contributor
@kittaakos kittaakos left a comment

Choose a reason for hiding this comment

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

Thank you for the help. The code looks good. I agree SOCKS5 would better qualify the protocol in the UI, but the (old) IDE 1.x shows SOCKS.

Screen Shot 2022-12-16 at 17 29 56

We should consider that tutorials, documentation, and screenshots might exist with SOCKS on the Internet. I am OK with the change too.

@wh201906
Copy link
Contributor Author

@kittaakos Shall I replace the SOCKS5 with SOCKS back in the Network panel?

@kittaakos
Copy link
Contributor

Shall I replace the SOCKS5 with SOCKS back in the Network panel?

I am OK with both. If you do not change it to SOCKS5, there is nothing to discuss UI-wise. Hence the review will be faster. If you prefer to change the UI, we must discuss it with the designers first. It's up to you.

@wh201906
Copy link
Contributor Author

I prefer to change the UI. Thanks for your reply.

@kittaakos kittaakos requested review from 91volt and ubidefeo December 16, 2022 17:15
per1234
per1234 approved these changes Dec 16, 2022
View reviewed changes
Copy link
Contributor
@per1234 per1234 left a comment

Choose a reason for hiding this comment

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

I verified this fixes #1769

Thanks @wh201906!

@kittaakos kittaakos self-requested a review December 21, 2022 10:14
kittaakos
kittaakos approved these changes Dec 21, 2022
View reviewed changes
Copy link
Contributor
@kittaakos kittaakos left a comment

Choose a reason for hiding this comment

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

I am OK merging the PR as is, including the UI changes, as it fixes basic IDE2 functionality. We can open a follow-up and figure out internally what to do with the UI.

@wh201906
Copy link
Contributor Author

I am OK merging the PR as is, including the UI changes, as it fixes basic IDE2 functionality. We can open a follow-up and figure out internally what to do with the UI.

Do I need to revert the UI changes in this PR, then open a new PR for it?

@kittaakos kittaakos merged commit 3f05396 into arduino:main Dec 21, 2022
@kittaakos kittaakos mentioned this pull request Dec 21, 2022
Closed
3 tasks
@kittaakos
Copy link
Contributor

Follow-up: #1782

@wh201906 wh201906 deleted the fix-socks branch December 21, 2022 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kittaakos kittaakos kittaakos approved these changes

@per1234 per1234 per1234 approved these changes

@91volt 91volt Awaiting requested review from 91volt

@ubidefeo ubidefeo Awaiting requested review from ubidefeo

Assignees
No one assigned
Labels
topic: CLI Related to Arduino CLI topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Doesn't work with SOCKS proxy
4 participants
@wh201906 @CLAassistant @kittaakos @per1234
0