Merge filter and notification servers, have both tasks run on the same servers

Merge filter and notification servers, have both tasks run on the same servers

[Felix Dahlke, 2014-01-30 13:51:48]

http://codereview.adblockplus.org/5302563972841472/diff/5629499534213120/mani...
File manifests/filterserver.pp (right):

http://codereview.adblockplus.org/5302563972841472/diff/5629499534213120/mani...
manifests/filterserver.pp:1: node 'server5', 'server6', 'server7', 'server11',
'server12', 'server15', 'server19', 'filter1', 'filter2', 'filter3', 'filter4',
'filter5', 'filter6', 'notification1', 'notification2' {
How about taking notification1 and 2 out of here and repurposing them as filter7
and filter8 later? (Can be done in a follow-up of course.)

http://codereview.adblockplus.org/5302563972841472/diff/5629499534213120/mani...
manifests/filterserver.pp:5: is_default => true
Not really a fan of this, I'd rather just merge notificationserver into
filterserver if we can't get the two modules to get along. I have some ideas for
achieving that below though.

http://codereview.adblockplus.org/5302563972841472/diff/5629499534213120/modu...
File modules/filterserver/manifests/init.pp (right):

http://codereview.adblockplus.org/5302563972841472/diff/5629499534213120/modu...
modules/filterserver/manifests/init.pp:3: class {'nginx':
Based on the Puppet documentation, I think this should work:

if !defined(Class['nginx']) {
  class {'nginx':
  …
}

Likewise for /var/www below. I'd prefer that to $is_default if it works.

http://codereview.adblockplus.org/5302563972841472/diff/5629499534213120/modu...
File modules/filterserver/templates/easylist-downloads.adblockplus.org.erb
(right):

http://codereview.adblockplus.org/5302563972841472/diff/5629499534213120/modu...
modules/filterserver/templates/easylist-downloads.adblockplus.org.erb:8: listen
80;
I think it should be fine to always go with this version, wouldn't it? The first
server that's getting defined would be the default server AFAIK.

[Felix Dahlke, 2014-01-30 13:58:02]

http://codereview.adblockplus.org/5302563972841472/diff/5629499534213120/modu...
File modules/filterserver/templates/easylist-downloads.adblockplus.org.erb
(right):

http://codereview.adblockplus.org/5302563972841472/diff/5629499534213120/modu...
modules/filterserver/templates/easylist-downloads.adblockplus.org.erb:8: listen
80;
On 2014/01/30 13:51:48, Felix H. Dahlke wrote:
> I think it should be fine to always go with this version, wouldn't it? The
first
> server that's getting defined would be the default server AFAIK.

I suppose it does make sense to be able to specify the default server. So I'm
fine with keeping this. However, I'd prefer the |if !defined()| version for
making sure nginx and /var/www aren't defined twice. I'd also prefer to not
explicitly set is_default to false for notificationserver in the node manifest.

[Wladimir Palant, 2014-01-30 14:11:36]

http://codereview.adblockplus.org/5302563972841472/diff/5629499534213120/modu...
File modules/filterserver/templates/easylist-downloads.adblockplus.org.erb
(right):

http://codereview.adblockplus.org/5302563972841472/diff/5629499534213120/modu...
modules/filterserver/templates/easylist-downloads.adblockplus.org.erb:8: listen
80;
On 2014/01/30 13:51:48, Felix H. Dahlke wrote:
> I think it should be fine to always go with this version, wouldn't it? The
first
> server that's getting defined would be the default server AFAIK.

No, normally there will be no default server - the requests will fall back to
nginx' default www directory. So I'd rather keep the is_default parameter, same
as in the web module.

[Felix Dahlke, 2014-01-30 14:31:29]

LGTM, feel free to address the final nit below.

http://codereview.adblockplus.org/5302563972841472/diff/5757334940811264/mani...
File manifests/filterserver.pp (right):

http://codereview.adblockplus.org/5302563972841472/diff/5757334940811264/mani...
manifests/filterserver.pp:9: is_default => false
I'd prefer not to explicitly set this to false here and use include instead.

[Wladimir Palant, 2014-01-30 14:50:23]

http://codereview.adblockplus.org/5302563972841472/diff/5757334940811264/mani...
File manifests/filterserver.pp (right):

http://codereview.adblockplus.org/5302563972841472/diff/5757334940811264/mani...
manifests/filterserver.pp:9: is_default => false
I'd rather keep invoking both modules the same way - and stating explicitly that
notificationserver could be the default as well.

[Felix Dahlke, 2014-01-30 14:53:22]

Fair enough, LGTM.

On 2014/01/30 14:50:23, Wladimir Palant wrote:
>
http://codereview.adblockplus.org/5302563972841472/diff/5757334940811264/mani...
> File manifests/filterserver.pp (right):
> 
>
http://codereview.adblockplus.org/5302563972841472/diff/5757334940811264/mani...
> manifests/filterserver.pp:9: is_default => false
> I'd rather keep invoking both modules the same way - and stating explicitly
that
> notificationserver could be the default as well.