search

linuxserver / docker-calibre

GNU General Public License v3.0
369 stars 64 forks source link

Dockerfile: Expose ports. #41

Closed saltydk closed 3 years ago

saltydk commented 3 years ago

linuxserver.io

Description:

Would allow service discovery without localhost port bindings.

Benefits of this PR and context:

Would allow service discovery without localhost port bindings.

How Has This Been Tested?

This is how I use all other containers.

Source / References:

LinuxServer-CI commented 3 years ago

I am a bot, here are the test results for this PR: https://ci-tests.linuxserver.io/lspipepr/calibre/v5.9.0-pkg-521149d6-pr-41/index.html https://ci-tests.linuxserver.io/lspipepr/calibre/v5.9.0-pkg-521149d6-pr-41/shellcheck-result.xml

tobbenb commented 3 years ago

I don't understand what you mean with service discovery? You will have to expose the port in either your docker run command or in your compose, which the EXPOSE in the dockerfile doesn't have any impact on.

So what exactly is it that doesn't work without the EXPOSE in the dockerfile?

saltydk commented 3 years ago

https://docs.docker.com/engine/reference/builder/#expose when using something like https://hub.docker.com/r/jwilder/nginx-proxy having the EXPOSE utilized means it can automatically identify the ports without having to bind the ports at all. I am not sure if it is a limitation in how this reverse proxy functions but it makes life easier to have the ports exposed and it offers no downside as it doesn't actually open up the ports.

tobbenb commented 3 years ago

We have swag, so we don't use other proxys 😉

I didn't know it uses the EXPOSE for this purpose. And it's correct that that adding it to the Dockerfile doesn't open any ports.

You still have to add an entry in the readme-vars.yml

saltydk commented 3 years ago

I'm unsure as to what you're expecting me to add to the readme-vars.yml file. Maybe it is due to it having been a long day. Would appreciate a hint :)

tobbenb commented 3 years ago

You need to add the changes in the changelog at the bottom. Newest entry at the top ofrhe changelog list.

saltydk commented 3 years ago

Like that? Or you want something more descriptive?

tobbenb commented 3 years ago

That is fine. Could you also remove the second EXPOSE and move the port after the other port?

EXPOSE 8080 8081

LinuxServer-CI commented 3 years ago

I am a bot, here are the test results for this PR: https://ci-tests.linuxserver.io/lspipepr/calibre/v5.9.0-pkg-521149d6-pr-41/index.html https://ci-tests.linuxserver.io/lspipepr/calibre/v5.9.0-pkg-521149d6-pr-41/shellcheck-result.xml

Roxedus commented 3 years ago

Just as an FYI, you can also define EXPOSE on runtime. And your choice of proxy can also take ports manually, reading the readme.

saltydk commented 3 years ago

I know but from my experience it doesn't work correctly without the expose when I don't do something like a localhost port binding.

LinuxServer-CI commented 3 years ago

I am a bot, here are the test results for this PR: https://ci-tests.linuxserver.io/lspipepr/calibre/v5.9.0-pkg-521149d6-pr-41/index.html https://ci-tests.linuxserver.io/lspipepr/calibre/v5.9.0-pkg-521149d6-pr-41/shellcheck-result.xml

saltydk commented 3 years ago

I can live with just adding the ports at runtime if the entry bothers you guys.