fileserver: Add file_limit option for browse#6648
Conversation
|
|
atakanyenel
force-pushed
the
master
branch
2 times, most recently
from
d95c79e to
1327fc6
Compare
francislavoie
changed the title
file_limit option for browse
atakanyenel
force-pushed
the
master
branch
2 times, most recently
from
48c5441 to
5a5452d
Compare
atakanyenel
force-pushed
the
master
branch
3 times, most recently
from
6d3dd2d to
05261ec
Compare
francislavoie
left a comment
francislavoie
left a comment
There was a problem hiding this comment.
Thanks!
For bonus points, it could probably use an adapt test (can just add it to an existing file_server test) to make sure it produces the correct JSON.
|
Thanks! This LGTM, but before I merge I have just one stupid nit: should it be called Similarly, |
|
I added the test and updated the constant name. I'm keeping the config name still as "file_limit". I can also change that if required. |
|
@mholt I don't want to pressure at all, but the CL still is approved but only needs your review to merge. It's the youngest review in the already approved CLs, so could you take a look, when you have time ? Then I can work on other issues too :) |
| // The first option must be `sort_by` and the second option must be `order` (if exists). | ||
| SortOptions []string `json:"sort,omitempty"` | ||
|
|
||
| // FileLimit limits the number of up to n DirEntry values in directory order. |
There was a problem hiding this comment.
Should we document that -1 means no limit? (And 0 is default limit, which is currently 10,000)
mholt
left a comment
mholt
left a comment
There was a problem hiding this comment.
I'm going to merge this, but I still think we should enhance the comment/docs before the final release; and maybe even mark this option as experimental since we don't quite understand its characteristics in the wild yet. Would you be able to help with that? I just don't have a chance right now. Thanks!
Yes, I can do this. I will send another PR soon. Thanks for the merge. |
…addyserver#6648) * Add file_limit option for file_server browse * Move file_limit inside browse. * add file_server_file_limit caddyfile adapt test.
4 participants
This PR fixes the issue #6644 . I'm not sure about the config name
file_limit, I prefermax_dir_limitetc.The default is still 10000.As mentioned in the issue, this PR gave me some background on file serve, so I will try to look at paging as well. Please feel free to edit any documentation as you see fit.