Issue 5459448122310656: Add rietveld to infrastructure

Issue 5459448122310656: Add rietveld to infrastructure (Closed)

Created:
Feb. 7, 2014, 8:46 a.m. by christian

Modified:
Feb. 12, 2014, 5:34 p.m.

Reviewers:
Felix Dahlke

Visibility:
Public.

Patch Set 4 : The two comments #

Total comments: 2

Patch Set 5 : Remove small dependency #

Created: Feb. 12, 2014, 4:44 p.m.

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+273 lines, -1 line)			Patch
M	Vagrantfile	View	1	1 chunk	+1 line, -0 lines	0 comments	Download
A	manifests/codereviewserver.pp	View	1 2	1 chunk	+10 lines, -0 lines	0 comments	Download
M	manifests/monitoringserver.pp	View	1 2	1 chunk	+2 lines, -1 line	0 comments	Download
M	manifests/nodes.pp	View	1 2	1 chunk	+1 line, -0 lines	0 comments	Download
M	manifests/vagrant.pp	View	1	1 chunk	+1 line, -0 lines	0 comments	Download
A	modules/private-stub/manifests/rietveld.pp	View	1	1 chunk	+4 lines, -0 lines	0 comments	Download
A	modules/rietveld/files/Makefile	View		1 chunk	+46 lines, -0 lines	0 comments	Download
A	modules/rietveld/files/rietveld.conf	View		1 chunk	+14 lines, -0 lines	0 comments	Download
A	modules/rietveld/manifests/init.pp	View	1 2 3 4	1 chunk	+80 lines, -0 lines	0 comments	Download
A	modules/rietveld/templates/settings.py.erb	View	1	1 chunk	+107 lines, -0 lines	0 comments	Download
A	modules/rietveld/templates/site.erb	View		1 chunk	+7 lines, -0 lines	0 comments	Download

Messages

Total messages: 10

Expand All Messages | Collapse All Messages

Felix Dahlke

Looks good in general, tons of nits and some comments below :) http://codereview.adblockplus.org/5459448122310656/diff/5629499534213120/Vagrantfile File Vagrantfile ...

Feb. 7, 2014, 3:27 p.m. (2014-02-07 15:27:36 UTC) #2

christian

Apply Changes http://codereview.adblockplus.org/5459448122310656/diff/5629499534213120/Vagrantfile File Vagrantfile (right): http://codereview.adblockplus.org/5459448122310656/diff/5629499534213120/Vagrantfile#newcode54 Vagrantfile:54: define_standard_vm config, 'codereview', '10.8.0.132' On 2014/02/07 15:27:36, ...

Feb. 7, 2014, 5:17 p.m. (2014-02-07 17:17:21 UTC) #3

Apply Changes

http://codereview.adblockplus.org/5459448122310656/diff/5629499534213120/Vagr...
File Vagrantfile (right):

http://codereview.adblockplus.org/5459448122310656/diff/5629499534213120/Vagr...
Vagrantfile:54: define_standard_vm config, 'codereview', '10.8.0.132'
On 2014/02/07 15:27:36, Felix H. Dahlke wrote:
> Should be "codereview1" with our naming scheme.
Done.

http://codereview.adblockplus.org/5459448122310656/diff/5629499534213120/mani...
File manifests/codereview.pp (right):

