Issue 2909 - Allow for adjusting the location of $hgweb::templates

Issue 2909 - Allow for adjusting the location of $hgweb::templates

[Wladimir Palant, 2015-08-24 18:37:02]

https://codereview.adblockplus.org/29324553/diff/29324554/modules/hgweb/manif...
File modules/hgweb/manifests/init.pp (right):

https://codereview.adblockplus.org/29324553/diff/29324554/modules/hgweb/manif...
modules/hgweb/manifests/init.pp:37: $templates = hiera('hgweb::templates',
'/usr/share/mercurial/templates'),
I might have said this before but I'm no fan of adding configurability for the
sake of configurability. Configuration options always add a cost, and be it
losing code locality like in this case. Why does this path have to be
configured? The mercurial-common package is being installed right below, and we
*know* its template path - no need for configuring.

The real problem here is that this configuration isn't what's currently running
on our server. That's what needs fixing, ASAP.

[mathias, 2015-08-24 19:03:08]

https://codereview.adblockplus.org/29324553/diff/29324554/modules/hgweb/manif...
File modules/hgweb/manifests/init.pp (right):

https://codereview.adblockplus.org/29324553/diff/29324554/modules/hgweb/manif...
modules/hgweb/manifests/init.pp:37: $templates = hiera('hgweb::templates',
'/usr/share/mercurial/templates'),
On 2015/08/24 18:37:02, Wladimir Palant wrote:
> I might have said this before but I'm no fan of adding  configurability for
the sake of configurability.

Me neither.

> Configuration options always add a cost, and be it losing code locality like
in this case. Why does this path have to be
> configured? The mercurial-common package is being installed right below, and
we
> *know* its template path - no need for configuring.

Exactly. There is no need to configure the module if one does not temper with
the Mercurial installation. It behaves properly by default.

> The real problem here is that this configuration isn't what's currently
running on our server. That's what needs fixing, ASAP.

You're right again. But you seem to confuse not being informed about the
rationale with being confronted with naive code. Thus below please find some
information on why I chose this approach:

There is no need for tweaking the system Mercurial packages implied within the
hgweb module whatsoever. This module runs from scratch, requires just a domain
name to be considered setup properly. Everything else is a feature we've added
because of a specific requirement.

Now the entire repository management itself has not been integrated with Puppet
yet (and maybe never will be, that depends on whether there's enough demand, of
course). Thus everything beyond the server setup is done by those who have
access to it. This includes not only the repository administration, but also the
synchronization. And the latter is the sole reason for using a custom Mercurial.

I do not see any reason for moving this specific detail to the hgweb module. At
least not yet, primarily because doing this properly requires integrating the
repository administration as well. And this requires time I do not have.
Remember that this is an intermediate project I suggested because of our urge to
setup the Git sync properly. 

Furthermore, it's quite possible that we decide to e.g. improve on the server2
setup, which features it's own hgweb setup - without any Git stuff - and may
thus become a great opportunity to re-use the module as-is.

To make a long story short: I am convinced that the approach I've suggested is
both reasonable and lightweight. I am not convinced that it's worth the effort
of writing down to be honest, but it seems like it's still necessary.

[Felix Dahlke, 2015-09-25 15:40:01]

