Add logrotate cronjob to all servers, don`t run logrotate unnecessarily on provisioning and use con…

Add logrotate cronjob to all servers, don`t run logrotate unnecessarily on provisioning and use con…

[Felix Dahlke, 2013-02-07 15:38:41]

Some comments, most of those probably not about your changes, but about the old
code, which I didn't review.

http://codereview.adblockplus.org/9351014/diff/1/modules/base/manifests/init.pp
File modules/base/manifests/init.pp (right):

http://codereview.adblockplus.org/9351014/diff/1/modules/base/manifests/init....
modules/base/manifests/init.pp:4: Stage['main'] -> Stage['post']
I'd like this to be consistent. So either this:

{'pre': before => Stage['main']}
{'post': require => Stage['main']}

Or this:

stage {'pre':}
stage {'post':}
Stage['pre'] -> Stage['main'] -> Stage['post']

http://codereview.adblockplus.org/9351014/diff/1/modules/logrotate/manifests/...
File modules/logrotate/manifests/init.pp (right):

http://codereview.adblockplus.org/9351014/diff/1/modules/logrotate/manifests/...
modules/logrotate/manifests/init.pp:4: path    => [ "/usr/bin/", "/bin/" ],
Mind getting rid of the whitespace between path and =>?

http://codereview.adblockplus.org/9351014/diff/1/modules/logrotate/manifests/...
modules/logrotate/manifests/init.pp:10: require => [
Should be fine without the enclosing square brackets:
require => Exec[...]

http://codereview.adblockplus.org/9351014/diff/1/modules/nginx/manifests/init.pp
File modules/nginx/manifests/init.pp (right):

http://codereview.adblockplus.org/9351014/diff/1/modules/nginx/manifests/init...
modules/nginx/manifests/init.pp:50: require => File['/etc/nginx/nginx.conf']
Why require the file here? Either the package, if we overwrite a default file
created by nginx, or nothing.

[Wladimir Palant, 2013-02-08 08:25:47]

All comments should be addressed now.

[Felix Dahlke, 2013-02-08 09:28:27]

Indeed, LGTM.