Skip to content

Conversation

mjmayer
Copy link
Contributor

@mjmayer mjmayer commented Sep 14, 2017

Documentation was unclear regarding statically named contains and where the environment variables went.

README.md Outdated

Note: If the docker-gen container name is static and you want to explicitly set it, use `-e NGINX_DOCKER_GEN_CONTAINER=nginx-gen`. The same thing is true with the nginx container (`-e NGINX_PROXY_CONTAINER=nginx`).
Note:
If the 3 containers are using using static names, as shown in the examples. The label (`--label com.github.jrcs.letsencrypt_nginx_proxy_companion.docker_gen`) on `nginx` container can be removed and additional evironment variables set on the `letsencrypt-nginx-proxy-companion` container. The variables are `-e NGINX_DOCKER_GEN_CONTAINER=nginx-gen` and `-e NGINX_PROXY_CONTAINER=nginx`.
Copy link
Member

Choose a reason for hiding this comment

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

The variables themselves are just NGINX_DOCKER_GEN_CONTAINER and NGINX_PROXY_CONTAINER, not the whole thing with the -e switch and the variable value.


* Then start any containers to be proxied as described previously.

Note: If the docker-gen container name is static and you want to explicitly set it, use `-e NGINX_DOCKER_GEN_CONTAINER=nginx-gen`. The same thing is true with the nginx container (`-e NGINX_PROXY_CONTAINER=nginx`).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed the -e switch from the variable names.

Copy link
Member

Choose a reason for hiding this comment

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

see below

README.md Outdated

Note: If the docker-gen container name is static and you want to explicitly set it, use `-e NGINX_DOCKER_GEN_CONTAINER=nginx-gen`. The same thing is true with the nginx container (`-e NGINX_PROXY_CONTAINER=nginx`).
Note:
If the 3 containers are using using static names, as shown in the examples. The label (`--label com.github.jrcs.letsencrypt_nginx_proxy_companion.docker_gen`) on `nginx` container can be removed and additional evironment variables set on the `letsencrypt-nginx-proxy-companion` container. The variables are `NGINX_DOCKER_GEN_CONTAINER=nginx-gen` and `NGINX_PROXY_CONTAINER=nginx`.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the -e from the variables.

Copy link
Member

Choose a reason for hiding this comment

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

The variables are NGINX_PROXY_CONTAINER and NGINX_DOCKER_GEN_CONTAINER, nginx and nginx-gen are their respective value in the context of this example only, that should be made clear like in the rest of the doc.

In my opinion the way you wrote it might cause people to believe that the two variables have to be set to nginx and nginx-gen regardless of the name given to the nginx and docker-gen containers. It should be made clearer that those two values have to be the static names chosen for the nginx and doker-gen containers.

@JrCs
Copy link
Collaborator

JrCs commented Nov 19, 2017

@buchdag is it ok to merge this PR ?

@buchdag
Copy link
Member

buchdag commented Nov 19, 2017

@JrCs I think the wording is still unclear as it is mixing labels themselves with the --label argument to docker run and variable names with their value in the context of an example while you carefully avoided doing that in the existing doc.

In the rest of the documentation labels are surround in double quotes. This commit makes it consistent in the static container name example.
Containers are now referred to as nginx contain and letsencrypt container. There was concern about confusion when using the name of the containers specific in the examples, rather than referring to them by generic names.
@mjmayer
Copy link
Contributor Author

mjmayer commented Nov 19, 2017

@buchdag I've made the wording more consistent with the rest of the readme. The labels are now surrounded by double quotes instead of being prefaced by --label. The names of the containers in the text are now generic and do not reference the container by its name defined in the docker run command.

@buchdag
Copy link
Member

buchdag commented Nov 19, 2017

@mjmayer I might not have been clear enough myself, your use of markdown was perfectly right, sorry for the unnecessary work.

What I meant is that you have be more explicit on the fact that nginxand nginx-gen both depends on the name you chose earlier for you container. If you use different names for your containers, you'll have to set the env var to different values.

Let me show you what I mean:

If [...] static names, both labels com.github.[...].nginx_proxy on the nginx container and com.github.[...].docker_gen on the docker-gen container can be removed.

[...] on the letsencrypt container are NGINX_PROXY_CONTAINER set to the name of the nginx container (here nginx) and NGINX_DOCKER_GEN_CONTAINER set to the name of the docker-gen container (here nginx-gen).

And then your example. The [...] are meant to be replaced by the existing text, obviously.

BTW there are two small typos ("using using" and "evironment").

@mjmayer
Copy link
Contributor Author

mjmayer commented Nov 19, 2017

@buchdag Thanks for clarifying. Hopefully the most recent commit is better.

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 58cf2b3 into nginx-proxy:master Nov 20, 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