Skip to content

Conversation

crunchtime-ali
Copy link

Rancher users need to be able to disable dockergens --only-exposed parameter to be able to have their containers detected. We have been using this approach for a year now issuing certificates for 25+ webservices.

The code is originally from @mcnilz (mcnilz@72b1799)

@buchdag
Copy link
Member

buchdag commented Nov 20, 2017

@dj-hedgehog turns out we did not really need -only-exposed at all, so I believe #230 takes care of this ?

@crunchtime-ali
Copy link
Author

@buchdag I just deleted a certificate from one of my domains, started again with the latest docker-letsencrypt-nginx-proxy-companion and started nginx-proxy again but it did not renew the certificate.

I restarted again with my own version from this PR and it renewed the certificate in question.
So I still need it :)

Is there any other information I can provide?

@buchdag
Copy link
Member

buchdag commented Nov 20, 2017

Right, #230 removed the -only-exposed from /app/start.sh but not from the reload_nginx() function on /app/functions.sh.

I think you can modify the PR to just remove the -only-exposedfrom line 101 on /app/function.sh.

-only-exposed does not appear to be of any use anymore in the case of nginx-proxy + the le companion (as the two templates fed to docker-gen already filter containers through other means), so we probably won't need a feature to turn it back on.

@crunchtime-ali
Copy link
Author

I removed all usages of ONLY_EXPOSED I added and removed -only-exposed from /app/function.sh and /app/start.sh

Copy link
Member

@buchdag buchdag left a comment

Choose a reason for hiding this comment

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

You are still committing two extra blank lines to README.md (lines 170 & 171 on your branch), remove them and we are good to go.

@crunchtime-ali
Copy link
Author

Oops, I missed those. They are gone now.

Copy link
Member

@buchdag buchdag left a comment

Choose a reason for hiding this comment

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

LGTM

@buchdag buchdag merged commit d42c846 into nginx-proxy:master Nov 20, 2017
@JrCs JrCs added the lgtm label Nov 21, 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.

3 participants