-
Notifications
You must be signed in to change notification settings - Fork 824
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
Support referencing nginx-gen container indirectly through another environment variable #126
Conversation
@ento can you fix the code with the reviews i have made ? |
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.
Can you made the changes from my reviews ?
@@ -75,7 +75,16 @@ source /app/functions.sh | |||
if [[ "$*" == "/bin/bash /app/start.sh" ]]; then | |||
check_docker_socket | |||
if [[ -z "${NGINX_DOCKER_GEN_CONTAINER:-}" ]]; then | |||
[[ -z "${NGINX_PROXY_CONTAINER:-}" ]] && get_nginx_proxy_cid | |||
if [[ ! -z "${NGINX_DOCKER_GEN_LINKED_CONTAINER_ENVVAR:-}" ]]; then |
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.
It 's better to use
if [[ -n
than
if [[ ! -z
# `<alias>_NAME` that gets set by Docker when linking containers. | ||
# We need to chop off the `/` prefix to be able to use its | ||
# value in Docker Remote API calls. | ||
export NGINX_DOCKER_GEN_CONTAINER=$(echo ${!NGINX_DOCKER_GEN_LINKED_CONTAINER_ENVVAR} | sed 's/^\///') |
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.
Sed is not necessary here. Instead use
export NGINX_DOCKER_GEN_CONTAINER=$(echo ${!NGINX_DOCKER_GEN_LINKED_CONTAINER_ENVVAR#/*})
I think the best is to use labels to references each others containers ? |
I think this is closed by #181. |
@teohhanhui #181 is only for the joined But #231 is another approach using a label and should solve this as well. |
Confirmed #231 works with Amazon ECS by using the
|
Motivation
I'm running this container alongside nginx-gen in Amazon ECS. The docker-gen container gets assigned a mangled name like "ecs-tws3-24-nginx-gen-aa9cf4babdd593ae5e00", which means I can't simply set
NGINX_DOCKER_GEN_CONTAINER=nginx-gen
.What I ended up doing is linking those containers and having Docker assign and expose a namespaced name as an environment variable like:
What this PR does
This PR adds the ability to specify the name of this environment variable instead of directly specifying docker-gen's container ID or name.