Concept for Trac as issue tracker.

Concept for Trac as issue tracker.

[Wladimir Palant, 2014-03-03 08:44:31]

http://codereview.adblockplus.org/5777328449781760/diff/5629499534213120/modu...
File modules/trac/files/permissions.csv (right):

http://codereview.adblockplus.org/5777328449781760/diff/5629499534213120/modu...
modules/trac/files/permissions.csv:3:
contributer,TICKET_MODIFY,TICKET_SET_MILESTONE,TICKET_SET_OWNER,TICKET_EDIT_KEYWORDS,TICKET_SET_PRIORITY
Typo here: contributor

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

http://codereview.adblockplus.org/5777328449781760/diff/5629499534213120/modu...
modules/trac/manifests/init.pp:2: package
{['python-mysqldb','python-setuptools','subversion']: ensure => present}
Style nit: Could you please put |ensure => present| on the next line, like we do
it everywhere?

http://codereview.adblockplus.org/5777328449781760/diff/5629499534213120/modu...
modules/trac/manifests/init.pp:36: require => Mysql::Db['trac']
Please add charset => 'utf8 COLLATE utf8_bin' to the mysql::db macro above
instead of running raw SQL here.

http://codereview.adblockplus.org/5777328449781760/diff/5629499534213120/modu...
modules/trac/manifests/init.pp:40: command => "easy_install Trac==1.0",
We might want to use the python-pip package and |pip install Trac==1.0| instead
- pip seems to be generally more flexible than setuptools.

http://codereview.adblockplus.org/5777328449781760/diff/5629499534213120/modu...
modules/trac/manifests/init.pp:44: unless => "which tracd"
We might prefer something like "python -c 'import
trac,sys;sys.exit(trac.__version__ != \"1.0\")'" here - this will allow
upgrading to a newer Trac version in future. Also, this approach will work for
other modules as well.

http://codereview.adblockplus.org/5777328449781760/diff/5629499534213120/modu...
modules/trac/manifests/init.pp:59: Exec['trac_env']]
This doesn't really depend on the trac enviroment, rather on
Package['python-setuptools']. Same for the other modules below.

Also, we need to check here whether the right module version is already
installed, similarly to how we do it for trac.

http://codereview.adblockplus.org/5777328449781760/diff/5629499534213120/modu...
modules/trac/manifests/init.pp:105: command => "tracd --port 8088 -d -s
/home/trac/environment",
This needs to run as a service, otherwise it won't restart on reboot for
example. This should be best done via FCGI - you can use spawn-fcgi::pool class
for that (see modules/web/manifests/server.pp for an example). This will also
allow using a Unix domain socket instead of a TCP port.

http://codereview.adblockplus.org/5777328449781760/diff/5629499534213120/modu...
modules/trac/manifests/init.pp:115: command => "trac-admin
/home/trac/environment upgrade",
Shouldn't we rather initialize the environment only after all the modules are
installed?

http://codereview.adblockplus.org/5777328449781760/diff/5629499534213120/modu...
modules/trac/manifests/init.pp:131: command => "trac-admin
/home/trac/environment permission import /home/trac/permissions.csv",
I don't think that we want to run that every time we provision the server.
Probably best to merge it with the environment initialization (trac-admin
initenv && trac-admin permission import)

http://codereview.adblockplus.org/5777328449781760/diff/5629499534213120/modu...
File modules/trac/templates/trac.ini.erb (right):

http://codereview.adblockplus.org/5777328449781760/diff/5629499534213120/modu...
modules/trac/templates/trac.ini.erb:83: admin = philip@hill-net.com
admins@adblockplus.org?

[Philip Hill, 2014-03-06 14:50:20]

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

http://codereview.adblockplus.org/5777328449781760/diff/5629499534213120/modu...
modules/trac/manifests/init.pp:115: command => "trac-admin
/home/trac/environment upgrade",
On 2014/03/03 08:44:31, Wladimir Palant wrote:
> Shouldn't we rather initialize the environment only after all the modules are
> installed?
As far as I unterstood, installing Plugins requires an alreaey set up
environment.

[Wladimir Palant, 2014-03-06 16:01:24]

Not sure why but my instance cannot persist blocked by/blocking fields. Does
that work for you?

And a minor annoyance: if priority isn't set it, marking the issue as blocking
another changes priority to "blocker" which makes no sense IMHO. I hope this
behavior can be disabled.

http://codereview.adblockplus.org/5777328449781760/diff/5154321532452864/mani...
File manifests/webserver.pp (right):

http://codereview.adblockplus.org/5777328449781760/diff/5154321532452864/mani...
manifests/webserver.pp:23: ',
I'd rather prefer you remove this unrelated change. Whether we should indent
content that goes into a file (along with that indentation) is really
questionable.

http://codereview.adblockplus.org/5777328449781760/diff/5154321532452864/modu...
File modules/trac/manifests/init.pp (right):

http://codereview.adblockplus.org/5777328449781760/diff/5154321532452864/modu...
modules/trac/manifests/init.pp:88: }
Add this to the list of packages on line 4?

[Wladimir Palant, 2014-03-06 17:57:40]

LGTM (the issues with the dependencies plugin can still be resolved later)