Issue 1608 - Introduce HG client module in Puppet

See https://issues.adblockplus.org/ticket/1608

[Wladimir Palant, 2015-01-13 08:10:48]

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

http://codereview.adblockplus.org/6065002532306944/diff/5629499534213120/modu...
modules/hg/manifests/init.pp:38: #   }
Does this API make sense for us? We typically don't check out specific
revisions, we rather check out the current state and have it updated
automatically. As you have it implemented right now, the automatic update still
has to be done separately.

http://codereview.adblockplus.org/6065002532306944/diff/5629499534213120/modu...
modules/hg/manifests/init.pp:50: ensure => installed,
Does a custom path make sense if you require Mercurial to be installed as a
system package?

http://codereview.adblockplus.org/6065002532306944/diff/5629499534213120/modu...
modules/hg/manifests/init.pp:83: $command = shellquote('hg', '--cwd', $target,
'pull', $source)
Please use -R or --repository rather than --cwd.

http://codereview.adblockplus.org/6065002532306944/diff/5629499534213120/modu...
modules/hg/manifests/init.pp:92: $revision = 'tip',
'tip' isn't a good default, it might be on a branch. 'default' is what we
usually use (head of the default branch).

http://codereview.adblockplus.org/6065002532306944/diff/5629499534213120/modu...
modules/hg/manifests/init.pp:99: $command = shellquote('hg', '--cwd', $target,
'pull', '--rev', $revision)
Please use -R or --repository rather than --cwd.

I guess you want to run hg update here, not hg pull. hg pull won't update unless
--update option is specified.

http://codereview.adblockplus.org/6065002532306944/diff/5629499534213120/modu...
modules/hg/manifests/init.pp:106: if $hg::post_update != '' {
Shouldn't this only be executed when an actual update was performed? Meaning
that |hg id -i| output changed after running the command.

http://codereview.adblockplus.org/6065002532306944/diff/5629499534213120/modu...
modules/hg/manifests/init.pp:126: }
Should this really be run unconditionally? What if the target directory already
exists?

[Oleksandr, 2015-06-22 07:11:34]

Noissue. Just testing if comments are working...

[mathias, 2015-06-23 10:45:15]

Closing this issue for now. Will be handled in the context of the buid-server
integration.

https://codereview.adblockplus.org/6065002532306944/diff/5629499534213120/mod...
File modules/hg/manifests/init.pp (right):

https://codereview.adblockplus.org/6065002532306944/diff/5629499534213120/mod...
modules/hg/manifests/init.pp:38: #   }
On 2015/01/13 08:10:48, Wladimir Palant wrote:
> Does this API make sense for us? We typically don't check out specific
> revisions, we rather check out the current state and have it updated
> automatically. As you have it implemented right now, the automatic update
still
> has to be done separately.

Well, the revision parameter is optional (and now defaulting to "default"), so
it would behave as expected.

But the keeping-up-to-date thingy is not included here. We can, of course,
include it as an optional (!) feature. I'll have a look into that while
addressing the other feedback from this patch-set.

https://codereview.adblockplus.org/6065002532306944/diff/5629499534213120/mod...
modules/hg/manifests/init.pp:50: ensure => installed,
On 2015/01/13 08:10:48, Wladimir Palant wrote:
> Does a custom path make sense if you require Mercurial to be installed as a
> system package?

The custom path is also used to ensure the $PATH for the $post_update hook being
modifiable.