Create directory for log files

[geoffgarside]

I've added loose management and creation of the nginx access log and error log files. If they don't exist then nginx will not pass its config test and won't start up (on FreeBSD at least), so these files need to exist prior to nginx being started or reloaded.

I've included specs which are passing, and I think testing the right things I'm not very familiar with testing puppet yet. Testing my branch on a virtual machine results in a successfully started nginx service too.

I also think I've managed to stick to the style guidelines, I've run the rake lint, rake syntax and rake validate tasks and I think they've all passed as well.

I'm not sure if this is the right thing to do or not though

if $vhost_purge == true {
   File[$log_dir] {
     purge   => true,
     recurse => true,
   }
}

Fixes #619

[geoffgarside]

I think this might currently break if access_log is set to syslog in the vhost resource. Noticed the syslog test in the Travis CI test output.

[3flex]

@geoffgarside I want to thank you wholeheartedly for including tests with your PR submission!

That said, it looks like you have the same issue as reported in #619. I think the answer is to have the module create the file in the correct location in FreeBSD/DragonflyBSD, which would already exist if set correctly. I'd prefer that simpler solution rather than adding more to the module if it's not necessary.

What do you think? Do you have another use case where you want a configurable log path, or are you just trying to get FreeBSD working?

[geoffgarside]

@3flex Thank you for having tests I could work out, I'd not tried rspec-puppet before. The contributor guidelines suggested they'd help.

Yes changing the default paths for FreeBSD and DragonflyBSD to /var/log rather than /var/log/nginx means the initial configuration will start properly. For some reason I'd got it into my head that the files had to exist prior to nginx starting, that might have been an earlier version or else I'm imagining things. So the #619 issue is purely down to /var/log/nginx not existing.

Theres also the problem of the nginx::vhost resource and its default log files. By default these also end up in /var/log/nginx which is good and makes sense. Either we'd need /var/log/nginx to exist or else also change these to be /var/log/nginx-vhost_name.access.log.

I would generally expect users to have at least one vhost defined, so keeping all of the nginx log files together in /var/log/nginx seems like a better idea to me. Given that the dirname of the log file path just needs to exist, not the entire path as I originally thought, it might be simpler to manage the /var/log/nginx directory on Linux and BSD platforms and keep the default log paths inside this directory.

Part of the reason for this PR was using the puppetlabs-operations/puppet-puppet module and the puppet::server class. This class doesn't provide a way to set the nginx puppetmaster vhost log paths so the default selected ones resulted in a configuration which nginx complained about.

[geoffgarside]

Would it make sense then for me to remove the file resources and tests from this PR leaving just the /var/log/nginx directory management, possibly testing and not managing if the log dir is just /var/log?

[3flex]

Most distro configs seem to create an access & error log, but don't create individual logs for any vhosts.

But since this module does by default, I think you're right. They should all go in the same directory, and to keep it simple that should be /var/log/nginx. I think what you said is best, just manage the /var/log/nginx directory in nginx::config class and the rest should work as expected. Please also remove the purging when vhost_purge == true.

Lastly if you could also squash to a single commit that would be greatly appreciated!

[geoffgarside]

Ok, that sounds good to me, I'll get that updated and squash it down.

[geoffgarside]

Ok, I've updated the changes now. As the main /var/log/nginx directory management was covered in the first commit I was able to just roll back the others.

One thing thats occurred to me while considering the host_purge option. If we don't define resources for the main access.log and error.log then they will be removed from the file system when vhost_purge is enabled. Would this be the correct expected behaviour or should the main logs be managed, provided they're within $log_dir so they would be kept when vhost_purge is enabled?

[3flex]

No, we definitely don't want the main access/error logs to be purged if vhosts are purged.

I'm also in two minds about purging logs at all. I don't think they should be; if you remove a vhost you might still want the logs around, better to err on the side of caution.

[geoffgarside]

I think if we had host_purge enabled then if we aren't managing the vhost log files it would clean out the logs on every puppet run wouldn't it. So as you say, probably best not to purge logs at all.

[geoffgarside]

Would there be any merit to adding a log_purge parameter?

[3flex]

Feel free to add it if you wish. I personally don't think it's required, but I'm happy to merge whether it's there or not.

[geoffgarside]

Removed the log directory purge