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

Consistent PORT allocation #55

Closed
wants to merge 8 commits into from
Closed

Conversation

lxfontes
Copy link

Use PORT environment to start single processes (PaaS style)

Today, It works fine when process name is the first one on Procfile

Example:

root@c2b73725640e:/app# cat Procfile
web: bundle exec unicorn -c config/unicorn.rb -p ${PORT:-3000} -E ${RAILS_ENV:-development}
realtime_linux: /app/realtime/realtime_server.linux -ws $PORT
realtime_mac: ./realtime/realtime_server

root@c2b73725640e:/app# PORT=5001 forego start web
forego          | starting web.1 on port 5001

When process web is NOT the first one on Procfile. Note the requested port 5001 vs allocated 5101

root@c2b73725640e:/app# cat Procfile
realtime_linux: /app/realtime/realtime_server.linux -ws $PORT
web: bundle exec unicorn -c config/unicorn.rb -p ${PORT:-3000} -E ${RAILS_ENV:-development}
realtime_mac: ./realtime/realtime_server

root@c2b73725640e:/app# PORT=5001 forego start web
forego          | starting web.1 on port 5101

This PR will only increment PORT allocation when needed.

@ddollar
Copy link
Owner

ddollar commented Apr 10, 2015

This code seems likely to have some side effects. Would you mind adding some tests around port numbers and PORT?

basePort was getting incremented by 100 on each process start
@lxfontes
Copy link
Author

Found another small bug on port allocation. This line

ps.Env["PORT"] = strconv.Itoa(port)

was changing the main env instead of a copy, thus changing the basePort

@lxfontes
Copy link
Author

tests are failing now that googlecode was taken down 😂

@ddollar
Copy link
Owner

ddollar commented Apr 23, 2016

Sorry for the long delay but please rebase on master and I will merge this. Thanks!

@ddollar ddollar closed this Apr 23, 2016
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.

None yet

3 participants