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

Update docs for Caddy image with a fix to its reload feature #2483

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

Conversation

BjoernLudwigPTB
Copy link

@BjoernLudwigPTB BjoernLudwigPTB commented Aug 24, 2024

Last August unexpected behaviour of the graceful reload feature was discussed in this upstream issue and a fix proposed alongside the suggestion to add that fix to the docs. This has not yet happened but should be solved by this PR.

This is my first contribution here, so if there is anything I should address to finalize this PR, I ask you kindly to let me know.

Copy link
Contributor

@whalelines whalelines left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me, as long as there remains only a single file under the image's /etc/caddy directory.

Copy link
Member

@tianon tianon left a comment

Choose a reason for hiding this comment

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

Agreed, seems OK to me -- cc @hairyhenderson @francislavoie (caddy image maintainers)

@tianon
Copy link
Member

tianon commented Aug 26, 2024

(Looks like there's also a bit of trailing whitespace that our markdownfmt is balking at)

caddy/content.md Outdated Show resolved Hide resolved
caddy/content.md Outdated Show resolved Hide resolved
caddy/content.md Outdated
-v caddy_data:/data \
%%IMAGE%%
```

#### ⚠️ Do not mount the Caddyfile directly at `/etc/caddy/Caddyfile`

This effectively disables Caddy's graceful reload feature in many cases depending on the way you apply changes to the file as discussed in [this issue](https://github.com/caddyserver/caddy/issues/5735#issuecomment-1675896585).
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't agree with this wording/framing. This isn't quite true, it only breaks if you happen to be using vim/vi in which case the way it saves files isn't tracked properly by Docker. But if you use a different editor like nano then the issue doesn't happen.

So I would prefer if this warned about making note of which editor you use to edit the file, rather than saying that it "breaks reloading" in general.

Copy link
Author

Choose a reason for hiding this comment

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

Agree, I will come up with a better wording reflecting the actual scope of the circumstances in which there is something to warn about. I submitted this proposal from the perspective of someone having followed the documentation exactly and due to having used vim to edit the file, I ran into the described situation.

Copy link
Author

Choose a reason for hiding this comment

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

@francislavoie I have now suggested a wording that better reflects the situation that is causing the actual problem. This makes the paragraph a little lengthier, but also more precise. What do you think? Would you agree to these changes?

caddy/content.md Outdated Show resolved Hide resolved
@@ -147,3 +149,5 @@ volumes:
```

Defining the data volume as [`external`](https://docs.docker.com/compose/compose-file/compose-file-v3/#external) makes sure `docker-compose down` does not delete the volume. You may need to create it manually using `docker volume create [project-name]_caddy_data`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Defining the data volume as [`external`](https://docs.docker.com/compose/compose-file/compose-file-v3/#external) makes sure `docker-compose down` does not delete the volume. You may need to create it manually using `docker volume create [project-name]_caddy_data`.
Defining the data volume as [`external`](https://docs.docker.com/compose/compose-file/compose-file-v3/#external) makes sure `docker compose down` does not delete the volume. You may need to create it manually using `docker volume create [project-name]_caddy_data`.

Copy link
Author

Choose a reason for hiding this comment

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

In fact, now that we are talking about this section as well: May I suggest to delete this paragraph and the parameter external = true in the suggested compose.yaml as a docker compose down wouldn't actually delete the volume. One would need to provide an addional -v or --volumes in addition to loose its content. In many cases providing this flag additionally would cause damage to applications, which usually do not mention this in their sample configurations or examples as Docker users usually seem to be aware of the consequences. If you agree @francislavoie I would simply delete these two parts in another commit.

Copy link
Author

@BjoernLudwigPTB BjoernLudwigPTB Oct 19, 2024

Choose a reason for hiding this comment

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

@francislavoie And please review my comment above and give your opinion on that.

  • suggestion 1: remove this paragraph and the external = true parameter for the volume in the compose.yaml.
  • option 2: Apply your suggested changes and leave everything else as is.

caddy/content.md Outdated Show resolved Hide resolved
caddy/content.md Outdated Show resolved Hide resolved
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.

4 participants