Log example in documentation has incorrect rotate_compress line

[gregmac]

The documentation at https://caddyserver.com/docs/log shows the example:

log requests.log {
	rotate_size 50  # Rotate after 50 MB
	rotate_age  90  # Keep rotated files for 90 days
	rotate_keep 20  # Keep at most 20 log files
	rotate_compress # Compress rotated log files in gzip format
}

This example should actually have the last line as rotate_compress gzip

[tobya]

@gregmac Thanks for the issue. If you wanted we would be delighted with a PR to /~https://github.com/caddyserver/website on the log file
/~https://github.com/caddyserver/website/blob/master/site/docs/log.html

[mholt]

Hi @gregmac - are you sure? It is a boolean value, not a string. Simply its presence will enable compression.

[gregmac]

@mholt With Caddy 0.10.4, when I put that line in as-is, I get the error:

Parse error: Wrong argument count or unexpected line ending after 'rotate_compress'

and caddy exits with code 1.

[gregmac]

@mholt Ah, I think I see what's happening. It's treating that directive the same as all other logrotate directives -- assuming they have an integer-type value -- but explicitly ignores the integer conversion error: /~https://github.com/mholt/caddy/blob/bf7b25482ede58728589b5b0868b294db48cbcdf/caddyhttp/httpserver/roller.go#L65 (added in #1731).

So I'm pretty sure basically any value will cause it to work, as long as there is a value. That's actually kind of misleading, because, for example, the following all enable compression:

rotate_compress 1
rotate_compress 0
rotate_compress false
rotate_compress off
rotate_compress none

---

I just ran into this -- twice, sadly -- because I copied+pasted that example into my config. As I write this I'm only three days into using Caddy so right now relying heavily on documentation and examples.

[mholt]

Ah, this is a bug then! Must have been missed by the tests. I wonder if @fern4lvarez would have a chance to help us fix it.

[fern4lvarez]

Not sure if there was a follow-up of #1731, but the implementation states that if you want to compress logs, you just specify

rotate_compress

No type, no additional values (see #1731 (comment) too). This is how the tests were written, and passed. So I don't know why we are saying this part of the documentation is a bug, or caddyserver/website#28 merged.

Now, if we want to force the type of the compress, or make it optional, then the implementation must be changed.

What I'm saying: Correct or not, implementation and documentation are consistent.

Let me know if I'm missing something.

[tobya]

@fern4lvarez I think as per @gregmac comment and my own testing on 0.10.4 if you do this

log / file.log {common} {
  	rotate_size     1
rotate_age     1
rotate_keep     1
rotate_compress
}

you get an error Parse error: Wrong argument count or unexpected line ending after 'rotate_compress'

If you include gzip (or anything it seems) after rotate_compress they you do not get an error. This would suggest either the documentation or the code needs to be updated.

[fern4lvarez]

@tobya that's weird, because that's almost exactly of what is being tested here /~https://github.com/mholt/caddy/blob/master/caddyhttp/errors/setup_test.go#L88

If no one has a quick idea for a fix, I can book some time and find a solution for this.

[mholt]

@fern4lvarez I think the problem is -- and it's my fault for not catching this -- the test has all the subdirectives on the same line, which should really be a syntax error. Each subdirective must be on its own line. Would you have a chance to update the tests and make the fix in the parser? I'm swamped with proxy rewrite for now, and we'd really appreciate it. 😄

[fern4lvarez]

thanks @tw4452852! I was totally lacking of time for this, gotta love OSS ❤️