Skip to content

Conversation

@2xic
Copy link
Contributor

@2xic 2xic commented Jul 30, 2025

Port 0 is used by OS to random a random ephemeral port which can be a very useful feature for CI and when you just need to spin up a server quickly.

The current implementation prevents this from being possible as 0 is falsy so we fallback to the default port. This change changes that by checking the type passed in the config to allow passing port number 0.

@2xic 2xic marked this pull request as ready for review July 30, 2025 13:01

await server.start()
const assignedPort = (server as any).port
expect(assignedPort).toBeGreaterThan(0)
Copy link
Contributor

Choose a reason for hiding this comment

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

it should be ensured > 1024, pglite is not meant to run on privileged ports.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you just asking me to add an additional assert, or something else? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

does node socket has warranty to a give a socket > 1024 ? if yes then just use .toBeGreaterThan(1024)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the OS will only not give out privileged any ports. Updated my test to check that port is greater than 1024.

@pmp-p
Copy link
Contributor

pmp-p commented Aug 7, 2025

#765

@2xic 2xic requested a review from pmp-p August 8, 2025 22:10
@pmp-p pmp-p linked an issue Aug 10, 2025 that may be closed by this pull request
@tdrz
Copy link
Collaborator

tdrz commented Aug 11, 2025

@2xic Looks good, thank you! Just one more thing: please mention this new feature in the project's README.md .

@2xic 2xic force-pushed the feature/allow-os-assigned-port branch from 579171e to 0d42b0b Compare August 11, 2025 17:07
@2xic
Copy link
Contributor Author

2xic commented Aug 11, 2025

@2xic Looks good, thank you! Just one more thing: please mention this new feature in the project's README.md .

Added in 0d42b0b

@tdrz tdrz merged commit cab4ce6 into electric-sql:main Aug 12, 2025
18 checks passed
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.

Allow random port assignment

3 participants