-
Notifications
You must be signed in to change notification settings - Fork 401
docs: Improve documentation for builtin VCL and pipe behavior #4430
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
docs: Improve documentation for builtin VCL and pipe behavior #4430
Conversation
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.
Pull request overview
This PR enhances documentation for Varnish's built-in VCL by clarifying extensible subroutines and explaining the change in default pipe behavior for unknown HTTP methods.
- Adds inline comments to
builtin.vclexplaining thatvcl_builtin_*subroutines can be overridden - Documents the concept of "extensible subs" in the user guide
- Provides examples for re-enabling pipe mode for custom HTTP methods and WebSocket connections
Reviewed changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
doc/sphinx/users-guide/vcl-built-in-code.rst |
Adds explanation of extensible subs concept, mentions Git repository availability, and provides examples for re-enabling pipe mode in Varnish 8.0+ |
bin/varnishd/builtin.vcl |
Adds consistent explanatory comments to all vcl_builtin_* subroutines indicating they can be overridden to change default behavior |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
nigoroll
left a comment
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.
I have mixed feedback about this suggestion.
the pipe mode explanation makes sense, and I am 👍🏽 on it specifically
the explanation of the vcl_builtin_* subs looks like an improvement at first, but I am not sure how helpful it really is: The reference to the naming scheme is incomplete, and the main motivation is subtly different to the added explanation: All subs of the builtin.vcl can be overridden, that is the whole point of appending the built-in VCL to the user VCL. IIUC, the main point about the vcl_builtin_* subs is to make them available to calls from custom VCL, while subs like vcl_req_* allow specific extensions of parts of the built-in behavior only.
Repeating the same comment all over the place in the built-in VCL does not seem to make any sense to me at all because, again, there is nothing special about the possibility to override some subs, they can all be overridden.
Maybe @gquintard or @dridi should document their original design principles? My understanding might still not match what they had in mind (Ref: 3f3d67d)
Similar thing with the comment on git: Sure, all we do is available in git, but that is no news.
Regarding the overall structure of the PR, I would suggest to split out at least the pipe example into a separate commit.
|
Thanks. About the repetitive comments in I will rework the PR to:
Pushing an updated version shortly. |
06a3ff4 to
0e8053c
Compare
|
TIL rewriting Git history with |
0e8053c to
142d735
Compare
nigoroll
left a comment
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.
Just a minor comment, otherwise I agree now.
142d735 to
eb15ca9
Compare
eb15ca9 to
b5e00bd
Compare
This addresses issue #4372 by improving documentation for the built-in VCL, based on feedback from @nigoroll and @walid-git.
The changes are split into two commits:
docs: Document vcl_pipe default behavior change/843ef0f: Updates the user guide to explain that Varnish no longer pipes unknown methods by default. It adds examples for re-enablingpipefor specific methods or requests and clarifies how to view the builtin VCL withvarnishd -x builtin.docs: Explain extensible builtin subs concept in builtin.vcl/142d735: Adds a comprehensive comment block to the top ofbuiltin.vclexplaining the two patterns for extending default behavior (vcl_builtin_*subs vs.vcl_req_*hooks).