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

Add option to server for --pick-port to start the server on a port within a range #136729

Merged
merged 6 commits into from
Nov 14, 2021

Conversation

tanhakabir
Copy link
Contributor

@roblourens
Copy link
Member

Not sure how I feel about this, when we could use --port then just shut down the server if it goes out of range. Is there a big disadvantage to doing it that way? On the other hand this flag seems straightforward and useful. Thoughts @alexdima?

@tanhakabir
Copy link
Contributor Author

tanhakabir commented Nov 11, 2021

Not sure how I feel about this, when we could use --port then just shut down the server if it goes out of range. Is there a big disadvantage to doing it that way? On the other hand this flag seems straightforward and useful. Thoughts @alexdima?

Shutting down the server on the remote-ssh extension side was a lot more code change so I thought this was cleaner than changing a bunch of logic in remote-ssh.

Also I'm not sure if it's okay to connect to a port outside the specified range if they set a specific range for those who want to whitelist a range of ports for remote-ssh.

Add option to server for port range to connect within

Fix semicolon linting issue

Add `pick-port` to list of accepted args
Copy link
Member

@alexdima alexdima left a comment

Choose a reason for hiding this comment

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

👍 I pushed a change to fix handling of parseInt errors. For whatever reason, the old code was using a try/catch around parseInt, but parseInt simply returns NaN when it cannot parse something. Also, we should always pass base 10 to it.

src/vs/server/main.js Outdated Show resolved Hide resolved
src/vs/server/main.js Outdated Show resolved Hide resolved
Copy link

@raghavthind2005 raghavthind2005 left a comment

Choose a reason for hiding this comment

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

The code looks good to me!

@roblourens roblourens merged commit 035eaab into main Nov 14, 2021
@roblourens roblourens deleted the tanha/remote/issue-1386 branch November 14, 2021 21:59
@github-actions github-actions bot locked and limited conversation to collaborators Dec 29, 2021
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.

4 participants