Issue 2184 - Configure codereview setup to use PostgreSQL

Note that it is planned to migrate the Nginx setup from class rietveld to class codereview in a subsequent patch-set. Also, see https://issues.adblockplus.org/ticket/2184 for more information --

[Felix Dahlke, 2015-04-22 05:32:22]

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

http://codereview.adblockplus.org/5686568333344768/diff/5629499534213120/modu...
modules/codereview/manifests/init.pp:45: $database_account =
hiera('codereview::database_account', 'rietveld'),
Should we really use defaults here? Seems preferable to have things fail faster.
If we have to provide a default, how about an empty string?

http://codereview.adblockplus.org/5686568333344768/diff/5629499534213120/modu...
modules/codereview/manifests/init.pp:50: class {'postgresql::server':
Isn't this just an "include" then?

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

http://codereview.adblockplus.org/5686568333344768/diff/5629499534213120/modu...
modules/rietveld/manifests/init.pp:8: $database = hiera('rietveld::database', {
Shouldn't we just set engine and name as below in the hiera configuration for
that node and fail if it's not specified?

[mathias, 2015-04-23 16:28:49]

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

http://codereview.adblockplus.org/5686568333344768/diff/5629499534213120/modu...
modules/codereview/manifests/init.pp:45: $database_account =
hiera('codereview::database_account', 'rietveld'),
On 2015/04/22 05:32:23, Felix H. Dahlke wrote:
> Should we really use defaults here? Seems preferable to have things fail
faster.
> If we have to provide a default, how about an empty string?

See my other comment regarding the rietveld parameters.

http://codereview.adblockplus.org/5686568333344768/diff/5629499534213120/modu...
modules/codereview/manifests/init.pp:50: class {'postgresql::server':
On 2015/04/22 05:32:23, Felix H. Dahlke wrote:
> Isn't this just an "include" then?

You're right, it makes more sense to use an include here. That would even allow
to set any parameters for the server (e.g. password) in Hiera, without any
changes to the code. The next patch-set will contain that change, but please
answer my other question regarding the default parameters first.

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

http://codereview.adblockplus.org/5686568333344768/diff/5629499534213120/modu...
modules/rietveld/manifests/init.pp:8: $database = hiera('rietveld::database', {
On 2015/04/22 05:32:23, Felix H. Dahlke wrote:
> Shouldn't we just set engine and name as below in the hiera configuration for
> that node and fail if it's not specified?

The idea is to have a working development setup without additional configuration
(which is possible anywhere with Hiera, in theory at least) - just including the
class should be enough (the $domain, $certificate, etc parameters are meant to
vanish from this file in another patch-set, as mentioned earlier). One could of
course argue that failing early when someone did not remember to configure every
requirement is more important than an out-of-the-box working solution. This
boils down to personal preference or philosophy, I guess. For myself, I prefer
the out-of-the-box approach (though I can live with either one). What about you?

[Felix Dahlke, 2015-05-11 20:10:44]

Sorry for the delay.

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

http://codereview.adblockplus.org/5686568333344768/diff/5629499534213120/modu...
modules/rietveld/manifests/init.pp:8: $database = hiera('rietveld::database', {
On 2015/04/23 16:28:49, matze wrote:
> On 2015/04/22 05:32:23, Felix H. Dahlke wrote:
> > Shouldn't we just set engine and name as below in the hiera configuration
for
> > that node and fail if it's not specified?
> 
> The idea is to have a working development setup without additional
configuration
> (which is possible anywhere with Hiera, in theory at least) - just including
the
> class should be enough (the $domain, $certificate, etc parameters are meant to
> vanish from this file in another patch-set, as mentioned earlier). One could
of
> course argue that failing early when someone did not remember to configure
every
> requirement is more important than an out-of-the-box working solution. This
> boils down to personal preference or philosophy, I guess. For myself, I prefer
> the out-of-the-box approach (though I can live with either one). What about
you?

So you mean you'd want to move away from private-stub in the long run and use
defaults instead?

[mathias, 2015-05-13 09:39:45]

On 2015/05/11 20:10:44, Felix H. Dahlke wrote:

> So you mean you'd want to move away from private-stub in the long run and use
> defaults instead?

Well, I'd love to get rid of it, but that's not realistic (for quite a while, at
least), especially because there's still a need for storing private files, where
we'd need an alternative approach then.

However, as we've discussed last year, there's a lot of possible improvements to
the way we write our modules - in order to increase robustness, comply more with
the DRY principle, ease the understanding and so on. We have agreed that writing
non-ABP-specific modules (like rietveld, but unlike codereviewserver to pick up
this very example here) as if they are meant to become published as standalone
ones. While that's not really the plan, it's a pretty good guideline to sum up a
lot of improvements.

In this context; I believe that a good module should be able to become set up
and running with as less effort as possible. Thus using suitable defaults is
probably a key factor.

[Felix Dahlke, 2015-05-13 11:45:04]

On 2015/05/13 09:39:45, matze wrote:
> On 2015/05/11 20:10:44, Felix H. Dahlke wrote:
> 
> > So you mean you'd want to move away from private-stub in the long run and
use
> > defaults instead?
> 
> Well, I'd love to get rid of it, but that's not realistic (for quite a while,
at
> least), especially because there's still a need for storing private files,
where
> we'd need an alternative approach then.
> 
> However, as we've discussed last year, there's a lot of possible improvements
to
> the way we write our modules - in order to increase robustness, comply more
with
> the DRY principle, ease the understanding and so on. We have agreed that
writing
> non-ABP-specific modules (like rietveld, but unlike codereviewserver to pick
up
> this very example here) as if they are meant to become published as standalone
> ones. While that's not really the plan, it's a pretty good guideline to sum up
a
> lot of improvements.
> 
> In this context; I believe that a good module should be able to become set up
> and running with as less effort as possible. Thus using suitable defaults is
> probably a key factor.

Alright, SQLite and that generic database name are indeed reasonable defaults
that doesn't necessarily have to be overwritten.

[Felix Dahlke, 2015-05-13 11:45:19]

On 2015/05/13 11:45:04, Felix H. Dahlke wrote:
> On 2015/05/13 09:39:45, matze wrote:
> > On 2015/05/11 20:10:44, Felix H. Dahlke wrote:
> > 
> > > So you mean you'd want to move away from private-stub in the long run and
> use
> > > defaults instead?
> > 
> > Well, I'd love to get rid of it, but that's not realistic (for quite a
while,
> at
> > least), especially because there's still a need for storing private files,
> where
> > we'd need an alternative approach then.
> > 
> > However, as we've discussed last year, there's a lot of possible
improvements
> to
> > the way we write our modules - in order to increase robustness, comply more
> with
> > the DRY principle, ease the understanding and so on. We have agreed that
> writing
> > non-ABP-specific modules (like rietveld, but unlike codereviewserver to pick
> up
> > this very example here) as if they are meant to become published as
standalone
> > ones. While that's not really the plan, it's a pretty good guideline to sum
up
> a
> > lot of improvements.
> > 
> > In this context; I believe that a good module should be able to become set
up
> > and running with as less effort as possible. Thus using suitable defaults is
> > probably a key factor.
> 
> Alright, SQLite and that generic database name are indeed reasonable defaults
> that doesn't necessarily have to be overwritten.

Sorry, forgot the LGTM :D