cpan-testers / cpantesters-deploy

Deploy scripts and configuration file for CPANTesters servers
4 stars 0 forks source link

Improving log of Apache2 #7

Closed glasswalk3r closed 7 years ago

glasswalk3r commented 7 years ago

ErrorLog was defined twice for the same virtual host Added custom fields to ErrorLogFormat Removed commented configuration

preaction commented 7 years ago

In order for me to review the changes, I need to know why the changes are necessary. Removing the first instance of the ErrorLog directive makes sense to me, because it was being ignored and could lead to confusion if someone checked that log and saw nothing. But why add the ErrorLogFormat? Why remove the commented-out RewriteLog lines?

Please also see this advice on how to write good commit messages. It's about the same thing: The commit message should explain why this change is being made. The diff tells me what's changing (so the commit message does not need to say that), but can't tell me why (so the commit message must say).

glasswalk3r commented 7 years ago

In order for me to review the changes, I need to know why the changes are necessary. Removing the first instance of the ErrorLog directive makes sense to me, because it was being ignored and could lead to confusion if someone checked that log and saw nothing. But why add the ErrorLogFormat? Why remove the commented-out RewriteLog lines?

My apologies. Here are the explanations:

preaction commented 7 years ago

This isn't source code, though. This is a configuration file. Often there are going to be bits left commented-out for easy re-enabling in crises. In a crisis, I'm not going to be here in this git repository looking for lost bits of configuration in the revision history, I'm going to be logged-on to the server and editing things by hand.

This one is fine: I never use the RewriteLog unless I know I need it, and at that point I'm probably deep in the documentation anyway. But there's no need to go around deleting configuration lines, even commented-out ones, unless everyone's certain they don't have any value.

preaction commented 7 years ago

Merged via command-line