Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29511584: #2762 - Fix provisioning of geoip to filter servers (Closed)

Created:
Aug. 10, 2017, 9:57 a.m. by f.nicolaisen
Modified:
Sept. 4, 2017, 10:42 a.m.
Reviewers:
f.lopez, mathias
Base URL:
https://hg1/infrastructure
Visibility:
Public.

Description

#2762 - Fix provisioning of geoip to filter servers Replaced by https://codereview.adblockplus.org/29527816/

Patch Set 1 #

Patch Set 2 : split the absent package into a legacy class #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+17 lines, -0 lines) Patch
M hiera/roles/filterserver.yaml View 1 1 chunk +2 lines, -0 lines 2 comments Download
A modules/adblockplus/manifests/legacy/filterserver.pp View 1 1 chunk +15 lines, -0 lines 1 comment Download

Messages

Total messages: 9
f.nicolaisen
Aug. 10, 2017, 9:57 a.m. (2017-08-10 09:57:30 UTC) #1
mathias
LGTM. Not ideal to place the package/purged resource in the same context where one can ...
Aug. 10, 2017, 5:30 p.m. (2017-08-10 17:30:40 UTC) #2
f.lopez
On 2017/08/10 17:30:40, mathias wrote: > LGTM. Not ideal to place the package/purged resource in ...
Aug. 10, 2017, 6:04 p.m. (2017-08-10 18:04:02 UTC) #3
mathias
On 2017/08/10 18:04:02, f.lopez wrote: > Maybe in the legacy class? So include some adblockplus::legacy::geoip ...
Aug. 10, 2017, 6:19 p.m. (2017-08-10 18:19:21 UTC) #4
f.lopez
On 2017/08/10 18:19:21, mathias wrote: > On 2017/08/10 18:04:02, f.lopez wrote: > > Maybe in ...
Aug. 10, 2017, 6:21 p.m. (2017-08-10 18:21:43 UTC) #5
f.lopez
On 2017/08/10 18:21:43, f.lopez wrote: > On 2017/08/10 18:19:21, mathias wrote: > > On 2017/08/10 ...
Aug. 10, 2017, 6:22 p.m. (2017-08-10 18:22:45 UTC) #6
mathias
On 2017/08/10 18:22:45, f.lopez wrote: > > Or you include geoip class normally in hiera ...
Aug. 10, 2017, 6:24 p.m. (2017-08-10 18:24:40 UTC) #7
f.nicolaisen
On 2017/08/10 18:24:40, mathias wrote: > On 2017/08/10 18:22:45, f.lopez wrote: > > > Or ...
Aug. 11, 2017, 1:40 p.m. (2017-08-11 13:40:45 UTC) #8
mathias
Aug. 14, 2017, 8:04 a.m. (2017-08-14 08:04:33 UTC) #9
On 2017/08/11 13:40:45, f.nicolaisen wrote:
> OK, I tried this in PS2, but noticed that if I configure only the legacy
class,
> it'll break because of the required resource in the other class. Is that
> something we can live with?

No.

https://codereview.adblockplus.org/29511584/diff/29512671/hiera/roles/filters...
File hiera/roles/filterserver.yaml (right):

https://codereview.adblockplus.org/29511584/diff/29512671/hiera/roles/filters...
hiera/roles/filterserver.yaml:6: geoip:
Since class filterserver depends on class geoip it should include the latter.

https://codereview.adblockplus.org/29511584/diff/29512671/hiera/roles/filters...
hiera/roles/filterserver.yaml:7: adblockplus::legacy::filterserver:
This class should in turn include class filterserver, it's basically the adapter
for the legacy module. You can use a root-level `filterserver::is_default: true`
to replace the parametrization above.

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

https://codereview.adblockplus.org/29511584/diff/29512671/modules/adblockplus...
modules/adblockplus/manifests/legacy/filterserver.pp:12: require =>
Cron['geoip'],
Why should ensuring absence require anything?

Powered by Google App Engine
This is Rietveld