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

Support referencing nginx-gen container indirectly through another environment variable #126

Closed

Conversation

ento
Copy link

@ento ento commented Nov 28, 2016

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:

NGINX_GEN_NAME=/ecs-tws3-23-nginx-letsencrypt-a6d9b293fc808bf7fd01/nginx-gen

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.

@JrCs
Copy link
Collaborator

JrCs commented Dec 30, 2016

@ento can you fix the code with the reviews i have made ?

Copy link
Collaborator

@JrCs JrCs left a 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
Copy link
Collaborator

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/^\///')
Copy link
Collaborator

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#/*})

@JrCs
Copy link
Collaborator

JrCs commented Feb 27, 2017

I think the best is to use labels to references each others containers ?

@teohhanhui
Copy link

I think this is closed by #181.

@helderco
Copy link

helderco commented Jul 8, 2017

@teohhanhui #181 is only for the joined jwilder/nginx-proxy container, not separate jwilder/docker-gen and nginx.

But #231 is another approach using a label and should solve this as well.

@ento
Copy link
Author

ento commented Sep 10, 2017

Confirmed #231 works with Amazon ECS by using the dockerLabels property:

{
  {
    "image": "nginx",
    "dockerLabels": {
      "com.github.jrcs.letsencrypt_nginx_proxy_companion.nginx_proxy": ""
    },
    ...
  },
  {
    "image": "ento/docker-nginx-gen-with-tpl",
    "dockerLabels": {
      "com.github.jrcs.letsencrypt_nginx_proxy_companion.docker_gen": ""
    },
    ...
  },
  ...
}

@ento ento closed this Sep 10, 2017
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