-
Notifications
You must be signed in to change notification settings - Fork 823
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
Added label-based mechanism to identify docker-gen container. This is needed w… #205
Conversation
…hen using compose or other mechanims that define dynamically container's name.
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.
You're making changes to app/nginx_location.conf
, which is out of scope here.
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.
There's the assumption here that you always have separate containers, but you could have jwilder/nginx-proxy
instead. So this needs to be optional.
the change to |
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.
There's the assumption here that you always have separate containers, but you could have jwilder/nginx-proxy instead. So this needs to be optional.
@@ -79,6 +92,7 @@ source /app/functions.sh | |||
|
|||
if [[ "$*" == "/bin/bash /app/start.sh" ]]; then | |||
check_docker_socket | |||
get_nginx_gen_cid |
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.
I think this should be:
[[ -z "${NGINX_DOCKER_GEN_CONTAINER:-}" ]] && get_nginx_gen_cid
to allow still using a manual environment variable when you do have access to a static name.
if [[ ! -z "${labeled_cid:-}" ]]; then | ||
export NGINX_DOCKER_GEN_CONTAINER=$labeled_cid | ||
fi | ||
if [[ -z "${NGINX_DOCKER_GEN_CONTAINER:-}" ]]; 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.
Remove this condition to make it optional, don't error out. Document it instead.
@@ -1,4 +1,5 @@ | |||
location ^~ /.well-known/acme-challenge/ { | |||
location ~ /.well-known { |
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.
Out of scope, changes should be discarded.
I need this, if you hadn't, I would! 👍 |
I found an issue with containers getting redeployed during letsencrypt lifecycle, so I made my own attempt with #231. It's working in production right now, with the helder/letsencrypt-nginx-proxy-companion fork (temporary until the PR is merged). |
#231 was merged so this can be closed. |
Closed by #231 |
Added label-based mechanism to identify docker-gen container. This is needed when using docker compose or other container mechanisms that define dynamically container's name.