Closed Bug 661425 Opened 9 years ago Closed 9 years ago

Review Firefox Sheriff calendar tool

Categories

(mozilla.org Graveyard :: Webdev, task)

task
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: wenzel, Assigned: tofumatt)

References

()

Details

Please review the Firefox sheriff calendar tool for the following points:
- code quality, maintainability, obvious flaws
- what's needed to get it up and running (keep notes for IT deployment later)
- when you have it running:
-- does it fulfill the requirements outlined in bug 571886 comment 26 ?
If there are minor changes (like typos) to be made, feel free to fork the tool onto github.com/mozilla/sheriff and commit them. Do not make any *substantial* changes should those be required, in that case please just report back.
Today I checked out a local copy of sheriff, setup a Ruby environment for it, and connected to MPT to access the LDAP server the instance of sheriff on khan is using. I managed to get sheriff somewhat up and running on my Mac, but I encountered a fair number of issues, either in terms of bugs or roadblocks to running the app.

The app seems to create too many MySQL connections. I didn't look deeply into why this is, but I routinely ran up to the max number of MySQL client connections on my Mac with default MySQL settings (and no other app connected). Restarting sheriff corrected the problem, but I'd get the error again after a few (between 3-6) page loads.

The app also seems to have trouble creating/retrieving sessions via Rack::Session::Sequel module. Again, not sure why this is, but it means a lot of different session ids are created during a single login request.

Getting the app up and running was a bit of work (it doesn't seem to work with Ruby 1.9.2, for instance). This could likely be corrected with documentation clarification (and using Bundler for gem management), but there was a bit of guesswork getting it up and running (and finding out which rake commands to run). Not running on Ruby 1.9.2 is an issue too, but could likely be corrected without too much fuss.

------------------------------------------

DEPLOYMENT/DEVELOPMENT:

The app doesn't seem to run without a Rackup config, but that's also pretty easy to fix. When I ran the app I also didn't see any assets (css/js) being served.

------------------------------------------

CODE QUALITY:

The code seems OK but the scale of the app seems too big for Sinatra. Sinatra is, in my mind, built for _very simple_, single-file web apps. I see a lot of Sinatra apps that outgrow the framework (which is really just an HTTP router with a few niceties); I think this app would quickly outgrow Sinatra if it hasn't already.

Given that we aren't a Ruby shop the code could be documented better and there does seem to be a lot of variable passing instead of object use.

------------------------------------------

REQUIREMENTS:

I'm honestly not sure if the app fulfils the requirements mentioned because I couldn't get it to function properly. I usually had to restart it because of SQL client connection errors or couldn't stay logged in because of session errors.

------------------------------------------

THOUGHTS:

I think we could learn from the way sheriff is designed in terms of flow, but it seems like it has enough work left to be done that a Django/playdoh rewrite may be in order. There are also deployment issues with Ruby. I can help in that regard; I know about Rack deployment, but it's not an infrastructure we currently possess.

Plus, the Flux team will be working with Django and LDAP for the Mozillians project, so there might be some code re-use that could happen there.

If I'm wrong about some of these things and they are easily fixable, then maybe a rewrite isn't necessary. I'd be more than happy to maintain a Ruby app, but given issues with the current app and our lack of Ruby infrastructure/knowledge, **I think a rewrite of this tool might be our best bet**.

I loathe suggesting throwing code away, so if I'm wrong on any of this please let me know.

I think we should evaluate rewriting this in Django at this point though.
Thanks for taking the time for this review, Matt!
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Matt, thanks for the thoughtful review!
Product: mozilla.org → mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.