Issue 29722943: #9024 - Introduce class abp::mercurial::extension::hggit

Issue 29722943: #9024 - Introduce class abp::mercurial::extension::hggit (Closed)

Created:
March 15, 2018, 1:17 a.m. by f.lopez

Modified:
March 20, 2018, 3:32 p.m.

Reviewers:
mathias, Fred

Visibility:
Public.

Description

#9024 - Introduce class abp::mercurial::extension::hggit

Patch Set 2 : Forgot to upload new file #

Total comments: 10

Patch Set 4 : For comment 4 #

Created: March 16, 2018, 6:22 p.m.

Download [raw] [tar.bz2]

		Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+41 lines, -9 lines)			Patch
	A	modules/adblockplus/manifests/mercurial/extension/hggit.pp	View	1 2 3	1 chunk	+41 lines, -0 lines	0 comments	Download
	M	modules/private-stub/hiera/base.yaml	View	1 2 3	1 chunk	+0 lines, -9 lines	0 comments	Download

Messages

Total messages: 7

Expand All Messages | Collapse All Messages

mathias

https://codereview.adblockplus.org/29722943/diff/29722946/modules/adblockplus/manifests/mercurial/extension/hggit.pp File modules/adblockplus/manifests/mercurial/extension/hggit.pp (right): https://codereview.adblockplus.org/29722943/diff/29722946/modules/adblockplus/manifests/mercurial/extension/hggit.pp#newcode4 modules/adblockplus/manifests/mercurial/extension/hggit.pp:4: # This class should be obsolete when puppet is ...

March 15, 2018, 5:57 p.m. (2018-03-15 17:57:05 UTC) #2

f.lopez

March 16, 2018, 12:24 a.m. (2018-03-16 00:24:00 UTC) #3

https://codereview.adblockplus.org/29722943/diff/29722946/modules/adblockplus...
File modules/adblockplus/manifests/mercurial/extension/hggit.pp (right):

https://codereview.adblockplus.org/29722943/diff/29722946/modules/adblockplus...
modules/adblockplus/manifests/mercurial/extension/hggit.pp:4: # This class
should be obsolete when puppet is => 4.1.0
On 2018/03/15 17:57:04, mathias wrote:
> ... and why is it required now? Without documenting the whole story this
comment
> is pointless. So please make sure either the ticket linked contains all the
info
> or the description above.

Should I include the description here? That's why I included the ticket
reference, in the ticket there is the full explanation, maybe just a few lines
explaining the problem?

https://codereview.adblockplus.org/29722943/diff/29722946/modules/adblockplus...
modules/adblockplus/manifests/mercurial/extension/hggit.pp:13: exec { "upgrade
setuptools":
On 2018/03/15 17:57:05, mathias wrote:
> Please use single quotes and no space after the bracket.

Acknowledged.

https://codereview.adblockplus.org/29722943/diff/29722946/modules/adblockplus...
modules/adblockplus/manifests/mercurial/extension/hggit.pp:15: require =>
Package['python-pip', 'libffi-dev', 'libssl-dev'],
On 2018/03/15 17:57:05, mathias wrote:
> This requires Package['python-pip'] being defined upfront, which is a) an
> implicit assumption prone to ordering issues and b) a hidden relationship
prone
> to dependency issues. For now just include it with ensure_packages() above.

Acknowledged.

https://codereview.adblockplus.org/29722943/diff/29722946/modules/adblockplus...
modules/adblockplus/manifests/mercurial/extension/hggit.pp:18: exec { "upgrade
urllib3":
On 2018/03/15 17:57:04, mathias wrote:
> Same as above.

Acknowledged.

https://codereview.adblockplus.org/29722943/diff/29722946/modules/adblockplus...
modules/adblockplus/manifests/mercurial/extension/hggit.pp:25: ensure =>
'0.8.9',
On 2018/03/15 17:57:05, mathias wrote:
> This value should be implemented as the default for an $ensure parameter of
this
> class.

Acknowledged.

mathias

https://codereview.adblockplus.org/29722943/diff/29723702/modules/adblockplus/manifests/mercurial/extension/hggit.pp File modules/adblockplus/manifests/mercurial/extension/hggit.pp (right): https://codereview.adblockplus.org/29722943/diff/29723702/modules/adblockplus/manifests/mercurial/extension/hggit.pp#newcode14 modules/adblockplus/manifests/mercurial/extension/hggit.pp:14: 'libssl-dev', Couldn't you use a $dependencies list with these ...

March 16, 2018, 9:03 a.m. (2018-03-16 09:03:33 UTC) #4

f.lopez

https://codereview.adblockplus.org/29722943/diff/29723702/modules/adblockplus/manifests/mercurial/extension/hggit.pp File modules/adblockplus/manifests/mercurial/extension/hggit.pp (right): https://codereview.adblockplus.org/29722943/diff/29723702/modules/adblockplus/manifests/mercurial/extension/hggit.pp#newcode14 modules/adblockplus/manifests/mercurial/extension/hggit.pp:14: 'libssl-dev', On 2018/03/16 09:03:33, mathias wrote: > Couldn't you ...

March 16, 2018, 6:11 p.m. (2018-03-16 18:11:46 UTC) #5

mathias

Only the commit message needs improvement now. How about "#.... - Introduce class ..::hggit" for ...

March 16, 2018, 9 p.m. (2018-03-16 21:00:30 UTC) #6

LGTM.

Expand All Messages | Collapse All Messages