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

Enable configuration of PORT and SSL_PORT #14

Merged
merged 2 commits into from
Feb 24, 2023

Conversation

rahearn
Copy link
Contributor

@rahearn rahearn commented Feb 23, 2023

For our use-case, we need to be able to listen on port 8080 instead of 9000. This change enables that while leaving default behavior almost completely unchanged.

  • Existing use cases of docker run -p 9000:9000... will continue to work unchanged
  • Anyone using docker run -P to automatically map container ports to random host ports will need to add --expose=9000 --expose=9443 to their command to maintain the same behavior.

If we were running entirely in docker, we could have done this with -p 8080:9000 but our environment actually unpacks the docker image and repacks it in another buildpack, so we can't use the -p flag and instead have to rely on $PORT

@rahearn rahearn marked this pull request as draft February 23, 2023 21:43
@rahearn
Copy link
Contributor Author

rahearn commented Feb 23, 2023

Converting this to draft as I work out some issues with this implementation with our provider.

Copy link

@mogul mogul left a comment

Choose a reason for hiding this comment

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

We should probably leave the defaults in there and override them with the content of the env vars only if they're present. That will also maintain existing behavior in the binary for cases where the port hasn't been specified in the environment previously.

@rhermanek rhermanek merged commit 96553c5 into ajilach:master Feb 24, 2023
@rahearn rahearn deleted the configure-port branch February 24, 2023 13:51
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.

3 participants