#3044 - Introduce class adblockplus::mercurial

#3044 - Introduce class adblockplus::mercurial

[mathias, 2017-08-24 16:40:53]

https://codereview.adblockplus.org/29526640/diff/29526641/hiera/roles/web/adb...
File hiera/roles/web/adblockplus.yaml (right):

https://codereview.adblockplus.org/29526640/diff/29526641/hiera/roles/web/adb...
hiera/roles/web/adblockplus.yaml:5: web::server::remote:
git://github.com/adblockplus/web.adblockplus.org.git
I assume the changes in this file are for demonstration and test purpose but do
not end up in the commit that gets pushed. Right?

https://codereview.adblockplus.org/29526640/diff/29526641/modules/adblockplus...
File modules/adblockplus/manifests/mercurial.pp (right):

https://codereview.adblockplus.org/29526640/diff/29526641/modules/adblockplus...
modules/adblockplus/manifests/mercurial.pp:5: # === Parameters:
Please document parameters $config and $package, thank you!

https://codereview.adblockplus.org/29526640/diff/29526641/modules/adblockplus...
modules/adblockplus/manifests/mercurial.pp:10: #     ensure => 'latest',
That is not a valid example.

https://codereview.adblockplus.org/29526640/diff/29526641/modules/adblockplus...
File modules/adblockplus/manifests/mercurial/extension.pp (right):

https://codereview.adblockplus.org/29526640/diff/29526641/modules/adblockplus...
modules/adblockplus/manifests/mercurial/extension.pp:5: # === Parameters:
Please document parameters $config and $package, thank you!

https://codereview.adblockplus.org/29526640/diff/29526641/modules/adblockplus...
modules/adblockplus/manifests/mercurial/extension.pp:63:
ensure_resource('package', 'python-dev')
?

https://codereview.adblockplus.org/29526640/diff/29526641/modules/web/manifes...
File modules/web/manifests/server.pp (right):

https://codereview.adblockplus.org/29526640/diff/29526641/modules/web/manifes...
modules/web/manifests/server.pp:4: $remote =
"https://hg.adblockplus.org/${repository}",
The changes in here are explanatory as well?

[f.lopez, 2017-08-24 17:45:58]

The `python-dev` package can be done here or in hiera with 
adblockplus::packages, what do you think? I'll upload a new patch after your
comments

https://codereview.adblockplus.org/29526640/diff/29526641/hiera/roles/web/adb...
File hiera/roles/web/adblockplus.yaml (right):

https://codereview.adblockplus.org/29526640/diff/29526641/hiera/roles/web/adb...
hiera/roles/web/adblockplus.yaml:5: web::server::remote:
git://github.com/adblockplus/web.adblockplus.org.git
On 2017/08/24 16:40:52, mathias wrote:
> I assume the changes in this file are for demonstration and test purpose but
do
> not end up in the commit that gets pushed. Right?

Exactly!

https://codereview.adblockplus.org/29526640/diff/29526641/modules/adblockplus...
File modules/adblockplus/manifests/mercurial.pp (right):

https://codereview.adblockplus.org/29526640/diff/29526641/modules/adblockplus...
modules/adblockplus/manifests/mercurial.pp:5: # === Parameters:
On 2017/08/24 16:40:52, mathias wrote:
> Please document parameters $config and $package, thank you!

Done.

https://codereview.adblockplus.org/29526640/diff/29526641/modules/adblockplus...
modules/adblockplus/manifests/mercurial.pp:10: #     ensure => 'latest',
On 2017/08/24 16:40:52, mathias wrote:
> That is not a valid example.

Done.

https://codereview.adblockplus.org/29526640/diff/29526641/modules/adblockplus...
File modules/adblockplus/manifests/mercurial/extension.pp (right):

https://codereview.adblockplus.org/29526640/diff/29526641/modules/adblockplus...
modules/adblockplus/manifests/mercurial/extension.pp:5: # === Parameters:
On 2017/08/24 16:40:52, mathias wrote:
> Please document parameters $config and $package, thank you!

Acknowledged.

https://codereview.adblockplus.org/29526640/diff/29526641/modules/adblockplus...
modules/adblockplus/manifests/mercurial/extension.pp:63:
ensure_resource('package', 'python-dev')
On 2017/08/24 16:40:52, mathias wrote:
> ?

This is needed for installing pip packages (for hg-git, for instance), I wasn't
sure if I put this here or where...

https://codereview.adblockplus.org/29526640/diff/29526641/modules/web/manifes...
File modules/web/manifests/server.pp (right):

https://codereview.adblockplus.org/29526640/diff/29526641/modules/web/manifes...
modules/web/manifests/server.pp:4: $remote =
"https://hg.adblockplus.org/${repository}",
On 2017/08/24 16:40:52, mathias wrote:
> The changes in here are explanatory as well?

Yes, this is both for testing purposes (if you want to test it works) and how it
would look like the integration, this particular file will be uploaded for
another codereview as well, although we can use it here to see the full picture.

[mathias, 2017-08-24 18:28:38]

https://codereview.adblockplus.org/29526640/diff/29526749/modules/adblockplus...
File modules/adblockplus/manifests/mercurial/extension.pp (right):

https://codereview.adblockplus.org/29526640/diff/29526749/modules/adblockplus...
modules/adblockplus/manifests/mercurial/extension.pp:11: #   Overwrite the
default package/extension options, to fine-tune the target
One can do more with that parameter than fine-tuning the target version. No need
to explain for what, but if you do so explicitly designate it as an example
please. Same in the other manifest.

[mathias, 2017-08-24 18:39:00]

LGTM on modules/adblockplus and modules/private-stub.