-
-
Notifications
You must be signed in to change notification settings - Fork 153
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 documentation for encode and file_server enhancements #148
Conversation
* minimum_length * prefer * match
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks so much for contributing these too. Really appreciate it, as will a lot of users!
Maybe the Response Matcher section should be more general, when caddyserver/caddy#4021 also gets merged?
Yeah, we can consider that for later possibly; I also don't just mind linking one to the other, or vice-versa.
TL;DR: Any reason for Since the client can override preference by explicit weighted values and both client and server need to negotiate an encoding both support, why does Trying out the beta and looking at these docs (I don't think they're published officially yet?), could you please clarify if I understand this correctly? Simple config using the two features (plus basic cache, having a placeholder for matching common mimetypes or file extensions to cache as sane default as well might be nice addition too 😉 ):
The This seems like an odd choice with possible redundancy? Once 2.4 is released officially, that would make deprecating Is there something I don't realize that the documentation has not made clear why Below is the related documentation in this PR. Apologies if I'm not seeing something obvious, but I figured it might be worth pointing out before 2.4 is released in case this inconsistency for related config was accidental:
|
I looked at the present Caddy 2.3 encode directive docs and read over the original issue by @ueffel for this feature. From what I can make out, There are no config examples for adjusting the encoders, just a mentioned setting for gzip compression level which seems to differ in approach between Caddyfile and JSON somewhat? Is it too much trouble to have |
Problem is, that the JSON-Object is marshalled to a Go-Map and these have to no guarantee on the order of keys when iterating over the map. Keys generally won't be in insertion order. The
It's not really clear which encoding is preferred. Another point is backwards compatibility. An implicit prefer order from Of cause It would be possible to implicitly set the prefer order in cases of easy configs ( |
EDIT: Raised an issue summarizing this comment better. TL;DR: I don't see any backwards compatibility concern since default content to encode has also changed. If you can implicitly set Hopefully @mholt agrees. It's a minor UX improvement, and I know that JSON config is encouraged over the Caddyfile, but still seems worthwhile for consistency with the new Lengthy response (you can ignore it, I'll provide a more terse/focused response again if relevant)
I'm not a Go dev, so I assume that means that despite the docs stating the format for Personally it seems odd to require what for most may seem redundant, along with the inconsistency with
While I don't know the internals of Caddyfile to JSON logic, I always just saw that as implicitly enabling (as is the case with zstd in the example) an encoding while also making it possible to configure the object (zstd doesn't support any config presently afaik, unlike gzip?). Is it invalid to also specify the encoders like:
As that supposedly would declare a preference order just as well as
Keeping
Was there ever a guarantee on this? From your issue on the topic it seems like this was unexpected as a user to discover how selection was working (you could not negotiate for zstd from cURL if gzip was declared first). Prior behaviour wasn't documented was it? As only zstd and gzip are supported officially, and no web browser officially supports zstd, how many users would be affected? Present behaviour was basically always negotiating gzip if the client made it available? If the admin explicitly restricted it to zstd or gzip, then no compatibility issue exists? The There are release notes for anyone upgrading that needs to be aware of any potential concerns (this feature is already enabling a restricted set of default file types instead of the previous default of anything goes where I noticed compressed assets like webp were going through gzip), I don't see "backwards compatibility" as being relevant here due to these related changes.
I understand that. That would be a decision for @mholt ? One of the parts about Caddy I enjoy over nginx is that for those who want to use Caddyfile config is much simpler with smart defaults. I think the |
@polarathene I appreciate that you're interested, and your feedback is valuable. But I do have to say that these walls of text are a huge burden for us. I takes a lot of effort to read through and potentially respond to each of your points. From a cost-benefit perspective, it feels really difficult to engage here. I'm saying this now because it's not the first time that I think @mholt or I have felt overwhelmed by these types of comments (e.g. the auto-https discussion) so I'd just like to ask to try to be more concise in the future. Anyways, I don't have a strong opinion either way, but I do agree we should make a decision on this before v2.4.0 stable, but we also can't afford to make this block that release (probably next week Monday ish). |
@francislavoie I understand, I apologize about the verbosity and will try to keep it terse in future. The TLDR at the top was an effort to address that. There's no need to block the release.
|
Adding documention for caddyserver/caddy#4045
Maybe the Response Matcher section should be more general, when caddyserver/caddy#4021 also gets merged?