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

Add custom entrypoint to generate the configuration #86

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

dabelenda
Copy link

Having to rebuild an image only to include the configuration is not ideal. This PR adds an rudimentary way to create the Caddyfile.

Copy link

@gamerlv gamerlv left a comment

Choose a reason for hiding this comment

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

Officially this image seems to promote bind mounting the Caddyfile config. That's how I've use it. But I can see where generating an config would be much more maintainable.

It appears this PR makes some breaking changes to how I and the readme suggest to use this container.

To maintain backwards compatibility maybe it's possible to append the config file /etc/Caddyfile to /etc/caddy/Caddyfile.

@@ -0,0 +1,15 @@
#!/bin/bash

rm -f /etc/caddy/Caddyfile
Copy link

Choose a reason for hiding this comment

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

How does this affect people using one of the recommended run commands (https://github.com/abiosoft/caddy-docker#using-local-caddyfile-and-sites-root)?
Looking this code over I would reason their config is just ignored. And an empty one generated instead.

Disabling an docker entrypoint command is somewhat awkward to do with the docker command. Perhaps an check could be added to see if the any of the env vars are present before rewriting the config.

Copy link
Author

Choose a reason for hiding this comment

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

I have added a backward compatibility mode, enabled by default.

@@ -26,12 +26,12 @@ RUN /usr/bin/caddy -version
RUN /usr/bin/caddy -plugins

EXPOSE 80 443 2015
VOLUME /root/.caddy /srv
VOLUME /root/.caddy /srv /etc/caddy
Copy link

Choose a reason for hiding this comment

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

If the config is being auto generated, having it also stored in an docker volume seems somewhat counter-intuitive. Does this provide an advantage for larger installations?

Copy link
Author

Choose a reason for hiding this comment

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

For me, a volume means read-write data, and the image itself should be read-only.

README.md Outdated

```sh
CADDY_ENTRY0_HOSTNAME=foo.bar.baz
CADDY_ENTRY0_MODE=self_signed
Copy link

Choose a reason for hiding this comment

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

Should this not read CADDY_ENTRY0_TLS_MODE?

CMD ["--conf", "/etc/Caddyfile", "--log", "stdout"]

ENTRYPOINT ["/bin/bash", "/docker-entrypoint.sh"]
CMD ["/usr/bin/caddy", "--conf", "/etc/caddy/Caddyfile", "--log", "stdout"]
Copy link

Choose a reason for hiding this comment

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

The readme.md has a few mentions where the main config is located in the container. But this change moves it.
The documentation should be updated to remain working with this change.

How would someone use the default config with this pull request?

@abiosoft
Copy link
Owner

Firstly, thanks for this and sorry this is coming a bit late.
I would like to mention that there is strong preference for non-breaking change and simplicity.
However, I will try to have a look and see if this provides a better experience for users.

Thanks.

@dabelenda
Copy link
Author

I made a change to the entrypoint, to make the PR a non-breaking change.

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