-
Notifications
You must be signed in to change notification settings - Fork 99
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
Allow server port to be set via environment variable #220
Conversation
Thanks for your interest in palantir/bulldozer, @f1sherman! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request. |
CLA signed! |
Thanks for proposing this and I'm glad you find Bulldozer useful! I'm not familiar with Heroku - do applications have to use the plain I ask because the library we use for the server, |
@bluekeyes that's a great point. As far as I can tell there is no way to change the environment variable that is used for the port. Doc: https://devcenter.heroku.com/articles/dynos#web-dynos Maybe another way to solve this would be to allow environment variables to be referenced from within the config file, so we could do something like this: server:
port: "${PORT}" I'm not sure how to do that from Go, but it seems possible. |
It's kind of unclear to me if this works when using containers rather than processes directly, but it sounds like the export BULLDOZER_PORT=$PORT (Note that if this works, we'd still need to modify Bulldozer to set configuration from the environment) If that doesn't work and we need to modify the container itself, I propose something a bit meta:
This means that by default we'll have the prefixes for variables, but in Heroku (or other places where you know things are isolated), you can set I think this will be easier to implement and maintain than replacing environment variable references in the configuration file. |
@bluekeyes I love it! I will check in with Heroku support to confirm how |
@bluekeyes after talking with Heroku support it seems that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for following up with Heroku support and iterating with me on this! I have one implementation suggestion that will take advantage of existing code and also support some additional variables for free.
server/server.go
Outdated
if v, ok := os.LookupEnv("BULLDOZER_PORT"); ok { | ||
if i, err := strconv.Atoi(v); err == nil { | ||
c.Server.Port = i | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of only supporting BULLDOZER_PORT
here, I'd like to do something that will support additional variables as well:
In the ParseConfig
function in server/config.go
somewhere around L68, you can add a call to c.Server.SetValuesFromEnv("BULLDOZER_")
. This will parse the BULLDOZER_PORT
variable, but also add support for configuring the listen address, TLS options, and other properties.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds awesome. I will try to get to this in the coming days. Thanks so much for your help and support!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bluekeyes sorry for taking so long to get back to you on this! I looked into adding c.Server.SetValuesFromEnv
, but it appears that the SetValuesFromEnv
method wasn't added until a later version of go-baseapp
than what this project is using. I submitted #228 to hopefully unblock us. Thanks again!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing the update as a separate PR. I've merged that, so you should be good to update this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bluekeyes of course! I think this should be all set now. I rebased for a clean git history and updated the PR description. Thanks again!
c2ab2e2
to
b5e6303
Compare
Heroku expects the web process to dynamically set the listen port based on the PORT variable. This, in combination with a [.profile](https://devcenter.heroku.com/articles/dynos#the-profile-file) to set `PORT=BULLDOZER_PORT`, will make it easier to run bulldozer in Heroku, as well as allowing other HTTP-related config to be set via environment variables.
b5e6303
to
a490697
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I'll try to include this in a release either later today or tomorrow.
Fantastic, thanks so much! Would it be too much trouble to update the Docker image as well? Really appreciate the quick response btw! |
New Docker tags ( |
@bluekeyes I'll give that a try, thanks! |
As suggested [here](palantir#220 (comment)), use a `BULLDOZER_ENV_PREFIX` environment variable to override the prefix for server configuration values. This makes it possible to run the Docker container on Heroku, where it is expected that the container will listen on the port specified in the `PORT` environment variable.
As suggested [here](palantir#220 (comment)), use a `BULLDOZER_ENV_PREFIX` environment variable to override the prefix for server configuration values. This makes it possible to run the Docker container on Heroku, where it is expected that the container will listen on the port specified in the `PORT` environment variable.
As suggested [here](palantir#220 (comment)), use a `BULLDOZER_ENV_PREFIX` environment variable to override the prefix for server configuration values. This makes it possible to run the Docker container on Heroku, where it is expected that the container will listen on the port specified in the `PORT` environment variable.
As suggested [here](palantir#220 (comment)), use a `BULLDOZER_ENV_PREFIX` environment variable to override the prefix for server configuration values. This makes it possible to run the Docker container on Heroku, where it is expected that the container will listen on the port specified in the `PORT` environment variable.
As suggested [here](palantir#220 (comment)), use a `BULLDOZER_ENV_PREFIX` environment variable to override the prefix for server configuration values. This makes it possible to run the Docker container on Heroku, where it is expected that the container will listen on the port specified in the `PORT` environment variable.
Thank you for this amazing tool!
Heroku expects the web process to dynamically set the listen port based on the PORT variable. This, in combination with
a .profile to set
PORT=BULLDOZER_PORT
, will make it easier to run bulldozer in Heroku, as well as allowing other HTTP-related config to be set via environment variables.https://devcenter.heroku.com/articles/container-registry-and-runtime#dockerfile-commands-and-runtime
This is my first time writing Go and I'm sure I didn't get everything exactly right. Please let me know what you'd like me to change here. And of course no worries if this feature cannot be accepted.