New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable the database. #1127

Merged
merged 2 commits into from Nov 14, 2013

Conversation

Projects
None yet
4 participants
@pmclanahan
Member

pmclanahan commented Aug 1, 2013

  • Add sqlite config to base settings.
  • Update tests to use Django's database aware TestCase.
  • Install south.
  • Install django-waffle.
  • Update jenkins to use the database for CI.

Django admin will be enabled in a separate PR.

@pmclanahan

This comment has been minimized.

Member

pmclanahan commented Aug 2, 2013

WOO! Tests are passing. @Osmose mind taking a look?

@Osmose

This comment has been minimized.

Member

Osmose commented Aug 2, 2013

This looks good except south isn't a submodule. If it's in src, it should be a submodule.

Generally, with south (and anything else that isn't tied to a git commit, really) I install it in vendor-local/lib instead of src. Do that, and this is an r+ from me. 💴

@pmclanahan

This comment has been minimized.

Member

pmclanahan commented Aug 2, 2013

Okay... I think this is ready. I've filed bug 901078 to add the database settings to the local settings files of the various bedrock environments. We should wait until it's closed before merging this.

@pmclanahan

This comment has been minimized.

Member

pmclanahan commented Oct 30, 2013

@Osmose @jgmize Added the last two commits today. We're nearing deployment. Going to try to get this on a demo server very soon. Any comments on the updates? It's also rebased on latest master.

@jgmize

This comment has been minimized.

Member

jgmize commented Oct 31, 2013

@pmclanahan most of this commit is adding South-- I did the same thing for the nucleus project, but I didn't like it then, and I still don't. What do you think about a mozilla mirror of South on github for the purpose of including it as a git submodule?

@Osmose

This comment has been minimized.

Member

Osmose commented Oct 31, 2013

I forget if you need to include the .egg-info stuff for pip to be smart about versions and such (I suspect you do, and IMO it's fine to include that kind've stuff if it doesn't have platform-specific stuff, which I'm assuming it doesn't), but I think including the code directly in the repo is a step up from submodules.

This way, you can use pip to manage the dependency on the dev side, which is much easier to work with than git submodules. In a nicer world, you can eventually do things like generate all of vendor using a requirements file, which we did for captain.

Git submodule commands are inconsistent, and working with them almost always ends up with some form of frustration from a dirty submodule or leftover files in .git.

What do you think about a mozilla mirror of South on github for the purpose of including it as a git submodule?

That's adding a lot of extra concerns over maintenance and tracking updates. I don't think submodules are worth that kind of commitment.

@pmclanahan

This comment has been minimized.

Member

pmclanahan commented Oct 31, 2013

I agree with @Osmose here. I dislike having dependencies in the repo, but I like checking them in better than submodules.

@Osmose do you have a good script or procedure for doing the requirements.txt <--> packages dir sync thing? And deployment script changes. The proper pip incantations for doing all that properly escapes me so far.

@Osmose

This comment has been minimized.

Member

Osmose commented Oct 31, 2013

The proper pip incantations for doing all that properly escapes me so far.

pip install -I --install-option="--home=pwd/vendor-local" -r requirements/prod.txt is what I used. It probably doesn't handle things like removing unused libraries, but it's a start.

Your deployment script shouldn't change at all, as the code is updated along with the normal app code, and you still have submodules in the form of playdoh.

@pmclanahan

This comment has been minimized.

Member

pmclanahan commented Oct 31, 2013

Yeah. Realized that right after I typed it :) My mind was a step further in using pip to cache known good installable packages in the repo, then using pip on deploy to install those into the venv or python environment on the server to avoid so much path munging. This is a nice middle way though.

import test_utils
from tower import activate
from funfactory.urlresolvers import (get_url_prefix, Prefixer, set_url_prefix)
class TestCase(unittest.TestCase):
class TestCase(TestCase):

This comment has been minimized.

@jgmize

jgmize Nov 14, 2013

Member

@pmclanahan thanks for all the work you've put into this. I have to ask: why add the overhead of the django TestCase when none of the current tests actually use the DB? It makes more sense to me to continue using unittest.TestCase by default, and only use the django TestCase (or a subclass) when there is actual DB usage.

This comment has been minimized.

@jgmize

jgmize Nov 14, 2013

Member

@pmclanahan per our discussion I agree, the additional second or two that it takes to run the tests with the django test case is outweighed by the mental overhead incurred by contributors used to having the django TestCase by default.

print ' - ' + table
cursor.execute('DROP TABLE %s;' % table)
cursor.execute('SET FOREIGN_KEY_CHECKS = 1;')
transaction.commit_unless_managed()

This comment has been minimized.

@jgmize

jgmize Nov 14, 2013

Member

https://docs.djangoproject.com/en/dev/topics/db/transactions/#sequences-of-custom-sql-queries

Not sure about this commit_unless_managed call. Also, I'm not seeing where we begin the transaction.

This comment has been minimized.

@jgmize

jgmize Nov 14, 2013

Member

The 1.4 docs don't seem to talk about commit_unless_managed being not useful anymore, nor do they mention atomic, so I'm not sure how helpful the dev docs link was, actually

pmac added some commits Aug 1, 2013

Bug 920067: Enable the database.
* Add sqlite config to base settings.
* Update tests to use Django's database aware TestCase.
* Install south.
* Install django-waffle.
* Update jenkins to use the database for CI.

Django admin will be enabled in a separate PR.

jgmize added a commit that referenced this pull request Nov 14, 2013

@jgmize jgmize merged commit 1046dbc into mozilla:master Nov 14, 2013

1 check passed

default Jenkins build 'bedrock_github' #2391 has succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment