Issue 2867 - Introduce module hgweb and corresponding server role

Issue 2867 - Introduce module hgweb and corresponding server role

[Felix Dahlke, 2015-08-17 18:05:04]

Looks pretty good as far as I'm concerned, just a few smaller comments. Plus,
I'm wondering if "hgserver" would be a more appropriate name for the module -
it's a bit more than just hgweb after all.

https://codereview.adblockplus.org/29323409/diff/29323754/modules/hgweb/files...
File modules/hgweb/files/hgaccess/repos/hgaccess (right):

https://codereview.adblockplus.org/29323409/diff/29323754/modules/hgweb/files...
modules/hgweb/files/hgaccess/repos/hgaccess:1: vagrant
Shouldn't this move to modules/private?

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

https://codereview.adblockplus.org/29323409/diff/29323754/modules/hgweb/manif...
modules/hgweb/manifests/init.pp:121: 
Nit: Superfluous empty line? Can't tell if it's deliberate.

[mathias, 2015-08-17 18:16:45]

Well, I don't really care about the naming that much. But if I where to argue
about this, I'd probably mention the both the fact that we may want to avoid the
meaningless server suffix, as well as the fact that none of our custom modules
are "clean" (read: "easily exportable") setups.

https://codereview.adblockplus.org/29323409/diff/29323754/modules/hgweb/files...
File modules/hgweb/files/hgaccess/repos/hgaccess (right):

https://codereview.adblockplus.org/29323409/diff/29323754/modules/hgweb/files...
modules/hgweb/files/hgaccess/repos/hgaccess:1: vagrant
On 2015/08/17 18:05:03, Felix Dahlke wrote:
> Shouldn't this move to modules/private?

There's no need for that, one can easily introduce a private version and link it
using the $hgweb::hgaccess parameter. One can also use this one as-is, and
immediately after the initial provision push new content to the repository with
the Vagrant key - just as one would do in development -, but then also removing
the Vagrant key in the same turn.

By the way, I recommend having the Vagrant key registered with the own agent
anyway. When working with Vagrant, especially when debugging multiple boxes that
talk to each other, that is.

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

https://codereview.adblockplus.org/29323409/diff/29323754/modules/hgweb/manif...
modules/hgweb/manifests/init.pp:121: 
On 2015/08/17 18:05:03, Felix Dahlke wrote:
> Nit: Superfluous empty line? Can't tell if it's deliberate.

It isn't. Wasn't.

[Felix Dahlke, 2015-08-17 18:21:20]

Wouldn't insist on a different name, hgweb is fine by me.

https://codereview.adblockplus.org/29323409/diff/29323754/modules/hgweb/files...
File modules/hgweb/files/hgaccess/repos/hgaccess (right):

https://codereview.adblockplus.org/29323409/diff/29323754/modules/hgweb/files...
modules/hgweb/files/hgaccess/repos/hgaccess:1: vagrant
On 2015/08/17 18:16:45, mathias wrote:
> On 2015/08/17 18:05:03, Felix Dahlke wrote:
> > Shouldn't this move to modules/private?
> 
> There's no need for that, one can easily introduce a private version and link
it
> using the $hgweb::hgaccess parameter. One can also use this one as-is, and
> immediately after the initial provision push new content to the repository
with
> the Vagrant key - just as one would do in development -, but then also
removing
> the Vagrant key in the same turn.

Seems pretty inconsistent with how we usually do it to me: We require a resource
from private, then supply something practical for development in private-stub.
Why handle it different in this particular module?

[mathias, 2015-08-17 18:28:09]

On 2015/08/17 18:21:20, Felix Dahlke wrote:
> Seems pretty inconsistent with how we usually do it to me: We require a
resource
> from private, then supply something practical for development in private-stub.
> Why handle it different in this particular module?

This is not the first time, neither for the module nor for this topic:
https://codereview.adblockplus.org/5686568333344768/#msg5

[Felix Dahlke, 2015-08-17 18:43:07]

On 2015/08/17 18:28:09, mathias wrote:
> On 2015/08/17 18:21:20, Felix Dahlke wrote:
> > Seems pretty inconsistent with how we usually do it to me: We require a
> resource
> > from private, then supply something practical for development in
private-stub.
> > Why handle it different in this particular module?
> 
> This is not the first time, neither for the module nor for this topic:
> https://codereview.adblockplus.org/5686568333344768/#msg5

Oh, fair enough. LGTM.

[Wladimir Palant, 2015-08-24 16:49:49]

https://codereview.adblockplus.org/29323409/diff/29323871/modules/hgweb/files...
File modules/hgweb/files/nginx.conf (right):

https://codereview.adblockplus.org/29323409/diff/29323871/modules/hgweb/files...
modules/hgweb/files/nginx.conf:1: add_header Strict-Transport-Security
"max-age=2592000";
This is duplicating String-Transport-Security header from the generic
configuration - this server is currently sending two String-Transport-Security
headers.

https://codereview.adblockplus.org/29323409/diff/29323871/modules/hgweb/files...
modules/hgweb/files/nginx.conf:3: location / {
We generally put opening brackets on the next line in nginx configuration.

https://codereview.adblockplus.org/29323409/diff/29323871/modules/hgweb/files...
modules/hgweb/files/nginx.conf:12: root /usr/share/mercurial/templates;
This isn't pointing to the Mercurial instance running on the server right now,
correct path would be
/usr/local/lib/python2.7/dist-packages/mercurial/templates/.

[mathias, 2015-08-24 17:38:46]

See https://codereview.adblockplus.org/29324553/ --

https://codereview.adblockplus.org/29323409/diff/29323871/modules/hgweb/files...
File modules/hgweb/files/nginx.conf (right):

https://codereview.adblockplus.org/29323409/diff/29323871/modules/hgweb/files...
modules/hgweb/files/nginx.conf:1: add_header Strict-Transport-Security
"max-age=2592000";
On 2015/08/24 16:49:48, Wladimir Palant wrote:
> This is duplicating String-Transport-Security header from the generic
> configuration - this server is currently sending two String-Transport-Security
> headers.

Done.

https://codereview.adblockplus.org/29323409/diff/29323871/modules/hgweb/files...
modules/hgweb/files/nginx.conf:3: location / {
On 2015/08/24 16:49:48, Wladimir Palant wrote:
> We generally put opening brackets on the next line in nginx configuration.

Done.

https://codereview.adblockplus.org/29323409/diff/29323871/modules/hgweb/files...
modules/hgweb/files/nginx.conf:12: root /usr/share/mercurial/templates;
On 2015/08/24 16:49:48, Wladimir Palant wrote:
> This isn't pointing to the Mercurial instance running on the server right now,
> correct path would be
> /usr/local/lib/python2.7/dist-packages/mercurial/templates/.

Done.