http://codereview.adblockplus.org/5459448122310656/diff/5629499534213120/mani...
manifests/codereview.pp:4: class {'rietveld':
On 2014/02/07 15:27:36, Felix H. Dahlke wrote:
> Wrong indentation, should be 2 spaces.

Done.

http://codereview.adblockplus.org/5459448122310656/diff/5629499534213120/modu...
File modules/rietveld/files/settings.py (right):

http://codereview.adblockplus.org/5459448122310656/diff/5629499534213120/modu...
modules/rietveld/files/settings.py:16: DATABASE_ENGINE = 'sqlite3'    #
'postgresql_psycopg2', 'postgresql', 'mysql', 'sqlite3' or 'oracle'.
On 2014/02/07 15:27:36, Felix H. Dahlke wrote:
> I think we should use MySQL or PostgreSQL, whatever is the better supported
> choice.

I was thinking about this, my idea was, that i can setup the whole data
migration on my notebook, and than just copy the sqlite file over. Than we have
a vanilla rietveld installation, what make upgrades easier, my changes are only
need for the initial setup, so this might be a good idea?

http://codereview.adblockplus.org/5459448122310656/diff/5629499534213120/modu...
modules/rietveld/files/settings.py:28: TIME_ZONE = 'America/Chicago'
On 2014/02/07 15:27:36, Felix H. Dahlke wrote:
> Shouldn't we set this to our time zone?

Done.

http://codereview.adblockplus.org/5459448122310656/diff/5629499534213120/modu...
modules/rietveld/files/settings.py:55: SECRET_KEY =
'el@4s$*(idwm5-87teftxlksckmy8$tyo7(tm!n-5x)zeuheex'
On 2014/02/07 15:27:36, Felix H. Dahlke wrote:
> Sounds to me like this belongs in private::rietveld.

Done.

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

http://codereview.adblockplus.org/5459448122310656/diff/5629499534213120/modu...
modules/rietveld/manifests/init.pp:19: exec { "get_rietveld":
On 2014/02/07 15:27:36, Felix H. Dahlke wrote:
> No space after {, and ' instead of ". Likewise below.

Done.

http://codereview.adblockplus.org/5459448122310656/diff/5629499534213120/modu...
modules/rietveld/manifests/init.pp:20: command => "hg clone
https://code.google.com/p/django-gae2django/ /home/rietveld/django-gae2django/",
On 2014/02/07 15:27:36, Felix H. Dahlke wrote:
> ' instead of ", likewise various times below. According to the style guide,
> strings that don't contain variables should use single quotes.

Done.

http://codereview.adblockplus.org/5459448122310656/diff/5629499534213120/modu...
modules/rietveld/manifests/init.pp:23: onlyif => "test ! -d
/home/rietveld/django-gae2django",
On 2014/02/07 15:27:36, Felix H. Dahlke wrote:
> This path (/home/rietveld/django-gae2django) is being repeated a lot, should
be
> a variable.

Done.

http://codereview.adblockplus.org/5459448122310656/diff/5629499534213120/modu...
modules/rietveld/manifests/init.pp:24: path =>
"/usr/bin:/usr/sbin:/bin:/usr/local/bin",
On 2014/02/07 15:27:36, Felix H. Dahlke wrote:
> This is repeated below, you can have this above instead:
> 
> Exec {
>   path => "…"
> }
> 
> That'd apply to every exec resource.

Done.

http://codereview.adblockplus.org/5459448122310656/diff/5629499534213120/modu...
modules/rietveld/manifests/init.pp:25: #refreshonly => true,
On 2014/02/07 15:27:36, Felix H. Dahlke wrote:
> Let's remove this if we don't need it. Likewise below.

Done.

http://codereview.adblockplus.org/5459448122310656/diff/5629499534213120/modu...
modules/rietveld/manifests/init.pp:28: file
{'/home/rietveld/django-gae2django/examples/rietveld/Makefile':
On 2014/02/07 15:27:36, Felix H. Dahlke wrote:
> This path (/home/rietveld/django-gae2django/examples/rietveld) is also being
> used a lot and should be a variable IMO.

Done.

http://codereview.adblockplus.org/5459448122310656/diff/5629499534213120/modu...
modules/rietveld/manifests/init.pp:43: command => "make all",
On 2014/02/07 15:27:36, Felix H. Dahlke wrote:
> "all" is not necessary
 Actual it is,see Makefile

default:
	@echo "Run 'make all' to fetch required sources to run this example."

http://codereview.adblockplus.org/5459448122310656/diff/5629499534213120/modu...
modules/rietveld/manifests/init.pp:47:
File['/home/rietveld/django-gae2django/examples/rietveld/Makefile'],
On 2014/02/07 15:27:36, Felix H. Dahlke wrote:
> Should be indented just one level deeper than the line above, i.e. 2 spaces.
> Likewise below.

Done.

http://codereview.adblockplus.org/5459448122310656/diff/5629499534213120/modu...
modules/rietveld/manifests/init.pp:61: #work around puppets limitation towards
upstart
On 2014/02/07 15:27:36, Felix H. Dahlke wrote:
> This is not really clear, what particular limitation is there with upstart and
> Puppet that causes us to fall back on init scripts here? Any particular bug
you
> can link to?
It is simple, puppet is not aware of upstart so it tries to start up via init.d,
so I use this method to not to include maybe a buggy third party method to use
the upstart script. I have not found a particular bug, but some things in their
tracker:
https://tickets.puppetlabs.com/browse/PUP-876?jql=text%20~%20%22upstart%22

http://codereview.adblockplus.org/5459448122310656/diff/5629499534213120/modu...
modules/rietveld/manifests/init.pp:65: require =>
File['/etc/init/rietveld.conf']
On 2014/02/07 15:27:36, Felix H. Dahlke wrote:
> Shouldn't this require Exec['install_rietveld']? Not requiring anything would
> also be fine, but I think it'd make sense to do this after the installation.
> That way you wouldn't have to require Exec['install_rietveld'] below in the
> service resource.
This only makes the symlink, the service it self need the install. It's just
shifting the need for the install from the service to the file. And for me it
was more logical to put it by the service. But if you think it would be more
belong here, I can put the dependency here.

http://codereview.adblockplus.org/5459448122310656/diff/5629499534213120/modu...
modules/rietveld/manifests/init.pp:75: command => "echo \"from django.db import
DEFAULT_DB_ALIAS as database; from django.contrib.auth.models import User;
User.objects.db_manager(database).create_superuser('rietveld',
'rietveld@example.org', '${admin_password}')\" | ./manage.py shell",
On 2014/02/07 15:27:36, Felix H. Dahlke wrote:
> I suppose it's fine to just have a single admin account for now, although
shared
> accounts are generally a bit of a pain. But it should probably be called
"admin"
> and definitely have "admins@adblockplus.org" as its email address.
> 
> Also, can't we do this in settings.py?

I found it easier to just make this one account and enter the rest of the
accounts via the webinterface, we only need to do this once. And we cannot do
this via settings.py, because we have no oauth in this version, if i remember
correctly.

Felix Dahlke

http://codereview.adblockplus.org/5459448122310656/diff/5629499534213120/modules/rietveld/files/settings.py File modules/rietveld/files/settings.py (right): http://codereview.adblockplus.org/5459448122310656/diff/5629499534213120/modules/rietveld/files/settings.py#newcode16 modules/rietveld/files/settings.py:16: DATABASE_ENGINE = 'sqlite3' # 'postgresql_psycopg2', 'postgresql', 'mysql', 'sqlite3' or ...

Feb. 7, 2014, 8:09 p.m. (2014-02-07 20:09:44 UTC) #4

http://codereview.adblockplus.org/5459448122310656/diff/5629499534213120/modu...
File modules/rietveld/files/settings.py (right):

http://codereview.adblockplus.org/5459448122310656/diff/5629499534213120/modu...
modules/rietveld/files/settings.py:16: DATABASE_ENGINE = 'sqlite3'    #
'postgresql_psycopg2', 'postgresql', 'mysql', 'sqlite3' or 'oracle'.
On 2014/02/07 17:17:21, christian wrote:
> On 2014/02/07 15:27:36, Felix H. Dahlke wrote:
> > I think we should use MySQL or PostgreSQL, whatever is the better supported
> > choice.
> 
> I was thinking about this, my idea was, that i can setup the whole data
> migration on my notebook, and than just copy the sqlite file over. Than we
have
> a vanilla rietveld installation, what make upgrades easier, my changes are
only
> need for the initial setup, so this might be a good idea?

Yeah, we can do it in a follow-up, you can create a Trello card for this after
you pushed this.

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

http://codereview.adblockplus.org/5459448122310656/diff/5629499534213120/modu...
modules/rietveld/manifests/init.pp:43: command => "make all",
On 2014/02/07 17:17:21, christian wrote:
> On 2014/02/07 15:27:36, Felix H. Dahlke wrote:
> > "all" is not necessary
>  Actual it is,see Makefile
> 
> default:
> 	@echo "Run 'make all' to fetch required sources to run this example."

Ah. Well that's peculiar :)

http://codereview.adblockplus.org/5459448122310656/diff/5629499534213120/modu...
modules/rietveld/manifests/init.pp:61: #work around puppets limitation towards
upstart
On 2014/02/07 17:17:21, christian wrote:
> On 2014/02/07 15:27:36, Felix H. Dahlke wrote:
> > This is not really clear, what particular limitation is there with upstart
and
> > Puppet that causes us to fall back on init scripts here? Any particular bug
> you
> > can link to?
> It is simple, puppet is not aware of upstart so it tries to start up via
init.d,
> so I use this method to not to include maybe a buggy third party method to use
> the upstart script. I have not found a particular bug, but some things in
their
> tracker:
>
https://tickets.puppetlabs.com/browse/PUP-876?jql=text%2520%7E%2520%2522upsta...

If Puppet can't do Upstart at all, I don't think this deserves a comment at all
actually. It's to be expected then.

http://codereview.adblockplus.org/5459448122310656/diff/5629499534213120/modu...
modules/rietveld/manifests/init.pp:65: require =>
File['/etc/init/rietveld.conf']
On 2014/02/07 17:17:21, christian wrote:
> On 2014/02/07 15:27:36, Felix H. Dahlke wrote:
> > Shouldn't this require Exec['install_rietveld']? Not requiring anything
would
> > also be fine, but I think it'd make sense to do this after the installation.
> > That way you wouldn't have to require Exec['install_rietveld'] below in the
> > service resource.
> This only makes the symlink, the service it self need the install. It's just
> shifting the need for the install from the service to the file. And for me it
> was more logical to put it by the service. But if you think it would be more
> belong here, I can put the dependency here.

Well, I think /etc/init.d/rietveld logically depends on both Rietveld and
/etc/init/rietveld.conf. So I'd either require both here or both below with the
service. Don't have a strong opinion on what'd make more sense.

http://codereview.adblockplus.org/5459448122310656/diff/5629499534213120/modu...
modules/rietveld/manifests/init.pp:75: command => "echo \"from django.db import
DEFAULT_DB_ALIAS as database; from django.contrib.auth.models import User;
User.objects.db_manager(database).create_superuser('rietveld',
'rietveld@example.org', '${admin_password}')\" | ./manage.py shell",
On 2014/02/07 17:17:21, christian wrote:
> On 2014/02/07 15:27:36, Felix H. Dahlke wrote:
> > I suppose it's fine to just have a single admin account for now, although
> shared
> > accounts are generally a bit of a pain. But it should probably be called
> "admin"
> > and definitely have "admins@adblockplus.org" as its email address.
> > 
> > Also, can't we do this in settings.py?
> 
> I found it easier to just make this one account and enter the rest of the
> accounts via the webinterface, we only need to do this once. And we cannot do
> this via settings.py, because we have no oauth in this version, if i remember
> correctly.

Ah, I see. Yeah sure, we only need to create the admin account(s) via Puppet.

http://codereview.adblockplus.org/5459448122310656/diff/5685265389584384/mani...
File manifests/codereview.pp (right):

http://codereview.adblockplus.org/5459448122310656/diff/5685265389584384/mani...
manifests/codereview.pp:4: class {'rietveld':
Still the wrong indentation here, should be 2 spaces.

http://codereview.adblockplus.org/5459448122310656/diff/5685265389584384/mani...
File manifests/monitoringserver.pp (right):

http://codereview.adblockplus.org/5459448122310656/diff/5685265389584384/mani...
manifests/monitoringserver.pp:83: nagios_host {'codereview.adblockplus.org': use
=> 'generic-host'}
Should be codereview1 here as well. Also, we should add it to 'http-servers'
below.

http://codereview.adblockplus.org/5459448122310656/diff/5685265389584384/mani...
File manifests/nodes.pp (right):

http://codereview.adblockplus.org/5459448122310656/diff/5685265389584384/mani...
manifests/nodes.pp:10: import 'codereview.pp'
Should be 'codereviewserver.pp', in line with the others.

christian

I add the Changes and add a trello board in the Doing section, you can ...

Feb. 10, 2014, 9:05 a.m. (2014-02-10 09:05:56 UTC) #5

Felix Dahlke

Looks to me like you missed two comments: http://codereview.adblockplus.org/5459448122310656/diff/5629499534213120/modules/rietveld/manifests/init.pp#newcode61 http://codereview.adblockplus.org/5459448122310656/diff/5629499534213120/modules/rietveld/manifests/init.pp#newcode65

Feb. 12, 2014, 11:45 a.m. (2014-02-12 11:45:36 UTC) #6

christian

Sorry, totally overlooked this. http://codereview.adblockplus.org/5459448122310656/diff/5629499534213120/modules/rietveld/manifests/init.pp File modules/rietveld/manifests/init.pp (right): http://codereview.adblockplus.org/5459448122310656/diff/5629499534213120/modules/rietveld/manifests/init.pp#newcode61 modules/rietveld/manifests/init.pp:61: #work around puppets limitation towards ...

Feb. 12, 2014, 2:14 p.m. (2014-02-12 14:14:26 UTC) #7

Sorry, totally overlooked this.

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

http://codereview.adblockplus.org/5459448122310656/diff/5629499534213120/modu...
modules/rietveld/manifests/init.pp:61: #work around puppets limitation towards
upstart
On 2014/02/07 20:09:45, Felix H. Dahlke wrote:
> On 2014/02/07 17:17:21, christian wrote:
> > On 2014/02/07 15:27:36, Felix H. Dahlke wrote:
> > > This is not really clear, what particular limitation is there with upstart
> and
> > > Puppet that causes us to fall back on init scripts here? Any particular
bug
> > you
> > > can link to?
> > It is simple, puppet is not aware of upstart so it tries to start up via
> init.d,
> > so I use this method to not to include maybe a buggy third party method to
use
> > the upstart script. I have not found a particular bug, but some things in
> their
> > tracker:
> >
>
https://tickets.puppetlabs.com/browse/PUP-876?jql=text%252520%257E%252520%252...
> 
> If Puppet can't do Upstart at all, I don't think this deserves a comment at
all
> actually. It's to be expected then.

Done.

http://codereview.adblockplus.org/5459448122310656/diff/5629499534213120/modu...
modules/rietveld/manifests/init.pp:65: require =>
File['/etc/init/rietveld.conf']
On 2014/02/07 20:09:45, Felix H. Dahlke wrote:
> On 2014/02/07 17:17:21, christian wrote:
> > On 2014/02/07 15:27:36, Felix H. Dahlke wrote:
> > > Shouldn't this require Exec['install_rietveld']? Not requiring anything
> would
> > > also be fine, but I think it'd make sense to do this after the
installation.
> > > That way you wouldn't have to require Exec['install_rietveld'] below in
the
> > > service resource.
> > This only makes the symlink, the service it self need the install. It's just
> > shifting the need for the install from the service to the file. And for me
it
> > was more logical to put it by the service. But if you think it would be more
> > belong here, I can put the dependency here.
> 
> Well, I think /etc/init.d/rietveld logically depends on both Rietveld and
> /etc/init/rietveld.conf. So I'd either require both here or both below with
the
> service. Don't have a strong opinion on what'd make more sense.

Done.

Felix Dahlke

http://codereview.adblockplus.org/5459448122310656/diff/5738600293466112/modules/rietveld/manifests/init.pp File modules/rietveld/manifests/init.pp (right): http://codereview.adblockplus.org/5459448122310656/diff/5738600293466112/modules/rietveld/manifests/init.pp#newcode72 modules/rietveld/manifests/init.pp:72: require => [Package['gunicorn'], File['/etc/init.d/rietveld'], Exec['install_rietveld']] Now the Exec['install_rietveld'] here ...

Feb. 12, 2014, 4:40 p.m. (2014-02-12 16:40:15 UTC) #8

christian

Done (Hopefully) http://codereview.adblockplus.org/5459448122310656/diff/5738600293466112/modules/rietveld/manifests/init.pp File modules/rietveld/manifests/init.pp (right): http://codereview.adblockplus.org/5459448122310656/diff/5738600293466112/modules/rietveld/manifests/init.pp#newcode72 modules/rietveld/manifests/init.pp:72: require => [Package['gunicorn'], File['/etc/init.d/rietveld'], Exec['install_rietveld']] On 2014/02/12 ...

Feb. 12, 2014, 4:45 p.m. (2014-02-12 16:45:31 UTC) #9

LGTM! :)

Expand All Messages | Collapse All Messages