Skip to content

Rename port envvar#34

Open
kpacha wants to merge 5 commits intodrone:v2from
kpacha:rename_port_envvar
Open

Rename port envvar#34
kpacha wants to merge 5 commits intodrone:v2from
kpacha:rename_port_envvar

Conversation

@kpacha
Copy link

@kpacha kpacha commented Mar 6, 2016

Do not use the $PORT env var because it doesn't allow you to use drone-wall in mesos environments.

@Tathanen
Copy link
Contributor

Tathanen commented Mar 6, 2016

In the readme there, you've replaced port with wall_port but in actuality, that first port was referring to api_port. It's kind of a confusing sentence, all of the vars there are implicitly prefixed by api_. The port->wall_port change you're making is for a var that was never mentioned in the readme in the first place, so you'll need to add a new sentence just for it (and maybe just add the api_ bits to that list of vars for clarity).

@scottferg would you mind vetting the actual code changes here? I'm not Docker-savvy enough to really be able to parse most of it. For context, this is being added to a separate branch for the v2 version, I'm not sure if the :2.1 in there relates to any github release notation tho.

$ docker run -p 3000:8080 -e API_SCHEME=$API_SCHEME -e API_DOMAIN=$API_DOMAIN \
-e API_TOKEN=$API_TOKEN -e API_PORT=$API_PORT -e WALL_PORT=8080 scottwferg/drone-wall:2.1

Drone wall exposes port `3000`. You can map this to whatever you like.

Choose a reason for hiding this comment

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

This makes no sense anymore then.

The WALL_PORT is the port that this application is going to bind to within the Docker container - therefore if the WALL_PORT is 8080, then exposing port 3000 is completely useless.

Copy link
Author

Choose a reason for hiding this comment

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

I agree but I don't want to change the way people is used to work with the v2 branch so here I was just trying to demonstrate how could it be used

@flyinprogrammer
Copy link

Also @kpacha if you're using marathon to do docker deploys on mesos then this change isn't actually required. You just have to know about some marathon wizardry whereby when you setup the port ask in your POST to marathon, set containerPort = 0; then marathon with expose, and set PORT to the ephemeral port it finds on the host:

From the docs:

However, if you have set containerPort to 0 then this will be the same as hostPort and you can use the $PORT environment variables.

-e API_TOKEN=$API_TOKEN -e API_PORT=$API_PORT scottwferg/drone-wall
$ docker pull scottwferg/drone-wall:2.1
$ docker run -p 3000:8080 -e API_SCHEME=$API_SCHEME -e API_DOMAIN=$API_DOMAIN \
-e API_TOKEN=$API_TOKEN -e API_PORT=$API_PORT -e WALL_PORT=8080 scottwferg/drone-wall:2.1
Copy link
Contributor

Choose a reason for hiding this comment

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

We should get this moved into the proper repo

@scottferg
Copy link
Contributor

LGTM

@Tathanen
Copy link
Contributor

Tathanen commented Mar 6, 2016

@kpacha if you wanna make those doc updates (that table one in particular) I'll get this merged in.

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.

4 participants