On 2015/08/24 19:03:08, mathias wrote:
>
https://codereview.adblockplus.org/29324553/diff/29324554/modules/hgweb/manif...
> File modules/hgweb/manifests/init.pp (right):
> 
>
https://codereview.adblockplus.org/29324553/diff/29324554/modules/hgweb/manif...
> modules/hgweb/manifests/init.pp:37: $templates = hiera('hgweb::templates',
> '/usr/share/mercurial/templates'),
> On 2015/08/24 18:37:02, Wladimir Palant wrote:
> > I might have said this before but I'm no fan of adding  configurability for
> the sake of configurability.
> 
> Me neither.
> 
> > Configuration options always add a cost, and be it losing code locality like
> in this case. Why does this path have to be
> > configured? The mercurial-common package is being installed right below, and
> we
> > *know* its template path - no need for configuring.
> 
> Exactly. There is no need to configure the module if one does not temper with
> the Mercurial installation. It behaves properly by default.
> 
> > The real problem here is that this configuration isn't what's currently
> running on our server. That's what needs fixing, ASAP.
> 
> You're right again. But you seem to confuse not being informed about the
> rationale with being confronted with naive code. Thus below please find some
> information on why I chose this approach:
> 
> There is no need for tweaking the system Mercurial packages implied within the
> hgweb module whatsoever. This module runs from scratch, requires just a domain
> name to be considered setup properly. Everything else is a feature we've added
> because of a specific requirement.
> 
> Now the entire repository management itself has not been integrated with
Puppet
> yet (and maybe never will be, that depends on whether there's enough demand,
of
> course). Thus everything beyond the server setup is done by those who have
> access to it. This includes not only the repository administration, but also
the
> synchronization. And the latter is the sole reason for using a custom
Mercurial.
> 
> I do not see any reason for moving this specific detail to the hgweb module.
At
> least not yet, primarily because doing this properly requires integrating the
> repository administration as well. And this requires time I do not have.
> Remember that this is an intermediate project I suggested because of our urge
to
> setup the Git sync properly. 
> 
> Furthermore, it's quite possible that we decide to e.g. improve on the server2
> setup, which features it's own hgweb setup - without any Git stuff - and may
> thus become a great opportunity to re-use the module as-is.
> 
> To make a long story short: I am convinced that the approach I've suggested is
> both reasonable and lightweight. I am not convinced that it's worth the effort
> of writing down to be honest, but it seems like it's still necessary.

So, in a nut shell, the setup on hg1 is largely manual and requires a different
template path? And we can't easily change it there? Then it makes sense to make
it configurable via Hiera I suppose... Everything is better than to have this
hot fixed in production.

[Felix Dahlke, 2015-10-01 06:46:28]

On 2015/09/25 15:40:01, Felix Dahlke wrote:
> On 2015/08/24 19:03:08, mathias wrote:
> >
>
https://codereview.adblockplus.org/29324553/diff/29324554/modules/hgweb/manif...
> > File modules/hgweb/manifests/init.pp (right):
> > 
> >
>
https://codereview.adblockplus.org/29324553/diff/29324554/modules/hgweb/manif...
> > modules/hgweb/manifests/init.pp:37: $templates = hiera('hgweb::templates',
> > '/usr/share/mercurial/templates'),
> > On 2015/08/24 18:37:02, Wladimir Palant wrote:
> > > I might have said this before but I'm no fan of adding  configurability
for
> > the sake of configurability.
> > 
> > Me neither.
> > 
> > > Configuration options always add a cost, and be it losing code locality
like
> > in this case. Why does this path have to be
> > > configured? The mercurial-common package is being installed right below,
and
> > we
> > > *know* its template path - no need for configuring.
> > 
> > Exactly. There is no need to configure the module if one does not temper
with
> > the Mercurial installation. It behaves properly by default.
> > 
> > > The real problem here is that this configuration isn't what's currently
> > running on our server. That's what needs fixing, ASAP.
> > 
> > You're right again. But you seem to confuse not being informed about the
> > rationale with being confronted with naive code. Thus below please find some
> > information on why I chose this approach:
> > 
> > There is no need for tweaking the system Mercurial packages implied within
the
> > hgweb module whatsoever. This module runs from scratch, requires just a
domain
> > name to be considered setup properly. Everything else is a feature we've
added
> > because of a specific requirement.
> > 
> > Now the entire repository management itself has not been integrated with
> Puppet
> > yet (and maybe never will be, that depends on whether there's enough demand,
> of
> > course). Thus everything beyond the server setup is done by those who have
> > access to it. This includes not only the repository administration, but also
> the
> > synchronization. And the latter is the sole reason for using a custom
> Mercurial.
> > 
> > I do not see any reason for moving this specific detail to the hgweb module.
> At
> > least not yet, primarily because doing this properly requires integrating
the
> > repository administration as well. And this requires time I do not have.
> > Remember that this is an intermediate project I suggested because of our
urge
> to
> > setup the Git sync properly. 
> > 
> > Furthermore, it's quite possible that we decide to e.g. improve on the
server2
> > setup, which features it's own hgweb setup - without any Git stuff - and may
> > thus become a great opportunity to re-use the module as-is.
> > 
> > To make a long story short: I am convinced that the approach I've suggested
is
> > both reasonable and lightweight. I am not convinced that it's worth the
effort
> > of writing down to be honest, but it seems like it's still necessary.
> 
> So, in a nut shell, the setup on hg1 is largely manual and requires a
different
> template path? And we can't easily change it there? Then it makes sense to
make
> it configurable via Hiera I suppose... Everything is better than to have this
> hot fixed in production.

