Issue 5735669590654976: #753 - set up an order system to let eyeo employees file order requests (Closed)
|
Created:
July 15, 2014, 2:47 p.m. by mathias Modified:
Aug. 7, 2014, 2:36 p.m. Reviewers:
Wladimir Palant CC:
Philip Hill Visibility:
Public. |
DescriptionSee https://issues.adblockplus.org/ticket/753
Patch Set 1 #Patch Set 2 : #753 - set up an order system to let eyeo employees file order requests #Patch Set 3 : #753 - set up an order system to let eyeo employees file order requests #
Total comments: 34
Patch Set 4 : #753 - set up an order system to let eyeo employees file order requests #Patch Set 5 : #753 - set up an order system to let eyeo employees file order requests #Patch Set 6 : #753 - set up an order system to let eyeo employees file order requests #
Total comments: 27
Patch Set 7 : #753 - set up an order system to let eyeo employees file order requests #
Total comments: 7
Patch Set 8 : #753 - set up an order system to let eyeo employees file order requests #Patch Set 9 : #753 - set up an order system to let eyeo employees file order requests #
MessagesTotal messages: 18
http://codereview.adblockplus.org/5735669590654976/diff/5750085036015616/mani... File manifests/issuesserver.pp (right): http://codereview.adblockplus.org/5735669590654976/diff/5750085036015616/mani... manifests/issuesserver.pp:43: unless => "mysql -utrac -p'${private::trac::database_password}' trac_orders --execute 'SHOW CREATE TABLE auth_cookie' | grep FEDERATED", This is quite complex, and there are lots of warning referring to federated MySQL tables (performance, security). Since both instances run on the same server, what about: CREATE VIEW trac_orders.auth_cookie AS SELECT * FROM trac.auth_cookie; This view should be updatable, won't it work as well? http://codereview.adblockplus.org/5735669590654976/diff/5750085036015616/mani... manifests/issuesserver.pp:53: # registration form (and process) in one project only. Why do we want this table to be synced (rather infrequently) rather than shared? http://codereview.adblockplus.org/5735669590654976/diff/5750085036015616/modu... File modules/trac/files/order-permissions.csv (right): http://codereview.adblockplus.org/5735669590654976/diff/5750085036015616/modu... modules/trac/files/order-permissions.csv:3: staff,TICKET_APPEND,TICKET_CREATE,TICKET_VIEW,TICKET_VIEW_SELF,TIMELINE_VIEW SEARCH_VIEW and REPORT_VIEW? http://codereview.adblockplus.org/5735669590654976/diff/5750085036015616/modu... modules/trac/files/order-permissions.csv:4: reviewers,REPORT_ADMIN,TAGS_ADMIN,TICKET_ACCEPT,TICKET_ADMIN,TICKET_BATCH_MODIFY,TICKET_INVOICE_RECEIVED,TICKET_MANUAL_PAYMENT_DONE,TICKET_MANUAL_PAYMENT_REQUIRED,TICKET_ORDER_URL,TICKET_PAYMENT_TYPE,TICKET_TRACKING_URL,TIMELINE_VIEW No need to repeat TIMELINE_VIEW everywhere, staff group already has that permission. http://codereview.adblockplus.org/5735669590654976/diff/5750085036015616/modu... modules/trac/files/order-permissions.csv:14: admin,TRAC_ADMIN This is a group, not a user. http://codereview.adblockplus.org/5735669590654976/diff/5750085036015616/modu... modules/trac/files/order-permissions.csv:16: Testinger,orderstaff I think we decided to always assign users to groups in the administration UI, keeping this csv file in sync isn't practicable since reapplying it has issues (it only adds permissions but doesn't remove them). http://codereview.adblockplus.org/5735669590654976/diff/5750085036015616/modu... File modules/trac/files/site.conf (right): http://codereview.adblockplus.org/5735669590654976/diff/5750085036015616/modu... modules/trac/files/site.conf:10: location /chrome Please keep the trailing slash for the location here and add one for the locations below. Nginx does prefix matches so the location as given here will also match https://issues.adblockplus.org/chrome-foo which is definitely not what we meant. http://codereview.adblockplus.org/5735669590654976/diff/5750085036015616/modu... modules/trac/files/site.conf:18: fastcgi_split_path_info ^/(orders)(.*)$; ^(/orders)(.*)$ please - the slash is part of the "script URL" here. http://codereview.adblockplus.org/5735669590654976/diff/5750085036015616/modu... modules/trac/files/site.conf:20: fastcgi_param SCRIPT_NAME /orders; This should be unnecessary if the issue above is fixed. http://codereview.adblockplus.org/5735669590654976/diff/5750085036015616/modu... File modules/trac/manifests/init.pp (right): http://codereview.adblockplus.org/5735669590654976/diff/5750085036015616/modu... modules/trac/manifests/init.pp:5: $permissions = "puppet:///modules/trac/permissions.csv", Shouldn't this be an instance parameter? While this file needs to be applied manually, we probably want to have both versions installed in obvious locations. http://codereview.adblockplus.org/5735669590654976/diff/5750085036015616/modu... modules/trac/manifests/init.pp:15: source => 'puppet:///modules/trac/site.conf', We have the Trac instances hardcoded in this configuration file which is problematic - adding an instance doesn't merely require changing manifests/issuesserver.pp but also a non-obvious change to site.conf in the module here. The best solution I can think of is a $locations parameter to the trac class, the server manifest would set it to ['/', '/orders/']. You can then use a template here to generate the configuration file. While this still duplicates the information in trac::instance calls (don't see a way to avoid it), it should still be a significant improvement. http://codereview.adblockplus.org/5735669590654976/diff/5750085036015616/modu... modules/trac/manifests/init.pp:126: $database_password = $private::trac::database_password, Given that the password is per user and not per database, it makes no sense to pass it in here - unless you pass in the corresponding user name as well. But I'd rather not have this parameter at all. http://codereview.adblockplus.org/5735669590654976/diff/5750085036015616/modu... modules/trac/manifests/init.pp:127: $theme = 'puppet:///modules/trac/theme.css') { Why a full path for the theme yet a relative one for the logo? This should be consistent, I guess a relative path will do for both. http://codereview.adblockplus.org/5735669590654976/diff/5750085036015616/modu... modules/trac/manifests/init.pp:140: command => "trac-admin /home/trac/$environment initenv \"$description\" mysql://trac:${database_password}@localhost:3306/$database", I think we should do at least some minimal escaping here, not sure about others but for the $description parameter this definitely doesn't look safe. Something along these lines: $escaped_description = regsubst($description, '[\'\\]', '\\\1', 'G') ... command => "... '$description' ...", http://codereview.adblockplus.org/5735669590654976/diff/5750085036015616/modu... modules/trac/manifests/init.pp:183: command => "trac-admin /home/trac/$environment deploy /home/trac/htdocs && fromdos /home/trac/htdocs/cgi-bin/trac.fcgi && chmod 755 /home/trac/htdocs/cgi-bin/trac.fcgi", I wonder whether it is a good idea to deploy two Trac instances with slightly different files to the same directory. This will fail once these instances start using different themes, and it apparently failed already for the logos which is why you had to choose different file names for them. How about using different deploy directories for the instances? http://codereview.adblockplus.org/5735669590654976/diff/5750085036015616/modu... modules/trac/manifests/init.pp:204: TRAC_ENV=/home/trac/$environment \$SCRIPT \"\$@\" This hack is only necessary because of reusing the same deployment directory, see above. http://codereview.adblockplus.org/5735669590654976/diff/5750085036015616/modu... File modules/trac/templates/orders.ini.erb (right): http://codereview.adblockplus.org/5735669590654976/diff/5750085036015616/modu... modules/trac/templates/orders.ini.erb:1: # -*- coding: utf-8 -*- Wasn't the idea to have a "common" trac.ini part that both configurations would include? I really don't like doing diffs in my head.
http://codereview.adblockplus.org/5735669590654976/diff/5750085036015616/mani... File manifests/issuesserver.pp (right): http://codereview.adblockplus.org/5735669590654976/diff/5750085036015616/mani... manifests/issuesserver.pp:43: unless => "mysql -utrac -p'${private::trac::database_password}' trac_orders --execute 'SHOW CREATE TABLE auth_cookie' | grep FEDERATED", On 2014/07/22 13:52:24, Wladimir Palant wrote: > This is quite complex, and there are lots of warning referring to federated > MySQL tables (performance, security). Since both instances run on the same > server, what about: > > CREATE VIEW trac_orders.auth_cookie AS SELECT * FROM trac.auth_cookie; > > This view should be updatable, won't it work as well? Sure, it does. Yet it does not allow to move to a distinct server some day (which was intended before). However, since that's not an issue any more, I'll update it accordingly. http://codereview.adblockplus.org/5735669590654976/diff/5750085036015616/mani... manifests/issuesserver.pp:53: # registration form (and process) in one project only. On 2014/07/22 13:52:24, Wladimir Palant wrote: > Why do we want this table to be synced (rather infrequently) rather than shared? Because the Trac software also stores other information in this fashion (see the NAME IN() clause, which restricts to those pieces we need). One example is search criteria, which may include ticket types and the like - those are not equal in the Trac instances. http://codereview.adblockplus.org/5735669590654976/diff/5750085036015616/modu... File modules/trac/files/order-permissions.csv (right): http://codereview.adblockplus.org/5735669590654976/diff/5750085036015616/modu... modules/trac/files/order-permissions.csv:3: staff,TICKET_APPEND,TICKET_CREATE,TICKET_VIEW,TICKET_VIEW_SELF,TIMELINE_VIEW On 2014/07/22 13:52:24, Wladimir Palant wrote: > SEARCH_VIEW and REPORT_VIEW? This entire file has been taken directly from the attachments in the Trac ticket. I have to discuss your points with Phill, especially because I can only guess the intentions behind the permission assignments in this file. http://codereview.adblockplus.org/5735669590654976/diff/5750085036015616/modu... modules/trac/files/order-permissions.csv:4: reviewers,REPORT_ADMIN,TAGS_ADMIN,TICKET_ACCEPT,TICKET_ADMIN,TICKET_BATCH_MODIFY,TICKET_INVOICE_RECEIVED,TICKET_MANUAL_PAYMENT_DONE,TICKET_MANUAL_PAYMENT_REQUIRED,TICKET_ORDER_URL,TICKET_PAYMENT_TYPE,TICKET_TRACKING_URL,TIMELINE_VIEW On 2014/07/22 13:52:24, Wladimir Palant wrote: > No need to repeat TIMELINE_VIEW everywhere, staff group already has that > permission. See my reply/comment above. http://codereview.adblockplus.org/5735669590654976/diff/5750085036015616/modu... modules/trac/files/order-permissions.csv:14: admin,TRAC_ADMIN On 2014/07/22 13:52:24, Wladimir Palant wrote: > This is a group, not a user. AFAIK Trac does not differ, at least not in case of the permissions. http://codereview.adblockplus.org/5735669590654976/diff/5750085036015616/modu... modules/trac/files/order-permissions.csv:16: Testinger,orderstaff On 2014/07/22 13:52:24, Wladimir Palant wrote: > I think we decided to always assign users to groups in the administration UI, > keeping this csv file in sync isn't practicable since reapplying it has issues > (it only adds permissions but doesn't remove them). Agreed. Yet this is still not optimal, at least not without making the production-data available for testing purposes and defining an alternative (vagrant/puppet) setup mechanism based on that data. http://codereview.adblockplus.org/5735669590654976/diff/5750085036015616/modu... File modules/trac/files/site.conf (right): http://codereview.adblockplus.org/5735669590654976/diff/5750085036015616/modu... modules/trac/files/site.conf:10: location /chrome On 2014/07/22 13:52:24, Wladimir Palant wrote: > Please keep the trailing slash for the location here and add one for the > locations below. Nginx does prefix matches so the location as given here will > also match https://issues.adblockplus.org/chrome-foo which is definitely not > what we meant. I tried that, it does not seem to work with a dual setup. Anyway, I'll try again. http://codereview.adblockplus.org/5735669590654976/diff/5750085036015616/modu... File modules/trac/manifests/init.pp (right): http://codereview.adblockplus.org/5735669590654976/diff/5750085036015616/modu... modules/trac/manifests/init.pp:5: $permissions = "puppet:///modules/trac/permissions.csv", On 2014/07/22 13:52:24, Wladimir Palant wrote: > Shouldn't this be an instance parameter? While this file needs to be applied > manually, we probably want to have both versions installed in obvious locations. Sure, agreed. http://codereview.adblockplus.org/5735669590654976/diff/5750085036015616/modu... modules/trac/manifests/init.pp:15: source => 'puppet:///modules/trac/site.conf', On 2014/07/22 13:52:24, Wladimir Palant wrote: > We have the Trac instances hardcoded in this configuration file which is > problematic - adding an instance doesn't merely require changing > manifests/issuesserver.pp but also a non-obvious change to site.conf in the > module here. The best solution I can think of is a $locations parameter to the > trac class, the server manifest would set it to ['/', '/orders/']. You can then > use a template here to generate the configuration file. While this still > duplicates the information in trac::instance calls (don't see a way to avoid > it), it should still be a significant improvement. What about a directory to write the necessary fragments to and then include them in the site.conf? E.g. /home/trac/nginx-site.d or something like that. http://codereview.adblockplus.org/5735669590654976/diff/5750085036015616/modu... modules/trac/manifests/init.pp:126: $database_password = $private::trac::database_password, On 2014/07/22 13:52:24, Wladimir Palant wrote: > Given that the password is per user and not per database, it makes no sense to > pass it in here - unless you pass in the corresponding user name as well. But > I'd rather not have this parameter at all. Well, assuming that we keep the shared database setup for a while, I'd go for the removal option. http://codereview.adblockplus.org/5735669590654976/diff/5750085036015616/modu... modules/trac/manifests/init.pp:127: $theme = 'puppet:///modules/trac/theme.css') { On 2014/07/22 13:52:24, Wladimir Palant wrote: > Why a full path for the theme yet a relative one for the logo? This should be > consistent, I guess a relative path will do for both. It's a bit more complicated; the $logo variable is re-used in the Trac configuration template.. But I see what I can improve here. http://codereview.adblockplus.org/5735669590654976/diff/5750085036015616/modu... modules/trac/manifests/init.pp:140: command => "trac-admin /home/trac/$environment initenv \"$description\" mysql://trac:${database_password}@localhost:3306/$database", On 2014/07/22 13:52:24, Wladimir Palant wrote: > I think we should do at least some minimal escaping here, not sure about others > but for the $description parameter this definitely doesn't look safe. Something > along these lines: > > $escaped_description = regsubst($description, '[\'\\]', '\\\1', 'G') > ... > command => "... '$description' ...", Just remembered this one: http://docs.puppetlabs.com/references/latest/function.html#shellquote - it should be an even better solution. http://codereview.adblockplus.org/5735669590654976/diff/5750085036015616/modu... modules/trac/manifests/init.pp:183: command => "trac-admin /home/trac/$environment deploy /home/trac/htdocs && fromdos /home/trac/htdocs/cgi-bin/trac.fcgi && chmod 755 /home/trac/htdocs/cgi-bin/trac.fcgi", On 2014/07/22 13:52:24, Wladimir Palant wrote: > I wonder whether it is a good idea to deploy two Trac instances with slightly > different files to the same directory. This will fail once these instances start > using different themes, and it apparently failed already for the logos which is > why you had to choose different file names for them. How about using different > deploy directories for the instances? Makes sense, yet I'm wondering why so many examples for shared setup did that the above way. I'll try it out, let's hope that this does not introduce more issues we aren't aware of yet ;-) http://codereview.adblockplus.org/5735669590654976/diff/5750085036015616/modu... modules/trac/manifests/init.pp:204: TRAC_ENV=/home/trac/$environment \$SCRIPT \"\$@\" On 2014/07/22 13:52:24, Wladimir Palant wrote: > This hack is only necessary because of reusing the same deployment directory, > see above. Dito, see above.
Changes according to review comments from patch sets 1-3: - Make $permissions a trac::instance parameter - Improve site.conf and FGCI setup - Convert FEDERATED TABLE into VIEW - Remove FEDERATED feature from MySQL server setup - Remove $database_password from trac::instance parameters - Use shellquote() to avoid malformed command syntax - Apply inherited / shared trac.ini - Remove accidential whitespace in otherwise unchanged nodes.pp - Merge upstream (473:8fa270199ca2) into working copy NOTE: The permissions haven't been updated yet. Phil and I will do that together.
A few issues have been found that are fixed with this patch-set already. Below
please find an updated list of changes since patch set 3:
- Make $permissions a trac::instance parameter
- Import current state from intermediate environment
- Import current state from intermediate environment
- Make $permissions a trac::instance parameter
- Improve site.conf and FGCI setup
- Convert FEDERATED TABLE into VIEW
- Remove FEDERATED feature from MySQL server setup
- Remove $database_password from trac::instance parameters
- Use shellquote() to avoid malformed command syntax
- Apply inherited / shared trac.ini
- Remove accidential whitespace in otherwise unchanged nodes.pp
- Include PrivateTicketsPlugin for Trac
- Ensure $fcgi_config_dir being created after Nginx has been installed
- Ensure registration only being possible in the official issue tracker
- Rename target trac_auth_cookie_{federated => view}
- Ensure Ngninx being restarted after ALL Trac setup steps have beem performed
- Update order-permissions.csv according to dev-export
- Blacklist TracTicketTemplate plugin for /orders instance
- Fix split/merge issue in [spam-filter] section of trac.ini
Note that the permissions are now updated according to those defined by Phill in
the test environment, so the former NOTE does not apply any more.
A few issues have been found that are fixed with this patch-set already.
Furthermore, Phil and I have improved the permissions. However, below please
find an updated list of changes since patch set 3:
- Make $permissions a trac::instance parameter
- Improve site.conf and FGCI setup
- Convert FEDERATED TABLE into VIEW
- Remove FEDERATED feature from MySQL server setup
- Remove $database_password from trac::instance parameters
- Use shellquote() to avoid malformed command syntax
- Apply inherited / shared trac.ini
- Remove accidential whitespace in otherwise unchanged nodes.pp
- Include PrivateTicketsPlugin for Trac
- Ensure $fcgi_config_dir being created after Nginx has been installed
- Ensure registration only being possible in the official issue tracker
- Rename target trac_auth_cookie_{federated => view}
- Ensure Ngninx being restarted after ALL Trac setup steps have beem performed
- Update order-permissions.csv according to dev-export
- Blacklist TracTicketTemplate plugin for /orders instance
- Fix split/merge issue in [spam-filter] section of trac.ini
- Extend *permissions.csv files by TAGS_VIEW
- Add order-staff permissions: TICKET_MODIFY,TICKET_EDIT_CC
- Bind various ticket edit permissions to TICKET_ADMIN instead of TICKET_MODIFY
http://codereview.adblockplus.org/5735669590654976/diff/5750085036015616/modu... File modules/trac/files/order-permissions.csv (right): http://codereview.adblockplus.org/5735669590654976/diff/5750085036015616/modu... modules/trac/files/order-permissions.csv:14: admin,TRAC_ADMIN On 2014/07/24 16:36:49, matze wrote: > AFAIK Trac does not differ, at least not in case of the permissions. No, it doesn't - but the permissions for the groups are defined further above. Anyway, doesn't matter any more with your recent changes. http://codereview.adblockplus.org/5735669590654976/diff/5750085036015616/modu... File modules/trac/manifests/init.pp (right): http://codereview.adblockplus.org/5735669590654976/diff/5750085036015616/modu... modules/trac/manifests/init.pp:15: source => 'puppet:///modules/trac/site.conf', On 2014/07/24 16:36:49, matze wrote: > What about a directory to write the necessary fragments to and then include them > in the site.conf? E.g. /home/trac/nginx-site.d or something like that. Good solution, yes. http://codereview.adblockplus.org/5735669590654976/diff/5750085036015616/modu... modules/trac/manifests/init.pp:140: command => "trac-admin /home/trac/$environment initenv \"$description\" mysql://trac:${database_password}@localhost:3306/$database", On 2014/07/24 16:36:49, matze wrote: > Just remembered this one: > http://docs.puppetlabs.com/references/latest/function.html#shellquote - it > should be an even better solution. Indeed, didn't know it existed. We should use that consistently whenever we are generating shell commands. http://codereview.adblockplus.org/5735669590654976/diff/5743114304094208/mani... File manifests/issuesserver.pp (right): http://codereview.adblockplus.org/5735669590654976/diff/5743114304094208/mani... manifests/issuesserver.pp:39: RENAME TABLE auth_cookie TO auth_cookie_original, auth_cookie_view TO auth_cookie;'", Why the renaming? We don't want that empty table. So the following should do: DROP TABLE IF EXISTS auth_cookie; CREATE VIEW auth_cookie AS SELECT * FROM trac.auth_cookie; http://codereview.adblockplus.org/5735669590654976/diff/5743114304094208/mani... manifests/issuesserver.pp:41: SHOW CREATE TABLE auth_cookie' | grep VIEW", How about: unless => "mysql -utrac -p'${private::trac::database_password}' trac_orders --execute 'SHOW CREATE VIEW auth_cookie'" No need to grep, the exit code of the mysql command seems to be sufficient. http://codereview.adblockplus.org/5735669590654976/diff/5743114304094208/modu... File modules/discourse/manifests/init.pp (left): http://codereview.adblockplus.org/5735669590654976/diff/5743114304094208/modu... modules/discourse/manifests/init.pp:264: value => '50', I don't think you intended this change here. http://codereview.adblockplus.org/5735669590654976/diff/5743114304094208/modu... File modules/trac/manifests/init.pp (right): http://codereview.adblockplus.org/5735669590654976/diff/5743114304094208/modu... modules/trac/manifests/init.pp:5: $fcgi_config_dir = '/etc/nginx/trac.d', This path is hardcoded in the site.conf, no point having it as a configurable parameter then. http://codereview.adblockplus.org/5735669590654976/diff/5743114304094208/modu... modules/trac/manifests/init.pp:133: $environment = 'environment', It's probably better to just set environment based on name and not have it configurable, same as with deployment directories: $environment = "environment-$name" http://codereview.adblockplus.org/5735669590654976/diff/5743114304094208/modu... modules/trac/manifests/init.pp:155: } I think what you really want here is removing trailing slashes: $location_base = regsubst($location, '/+$', '') http://codereview.adblockplus.org/5735669590654976/diff/5743114304094208/modu... modules/trac/manifests/init.pp:174: }", I'd rather go with a template here, having nginx configuration files inline is ugly (also because of the escaping required). http://codereview.adblockplus.org/5735669590654976/diff/5743114304094208/modu... modules/trac/manifests/init.pp:176: } This needs: notify => Service['nginx'] Otherwise nginx might not pick up the changes to configuration files. http://codereview.adblockplus.org/5735669590654976/diff/5743114304094208/modu... modules/trac/manifests/init.pp:242: file {"/home/trac/htdocs-$name/htdocs/common/$logo": Feel free to always name that file logo.png (naming conflict has been resolved by separating deployment directories), regardless of the source file name. Then this entry in trac.ini will be static and you will be able to use an absolute path for the $logo parameter, consistent with $theme. http://codereview.adblockplus.org/5735669590654976/diff/5743114304094208/modu... modules/trac/manifests/init.pp:249: spawn-fcgi::pool {"${name}d": I think "tracd_$name" was a better name for the service... http://codereview.adblockplus.org/5735669590654976/diff/5743114304094208/modu... modules/trac/manifests/init.pp:257: notify => Service['nginx'], This one is pointless - nginx doesn't care whether the FCGI process is running, it will simply produce "502 Bad Gateway" if it isn't. http://codereview.adblockplus.org/5735669590654976/diff/5743114304094208/modu... File modules/trac/templates/orders.ini.erb (right): http://codereview.adblockplus.org/5735669590654976/diff/5743114304094208/modu... modules/trac/templates/orders.ini.erb:60: debug_sql = True Debugging left-over? I don't think we want this in production.
- Make $permissions a trac::instance parameter
- Improve site.conf and FGCI setup
- Convert FEDERATED TABLE into VIEW
- Remove FEDERATED feature from MySQL server setup
- Remove $database_password from trac::instance parameters
- Use shellquote() to avoid malformed command syntax
- Apply inherited / shared trac.ini
- Remove accidential whitespace in otherwise unchanged nodes.pp
- Include PrivateTicketsPlugin for Trac
- Ensure $fcgi_config_dir being created after Nginx has been installed
- Ensure registration only being possible in the official issue tracker
- Rename target trac_auth_cookie_{federated => view}
- Ensure Ngninx being restarted after ALL Trac setup steps have beem performed
- Update order-permissions.csv according to dev-export
- Blacklist TracTicketTemplate plugin for /orders instance
- Fix split/merge issue in [spam-filter] section of trac.ini
- Extend *permissions.csv files by TAGS_VIEW
- Add order-staff permissions: TICKET_MODIFY,TICKET_EDIT_CC
- Bind various ticket edit permissions to TICKET_ADMIN instead of TICKET_MODIFY
- Remove unneccessary grep(1) from trac_auth_cookie_view/unless directive
- Improve computation of $location_base using regsubst()
- Notify nginx(8) when a configuration fragment changes
- Unify the logo image location
- Remove hard-coded site.conf
- Remove $environment parameter from trac::instance
- Use template() to create nginx/fcgi configuration fragments
- Various minor improvements and bug-avoiding hacks
http://codereview.adblockplus.org/5735669590654976/diff/5735735550279680/mani... File manifests/issuesserver.pp (right): http://codereview.adblockplus.org/5735669590654976/diff/5735735550279680/mani... manifests/issuesserver.pp:37: RENAME TABLE auth_cookie TO auth_cookie_original, auth_cookie_view TO auth_cookie;'", Not addressed yet: On 2014/07/31 09:46:53, Wladimir Palant wrote: > Why the renaming? We don't want that empty table. So the following should do: > > DROP TABLE IF EXISTS auth_cookie; > CREATE VIEW auth_cookie AS SELECT * FROM trac.auth_cookie; http://codereview.adblockplus.org/5735669590654976/diff/5735735550279680/modu... File modules/trac/manifests/init.pp (right): http://codereview.adblockplus.org/5735669590654976/diff/5735735550279680/modu... modules/trac/manifests/init.pp:243: notify => Service['nginx'], Not addressed: On 2014/07/31 09:46:53, Wladimir Palant wrote: > This one is pointless - nginx doesn't care whether the FCGI process is running, > it will simply produce "502 Bad Gateway" if it isn't. http://codereview.adblockplus.org/5735669590654976/diff/5735735550279680/modu... File modules/trac/templates/fcgi.conf.erb (right): http://codereview.adblockplus.org/5735669590654976/diff/5735735550279680/modu... modules/trac/templates/fcgi.conf.erb:4: fastcgi_split_path_info ^(<%= @location_base %>)(.*)\$; Remove the backslash before $? http://codereview.adblockplus.org/5735669590654976/diff/5735735550279680/modu... File modules/trac/templates/orders.ini.erb (right): http://codereview.adblockplus.org/5735669590654976/diff/5735735550279680/modu... modules/trac/templates/orders.ini.erb:41: handedout.permissions = TICKET_ADMIN Is removing of the receiptreceived state intentional? http://codereview.adblockplus.org/5735669590654976/diff/5735735550279680/modu... modules/trac/templates/orders.ini.erb:58: debug_sql = True Not addressed: On 2014/07/31 09:46:53, Wladimir Palant wrote: > Debugging left-over? I don't think we want this in production.
http://codereview.adblockplus.org/5735669590654976/diff/5743114304094208/mani... File manifests/issuesserver.pp (right): http://codereview.adblockplus.org/5735669590654976/diff/5743114304094208/mani... manifests/issuesserver.pp:39: RENAME TABLE auth_cookie TO auth_cookie_original, auth_cookie_view TO auth_cookie;'", On 2014/07/31 09:46:53, Wladimir Palant wrote: > Why the renaming? We don't want that empty table. So the following should do: > > DROP TABLE IF EXISTS auth_cookie; > CREATE VIEW auth_cookie AS SELECT * FROM trac.auth_cookie; Well, I still prefer my version: Beside having an implicit (thus cheap) backup, it's also atomic. Therefore, it fails early enough no matter what could go wrong. And the *_original does not hurt anyone - but may provide important structure information for debugging after a future update or something like that. Thus, although I understand that it might be overkill in this particular situation, I do not see any reason to drop this practice - or that table. http://codereview.adblockplus.org/5735669590654976/diff/5743114304094208/mani... manifests/issuesserver.pp:41: SHOW CREATE TABLE auth_cookie' | grep VIEW", On 2014/07/31 09:46:53, Wladimir Palant wrote: > How about: > > unless => "mysql -utrac -p'${private::trac::database_password}' trac_orders > --execute 'SHOW CREATE VIEW auth_cookie'" > > No need to grep, the exit code of the mysql command seems to be sufficient. Agreed, done. http://codereview.adblockplus.org/5735669590654976/diff/5743114304094208/modu... File modules/trac/manifests/init.pp (right): http://codereview.adblockplus.org/5735669590654976/diff/5743114304094208/modu... modules/trac/manifests/init.pp:5: $fcgi_config_dir = '/etc/nginx/trac.d', On 2014/07/31 09:46:53, Wladimir Palant wrote: > This path is hardcoded in the site.conf, no point having it as a configurable > parameter then. Well, since the site.conf is just one line, I removed it in favor of $fcgi_config_dir ;-) - As soon as we may need more directives in there we can convert it into a template. http://codereview.adblockplus.org/5735669590654976/diff/5743114304094208/modu... modules/trac/manifests/init.pp:133: $environment = 'environment', On 2014/07/31 09:46:53, Wladimir Palant wrote: > It's probably better to just set environment based on name and not have it > configurable, same as with deployment directories: > > $environment = "environment-$name" Sure, but there's another but: This will cause the trac-admin/initenv to become a major source of error, requiring a hack (see issueserver.pp). Both versions are somehow ugly, but mine/the former one does not introduce any quirks in the file system. http://codereview.adblockplus.org/5735669590654976/diff/5743114304094208/modu... modules/trac/manifests/init.pp:155: } On 2014/07/31 09:46:53, Wladimir Palant wrote: > I think what you really want here is removing trailing slashes: > > $location_base = regsubst($location, '/+$', '') Makes sense, done. http://codereview.adblockplus.org/5735669590654976/diff/5743114304094208/modu... modules/trac/manifests/init.pp:174: }", On 2014/07/31 09:46:53, Wladimir Palant wrote: > I'd rather go with a template here, having nginx configuration files inline is > ugly (also because of the escaping required). Done. http://codereview.adblockplus.org/5735669590654976/diff/5743114304094208/modu... modules/trac/manifests/init.pp:176: } On 2014/07/31 09:46:53, Wladimir Palant wrote: > This needs: > > notify => Service['nginx'] > > Otherwise nginx might not pick up the changes to configuration files. Done. http://codereview.adblockplus.org/5735669590654976/diff/5743114304094208/modu... modules/trac/manifests/init.pp:242: file {"/home/trac/htdocs-$name/htdocs/common/$logo": On 2014/07/31 09:46:53, Wladimir Palant wrote: > Feel free to always name that file logo.png (naming conflict has been resolved > by separating deployment directories), regardless of the source file name. Then > this entry in trac.ini will be static and you will be able to use an absolute > path for the $logo parameter, consistent with $theme. Done. http://codereview.adblockplus.org/5735669590654976/diff/5743114304094208/modu... modules/trac/manifests/init.pp:249: spawn-fcgi::pool {"${name}d": On 2014/07/31 09:46:53, Wladimir Palant wrote: > I think "tracd_$name" was a better name for the service... Sure, I think so too. But I've had a hard time trying to find a way to tell the spawn-fcgi module that the former "tracd" is gone and replaced by two new instances. It always failed to do anything right (when migrating from the current production setup, a plain installation worked of course), so I figured it may be better not to waste any more time on that (with the current version the old name remains in use). However, after your review I've applied a hack to issueserver.pp - it works now with more verbose pool names, but is still ugly. http://codereview.adblockplus.org/5735669590654976/diff/5743114304094208/modu... modules/trac/manifests/init.pp:257: notify => Service['nginx'], On 2014/07/31 09:46:53, Wladimir Palant wrote: > This one is pointless - nginx doesn't care whether the FCGI process is running, > it will simply produce "502 Bad Gateway" if it isn't. Not quite correct, I believe. It's the file descriptors to the sockets that nginx(8) needs to re-open if it still has a handle on any of them. Otherwise one gets gateway errors although the processes are running. At least that's what we've encountered during the tests. http://codereview.adblockplus.org/5735669590654976/diff/5743114304094208/modu... File modules/trac/templates/orders.ini.erb (right): http://codereview.adblockplus.org/5735669590654976/diff/5743114304094208/modu... modules/trac/templates/orders.ini.erb:60: debug_sql = True On 2014/07/31 09:46:53, Wladimir Palant wrote: > Debugging left-over? I don't think we want this in production. Removed.
http://codereview.adblockplus.org/5735669590654976/diff/5743114304094208/mani... File manifests/issuesserver.pp (right): http://codereview.adblockplus.org/5735669590654976/diff/5743114304094208/mani... manifests/issuesserver.pp:39: RENAME TABLE auth_cookie TO auth_cookie_original, auth_cookie_view TO auth_cookie;'", On 2014/08/01 13:59:46, matze wrote: > Well, I still prefer my version: Beside having an implicit (thus cheap) backup, > it's also atomic. Backup of an empty table that isn't used anywhere? Also, why should we care here whether the operation is atomic? >And the *_original does not hurt anyone - but may provide important > structure information for debugging after a future update or something like > that. I'm generally unhappy about crap left behind just because it might become useful at some point - that's what version control systems are for. I'd rather not waste time in a year or two trying to figure out what this table is good for. http://codereview.adblockplus.org/5735669590654976/diff/5743114304094208/modu... File modules/trac/manifests/init.pp (right): http://codereview.adblockplus.org/5735669590654976/diff/5743114304094208/modu... modules/trac/manifests/init.pp:257: notify => Service['nginx'], On 2014/08/01 13:59:46, matze wrote: > Not quite correct, I believe. We had FCGI processes crash often enough that I know - nginx doesn't need to be reloaded if the process is restared.
http://codereview.adblockplus.org/5735669590654976/diff/5743114304094208/mani... File manifests/issuesserver.pp (right): http://codereview.adblockplus.org/5735669590654976/diff/5743114304094208/mani... manifests/issuesserver.pp:39: RENAME TABLE auth_cookie TO auth_cookie_original, auth_cookie_view TO auth_cookie;'", On 2014/08/01 14:11:13, Wladimir Palant wrote: > On 2014/08/01 13:59:46, matze wrote: > > Well, I still prefer my version: Beside having an implicit (thus cheap) > backup, > > it's also atomic. > > Backup of an empty table that isn't used anywhere? Also, why should we care here > whether the operation is atomic? > > >And the *_original does not hurt anyone - but may provide important > > structure information for debugging after a future update or something like > > that. > > I'm generally unhappy about crap left behind just because it might become useful > at some point - that's what version control systems are for. I'd rather not > waste time in a year or two trying to figure out what this table is good for. Since you prefer it without, I'll adjust it. But, if you're interested,.. as I see it, it's not crap. It's documentation: As long as we do not mirror every external resource (including e.g. trac and APT packages for example), the original structure may change at any time and may cause strange effects in development after future releases. Every debugging developer would be happy if we left the original there. I've been there. A lot. Also; the atomic nature allows that thingy to fail if we do anything wrong (e.g. in the future) before every session is destroyed. I understand that this might be considered overkill in this particular situation. But I'm not used to dropping good practices just because they are not considered an requirement. It feels like an appendix operation without having a problem with the blind gut at all.. http://codereview.adblockplus.org/5735669590654976/diff/5743114304094208/modu... File modules/trac/manifests/init.pp (right): http://codereview.adblockplus.org/5735669590654976/diff/5743114304094208/modu... modules/trac/manifests/init.pp:257: notify => Service['nginx'], On 2014/08/01 14:11:13, Wladimir Palant wrote: > On 2014/08/01 13:59:46, matze wrote: > > Not quite correct, I believe. > > We had FCGI processes crash often enough that I know - nginx doesn't need to be > reloaded if the process is restared. Indeed, Nginx will re-open the descriptor - after it realizes that the handle is invalid, at least. However, since it's only during the computation of the first page view after the provision, I'll remove it. http://codereview.adblockplus.org/5735669590654976/diff/5735735550279680/modu... File modules/trac/templates/orders.ini.erb (right): http://codereview.adblockplus.org/5735669590654976/diff/5735735550279680/modu... modules/trac/templates/orders.ini.erb:41: handedout.permissions = TICKET_ADMIN On 2014/08/01 13:56:08, Wladimir Palant wrote: > Is removing of the receiptreceived state intentional? It is, yes. Phill and Christian do not see any need for that any more. http://codereview.adblockplus.org/5735669590654976/diff/5735735550279680/modu... modules/trac/templates/orders.ini.erb:58: debug_sql = True On 2014/08/01 13:56:08, Wladimir Palant wrote: > Not addressed: > > On 2014/07/31 09:46:53, Wladimir Palant wrote: > > Debugging left-over? I don't think we want this in production. Removed (it was only addressed in the other ini template, still it should have been removed here also).
- Make $permissions a trac::instance parameter
- Improve site.conf and FGCI setup
- Convert FEDERATED TABLE into VIEW
- Remove FEDERATED feature from MySQL server setup
- Remove $database_password from trac::instance parameters
- Use shellquote() to avoid malformed command syntax
- Apply inherited / shared trac.ini
- Remove accidential whitespace in otherwise unchanged nodes.pp
- Include PrivateTicketsPlugin for Trac
- Ensure $fcgi_config_dir being created after Nginx has been installed
- Ensure registration only being possible in the official issue tracker
- Rename target trac_auth_cookie_{federated => view}
- Ensure Ngninx being restarted after ALL Trac setup steps have beem performed
- Update order-permissions.csv according to dev-export
- Blacklist TracTicketTemplate plugin for /orders instance
- Fix split/merge issue in [spam-filter] section of trac.ini
- Extend *permissions.csv files by TAGS_VIEW
- Add order-staff permissions: TICKET_MODIFY,TICKET_EDIT_CC
- Bind various ticket edit permissions to TICKET_ADMIN instead of TICKET_MODIFY
- Remove unneccessary grep(1) from trac_auth_cookie_view/unless directive
- Improve computation of $location_base using regsubst()
- Notify nginx(8) when a configuration fragment changes
- Unify the logo image location
- Remove hard-coded site.conf
- Remove $environment parameter from trac::instance
- Use template() to create nginx/fcgi configuration fragments
- Various minor improvements and bug-avoiding hacks
LGTM
According to Phill's requests during testing, here are a few more minor updates: - Ensure subsequent provisioning to work as intended - Fix module name in PrivateTickesPlugin/onlyif directive - Allow "waiting for payment" as possible state before "received" - Add SEARCH_VIEW privilege to accountants, buyers and reviewers role The provisioning itself has now been tested in the following scenarios: - Vagrant up from scratch to the current patch set - Vagrant up to the current production state, following an update to the current patch set - Either of the above followed by multiple provision runs without prior modification Note that I've contacted the author of the spawn-fcgi module using GitHub, in order to suggest a few improvements that may avoid the dirty hacks in e.g. issueserver.pp in the future. Furthermore, please note that one must update the nginx module before (see #1175; 477:ddbcd39d4cbc).
LGTM 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
