#2762 - Fix provisioning of geoip to filter servers

#2762 - Fix provisioning of geoip to filter servers

[mathias, 2017-08-29 18:09:00]

https://codereview.adblockplus.org/29527816/diff/29527817/modules/adblockplus...
File modules/adblockplus/manifests/legacy/filterserver.pp (right):

https://codereview.adblockplus.org/29527816/diff/29527817/modules/adblockplus...
modules/adblockplus/manifests/legacy/filterserver.pp:3: # A class for
maintaining legacy filter servers.
That comment is pretty much pointless. Please either omit the entire class
comment section here (like the "new" style Puppet of documentation, where them
authors start to just write everything into the README) or come up with
something more useful, thank you! <3

https://codereview.adblockplus.org/29527816/diff/29527817/modules/adblockplus...
modules/adblockplus/manifests/legacy/filterserver.pp:8: include filterserver
There's a blank line missing above this one.

https://codereview.adblockplus.org/29527816/diff/29527817/modules/adblockplus...
modules/adblockplus/manifests/legacy/filterserver.pp:10: # Remove any manually
installed geoip-database-contrib
That comment is pointless entirely, it explains the same stuff the code does.
How about "Formerly used package foobar conflicts with package baz" or just a
linkt to the ticket?

https://codereview.adblockplus.org/29527816/diff/29527817/modules/adblockplus...
modules/adblockplus/manifests/legacy/filterserver.pp:11:
package{'geoip-database-contrib':
There's a whitespace character missing after the `package` keyword.

[f.lopez, 2017-08-30 01:52:38]

https://codereview.adblockplus.org/29527816/diff/29527817/modules/adblockplus...
File modules/adblockplus/manifests/legacy/filterserver.pp (right):

https://codereview.adblockplus.org/29527816/diff/29527817/modules/adblockplus...
modules/adblockplus/manifests/legacy/filterserver.pp:3: # A class for
maintaining legacy filter servers.
On 2017/08/29 18:09:00, mathias wrote:
> That comment is pretty much pointless. Please either omit the entire class
> comment section here (like the "new" style Puppet of documentation, where them
> authors start to just write everything into the README) or come up with
> something more useful, thank you! <3

Done.

https://codereview.adblockplus.org/29527816/diff/29527817/modules/adblockplus...
modules/adblockplus/manifests/legacy/filterserver.pp:8: include filterserver
On 2017/08/29 18:09:00, mathias wrote:
> There's a blank line missing above this one.

Done.

https://codereview.adblockplus.org/29527816/diff/29527817/modules/adblockplus...
modules/adblockplus/manifests/legacy/filterserver.pp:10: # Remove any manually
installed geoip-database-contrib
On 2017/08/29 18:09:00, mathias wrote:
> That comment is pointless entirely, it explains the same stuff the code does.
> How about "Formerly used package foobar conflicts with package baz" or just a
> linkt to the ticket?

Done.

https://codereview.adblockplus.org/29527816/diff/29527817/modules/adblockplus...
modules/adblockplus/manifests/legacy/filterserver.pp:11:
package{'geoip-database-contrib':
On 2017/08/29 18:09:00, mathias wrote:
> There's a whitespace character missing after the `package` keyword.

Done.

[mathias, 2017-08-30 08:47:36]

https://codereview.adblockplus.org/29527816/diff/29530607/modules/adblockplus...
File modules/adblockplus/manifests/legacy/filterserver.pp (right):

https://codereview.adblockplus.org/29527816/diff/29530607/modules/adblockplus...
modules/adblockplus/manifests/legacy/filterserver.pp:9: # Manually installed
package conflicts with geoip module
Why would we use Puppet to ensure a manually installed package being purged?
That's nonsense (despite someone maybe "fixing" the thus far missing include for
class geoip in class nginx by over and over manually installing the package); We
use puppet to purge the FORMERLY installed package.

[f.lopez, 2017-08-30 15:19:47]

On 2017/08/30 08:47:36, mathias wrote:
>
https://codereview.adblockplus.org/29527816/diff/29530607/modules/adblockplus...
> File modules/adblockplus/manifests/legacy/filterserver.pp (right):
> 
>
https://codereview.adblockplus.org/29527816/diff/29530607/modules/adblockplus...
> modules/adblockplus/manifests/legacy/filterserver.pp:9: # Manually installed
> package conflicts with geoip module
> Why would we use Puppet to ensure a manually installed package being purged?
> That's nonsense (despite someone maybe "fixing" the thus far missing include
for
> class geoip in class nginx by over and over manually installing the package);
We
> use puppet to purge the FORMERLY installed package.

Well, there is already a link to the ticket, so I'll just remove this comment.

[mathias, 2017-08-30 17:42:30]

LGTM.