So, what I don't understand is: Why _are_ the templates not in the default
location on hg1? Do I understand it correctly that we're not using the Mercurial
installed by Puppet, but a manually installed one? If so, for what reason?

[mathias, 2015-10-01 07:04:07]

> Do I understand it correctly that we're not using the Mercurial installed by
Puppet, but a manually installed one?

Yes.

> If so, for what reason?

It is a requirement for the hg-git version in use, which is not a part of the
Puppet setup yet.

[Felix Dahlke, 2015-10-01 08:37:10]

On 2015/10/01 07:04:07, mathias wrote:
> It is a requirement for the hg-git version in use, which is not a part of the
> Puppet setup yet.

OK, follow-up question: Couldn't we install the Mercurial version we need
through Puppet? It's presumably been installed through pip, Puppet can do that,
can't it?

Related question: Have we documented the manual setup of hg1 somewhere? Ideally
in follow-up issues I guess - we should remove the manual bits here over time.

[mathias, 2015-10-01 08:51:53]

> OK, follow-up question: Couldn't we install the Mercurial version we need
> through Puppet? It's presumably been installed through pip, Puppet can do
that,
> can't it?

That is possible, yes. But similar to a proper hgweb integration, that would
require a bit more effort than "just" another package resource. We would need to
make sure it does not conflict with the Mercurial resources from the base
class/role in Puppet.

> Related question: Have we documented the manual setup of hg1 somewhere?
Ideally
> in follow-up issues I guess - we should remove the manual bits here over time.

No we have not documented much about that yet, clearly a follow-up.

[Felix Dahlke, 2015-10-01 09:05:03]

On 2015/10/01 08:51:53, mathias wrote:
> > OK, follow-up question: Couldn't we install the Mercurial version we need
> > through Puppet? It's presumably been installed through pip, Puppet can do
> that,
> > can't it?
> 
> That is possible, yes. But similar to a proper hgweb integration, that would
> require a bit more effort than "just" another package resource. We would need
to
> make sure it does not conflict with the Mercurial resources from the base
> class/role in Puppet.

OK, I see, wow. Then I'm OK with this approach, under two conditions:

1. We create a follow-up issue for installing the Mercurial version required by
hg-git through Puppet here.
2. We add a comment to the templates parameter (it's not documented at all yet,
if I saw that correctly) that explains why the parameter is necessary, and links
to the newly created follow-up issue.

Moving Wladimir to CC - if he can think of a solution to this that's a similarly
small effort, we can always back this out and apply the better patch. But I'm
_very_ uncomfortable with having a hot fix live here.

[mathias, 2015-10-01 12:02:05]

Follow-up: https://issues.adblockplus.org/ticket/3145

[Felix Dahlke, 2015-10-12 08:10:13]

LGTM (Anyone reading this, see my last comment however - we should solve this
properly at some point.)