Issue 29435656: Noissue - Generate content on provision (Closed)
DescriptionNoissue - Generate content on provision
Patch Set 1 #
Total comments: 1
Patch Set 2 : For comment 2 #
Total comments: 1
Patch Set 3 : For comment 4 #
Total comments: 4
Patch Set 4 : For comments 5 trhough 9 #MessagesTotal messages: 11
I am a bit worried that a deployment may collide with a running cronjob, leading to undefined behavior. Please restrict the exec resource using `refreshonlyy => $::environment != "development"` or similar. https://codereview.adblockplus.org/29435656/diff/29435657/modules/web/manifes... File modules/web/manifests/server.pp (right): https://codereview.adblockplus.org/29435656/diff/29435657/modules/web/manifes... modules/web/manifests/server.pp:118: command => "python -m cms.bin.generate_static_pages /home/www/${repository} /var/www/${vhost}", Please export the bits shared with the cronjob command to variables, and use shellquote() to combine them into the respective resource's command parameter.
https://codereview.adblockplus.org/29435656/diff/29435659/modules/web/manifes... File modules/web/manifests/server.pp (right): https://codereview.adblockplus.org/29435656/diff/29435659/modules/web/manifes... modules/web/manifests/server.pp:157: command => "hg pull -q -R /home/www/${repository} && python -m cms.bin.generate_static_pages /home/www/${repository} /var/www/${vhost}", What about using $initialize_content_exec here as well?
https://codereview.adblockplus.org/29435656/diff/29437582/modules/web/manifes... File modules/web/manifests/server.pp (right): https://codereview.adblockplus.org/29435656/diff/29437582/modules/web/manifes... modules/web/manifests/server.pp:124: if $::environment == 'development' { If you'd used refreshonly => $::environment != "development" on the exec resource you would not only minimize complexity and have a resource definition available irregardless of whether it runs (which eases ordering when necessary), but also ensure a newly provisioned server in production would be able to serve content right after provisioning. https://codereview.adblockplus.org/29435656/diff/29437582/modules/web/manifes... modules/web/manifests/server.pp:157: command => "hg pull -q -R /home/www/${repository} && ${initialize_content_exec} /home/www/${repository} /var/www/${vhost}", Please export the commands preceeding and following $initialize_content_exec in the same fashion as the latter, and join([$preceding, $i_c_e, $following], " &&\n") afterwards, or similar (i.e. join(['set -e', $p, $i_c_e, $f], ';')).
https://codereview.adblockplus.org/29435656/diff/29437582/modules/web/manifes... File modules/web/manifests/server.pp (right): https://codereview.adblockplus.org/29435656/diff/29437582/modules/web/manifes... modules/web/manifests/server.pp:124: if $::environment == 'development' { On 2017/05/12 16:42:12, mathias wrote: > If you'd used refreshonly => $::environment != "development" on the exec > resource you would not only minimize complexity and have a resource definition > available irregardless of whether it runs (which eases ordering when necessary), > but also ensure a newly provisioned server in production would be able to serve > content right after provisioning. You are right. https://codereview.adblockplus.org/29435656/diff/29437582/modules/web/manifes... modules/web/manifests/server.pp:157: command => "hg pull -q -R /home/www/${repository} && ${initialize_content_exec} /home/www/${repository} /var/www/${vhost}", On 2017/05/12 16:42:12, mathias wrote: > Please export the commands preceeding and following $initialize_content_exec in > the same fashion as the latter, and join([$preceding, $i_c_e, $following], " > &&\n") afterwards, or similar (i.e. join(['set -e', $p, $i_c_e, $f], ';')). I actually thought of this but for a different commit, should I do it in this one then? cause I'd like to do it for all the exec resources in this manifest since I'm already here
On 2017/05/12 17:43:39, f.lopez wrote: > I actually thought of this but for a different commit, should I do it in this > one then? cause I'd like to do it for all the exec resources in this manifest > since I'm already here I thought one could do it with this one because you touch this line anyway. But the others should get a separate commit for that.
On 2017/05/12 18:06:36, mathias wrote: > On 2017/05/12 17:43:39, f.lopez wrote: > > I actually thought of this but for a different commit, should I do it in this > > one then? cause I'd like to do it for all the exec resources in this manifest > > since I'm already here > > I thought one could do it with this one because you touch this line anyway. But > the others should get a separate commit for that. Fair enough.
LGTM. 
|
