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.

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

fix(server): ensure using a free port for dev console #5163

Merged
merged 7 commits into from Sep 29, 2023
Merged

fix(server): ensure using a free port for dev console #5163

merged 7 commits into from Sep 29, 2023

Conversation

vvagaytsev
Copy link
Collaborator

What this PR does / why we need it:

Which issue(s) this PR fixes:

Patches #4984
Fixes #5158

Special notes for your reviewer:

@vvagaytsev vvagaytsev mentioned this pull request Sep 29, 2023
Closed
@vvagaytsev
Copy link
Collaborator Author

@eysi09 should we use defaultWatchServerPort = 9777 or defaultServerPort = 9700 as a default port value for serve and dev commands?

The --help documentation says it should be 9700. So I'll change it to be defaultServerPort = 9700.

@vvagaytsev vvagaytsev requested review from eysi09 and a team September 29, 2023 12:48
@vvagaytsev vvagaytsev marked this pull request as ready for review September 29, 2023 12:48
@TimBeyer
Copy link
Contributor

How does the front end know to connect to this port? Do we store that info on the session? Or is this another different server?

@vvagaytsev
Copy link
Collaborator Author

@TimBeyer we set it as localServerPort when we call CloudApi.registerSession.

@vvagaytsev
fix(server): ensure using a free port for dev console
96c846a
@vvagaytsev
fix(server): don't swallow errors on server start
96afc96 
Also print the error message and exit.
@vvagaytsev
fix(server): better error handling on server start
2932e50 
* Fail if the explicitly specified port is empty
* Always retry on `EADDRINUSE` error if the port hasn't been specified explicitly
* Always fail on any errno error expect `EADDRINUSE`
* Log the port the server has started at
@vvagaytsev
fix(server): use correct default port value
dbd3380
@vvagaytsev
fix(server): correct port range
8bc0fba
TimBeyer
TimBeyer previously approved these changes Sep 29, 2023
View reviewed changes
@vvagaytsev
fix: remove unnecessary port range
f28d0b3 
Alternative port range makes so sense when `port` is specified:
https://github.com/unjs/get-port-please
@vvagaytsev vvagaytsev requested review from TimBeyer and a team September 29, 2023 14:07
TimBeyer
TimBeyer previously approved these changes Sep 29, 2023
View reviewed changes
TimBeyer
TimBeyer previously approved these changes Sep 29, 2023
View reviewed changes
@vvagaytsev vvagaytsev marked this pull request as draft September 29, 2023 15:28
@vvagaytsev
Copy link
Collaborator Author

Now the tests are green

@vvagaytsev vvagaytsev marked this pull request as ready for review September 29, 2023 16:16
@vvagaytsev vvagaytsev requested review from a team and TimBeyer September 29, 2023 16:16
@vvagaytsev vvagaytsev added this pull request to the merge queue Sep 29, 2023
Merged via the queue into main with commit 3a17402 Sep 29, 2023
41 checks passed
@vvagaytsev vvagaytsev deleted the fix/5158 branch September 29, 2023 16:58
@eysi09
Copy link
Collaborator

eysi09 commented Oct 4, 2023

cc @trymbill

This has already been merged but wondering if the work you've been doing makes assumptions about ports?

@trymbill
Copy link
Contributor

trymbill commented Oct 4, 2023

[...] if the work you've been doing makes assumptions about ports?

Desktop handles grabbing a free port by itself (see: https://github.com/garden-io/garden-platform/blob/61a0d724716a5324400bbe903af912aa4c8b3dd8/desktop/src-tauri/src/garden_core_process_manager.rs#L89-L95), so this shouldn't change anything on that end.

vvagaytsev added a commit that referenced this pull request Oct 4, 2023
@vvagaytsev
chore(server): restore the previous default server port number 9777
86c3540 
Patches #5163.
@vvagaytsev vvagaytsev mentioned this pull request Oct 4, 2023
Merged
github-merge-queue bot pushed a commit that referenced this pull request Oct 4, 2023
@vvagaytsev
chore(server): restore the previous default server port number 9777 (…
64344b4 
…#5185)

Patches #5163.
@stefreak stefreak mentioned this pull request Nov 2, 2023
Closed
@vvagaytsev vvagaytsev mentioned this pull request Nov 7, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TimBeyer TimBeyer TimBeyer approved these changes

@eysi09 eysi09 Awaiting requested review from eysi09

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Garden server fails silently on port conflicts
4 participants
@vvagaytsev @TimBeyer @eysi09 @trymbill