889015 - Switch to South for managing migrations

Status: RESOLVED FIXED

(Webtools Graveyard :: Elmo, defect, P2)

Description

[Peter Bengtsson [:peterbe]]

Bye bye Nashvegas. Say hello to South.

Comment 1

[[DEACTIVATED] Adrian Gaudebert]

Attachment: patch.diff

Summary:

* added South to vendor-local, installed apps, requirements
* removed nashvegas and its migration files
* added initial migration state for all apps with content in models.py

Comment 2

[Peter Bengtsson [:peterbe]]

Comment on attachment 779797 [details] [diff] [review]
patch.diff

Review of attachment 779797 [details] [diff] [review]:
-----------------------------------------------------------------

You also need to update update_site.py

::: apps/mbdb/fields.py
@@ +68,4 @@
>                                      connection=connection, prepared=True))
>  
>  
> +add_introspection_rules([], ["^mbdb\.fields\.PickledObjectField"])

Perhaps a comment why this is needed. It's not particularly normal.

Comment 3

[[DEACTIVATED] Adrian Gaudebert]

Attachment: patch.diff

New patch, does the same as the previous plus:

* updated update_site.py script to run South migration
* fixed a bug in mbdb's models preventing the migration to run

Peter, do you also want a diff from the last commit in addition to that big diff?

Comment 4

[Peter Bengtsson [:peterbe]]

Comment on attachment 781169 [details] [diff] [review]
patch.diff

Review of attachment 781169 [details] [diff] [review]:
-----------------------------------------------------------------

It looks like all is in order.

::: scripts/update_site.py
@@ +31,4 @@
>  
>  GIT_PULL = "git pull -q origin %(branch)s"
>  GIT_SUBMODULE = "git submodule update --init --recursive"
> +SOUTH_EXEC = "./manage.py migrate"

Wow! I did not know this was possible. It used to be that you had to type each app name after the command "migrate".

Comment 5

[[github robot]]

Commit pushed to develop at https://github.com/mozilla/elmo

https://github.com/mozilla/elmo/commit/5acbdd4e74376d8da5d255fa14f4349d6fd44047
bug 889015, switched to South for migrations, removed nashvegas, r=peterbe

Comment 6

[[DEACTIVATED] Adrian Gaudebert]

(In reply to Peter Bengtsson [:peterbe] from comment #4)
> > +SOUTH_EXEC = "./manage.py migrate"
> 
> Wow! I did not know this was possible. It used to be that you had to type
> each app name after the command "migrate".

Yeah, me neither, I was trying to find a good way to list all the apps when I tried that and was happily surprised to see that it worked. :)

Comment 7

[[DEACTIVATED] Adrian Gaudebert]

> FATAL ERROR - The following SQL query failed: CREATE INDEX `mbdb_property_40858fbd` ON `mbdb_property` (`value`);
> The error was: (1170, "BLOB/TEXT column 'value' used in key specification without a key length")

I thought this was solved locally, it wasn't. For some reason when I ran ./manage.py migrate again, it didn't error out.

Comment 8

[[DEACTIVATED] Adrian Gaudebert]

Attachment: patch.diff

Fixed the error in South migration. Turned back custom fields to inherit models.Field. (I made them inherit models.TextField instead thinking that solved this bug, it appears it didn't. )

Comment 9

[Peter Bengtsson [:peterbe]]

Comment on attachment 781597 [details] [diff] [review]
patch.diff

Review of attachment 781597 [details] [diff] [review]:
-----------------------------------------------------------------

r+ from me but hoping for some interesting thoughts from Pike regarding effectively disabling the index on the TEXT fields.

Comment 10

[[github robot]]

Commit pushed to develop at https://github.com/mozilla/elmo

https://github.com/mozilla/elmo/commit/78c5b4cf615a99671c060f3e8682a606fc6e0f18
bug 889015, fixed error in South migration, r=peterbe

Comment 11

[[DEACTIVATED] Adrian Gaudebert]

I have pushed this to develop to (hopefully) fix the build before I leave on vacation. I'm happy to update it later if need be. :)

Comment 12

[[DEACTIVATED] Adrian Gaudebert]

This is still broken: I didn't run the unit tests before merging my work, and I must apologize for this. Patch incoming.

Comment 13

[[DEACTIVATED] Adrian Gaudebert]

Attachment: patch.diff

Fixed the migration unit test.

Comment 14

[Axel Hecht [:Pike]]

I think I want mbdb.models.Property.value to be indexed, I'm actually having searches on that.

Comment 15

[Peter Bengtsson [:peterbe]]

Comment on attachment 782198 [details] [diff] [review]
patch.diff

Review of attachment 782198 [details] [diff] [review]:
-----------------------------------------------------------------

With this, now the whole test suite works?

Comment 16

[Axel Hecht [:Pike]]

Comment on attachment 781597 [details] [diff] [review]
patch.diff

Review of attachment 781597 [details] [diff] [review]:
-----------------------------------------------------------------

I don't really understand this patch. Why drop being a textfield and disabling indexing? Where does the base column type come from in the end?

Also, indexing in TEXT fields is OK in mysql, it's just not OK in text fields without a max_length smaller than the right amount.

Comment 17

[Peter Bengtsson [:peterbe]]

:Pike do you have a suggestion how to solve the migration problem then if we need to continue to have an index on a TEXT field?

In the Monday meeting, which Adrian missed, you talked about forcing the mysql driver to somehow store that column specifically as a byte string. But I honestly don't know where to even begin doing such a hack.

Comment 18

[Axel Hecht [:Pike]]

Checked the code, it doesn't really matter if we inherit from TextField or Field, as we keep the 

    def get_internal_type(self):
        return 'TextField'

That's the one that matters. I also realized that making us inherit from Field is actually an undo of the previous patch. And I verified that the actual DBs don't have an index right now.

Not sure if going for a BLOB would help, we have 1200 characters in existing properties, so a regular index in innodb won't help (max of 767 bytes). If we're new enough, we could try to play with full text search, though.

Comment 19

[[github robot]]

Commit pushed to develop at https://github.com/mozilla/elmo

https://github.com/mozilla/elmo/commit/8c9093396550eda3f20dda362470c2b7381f3f23
bug 889015, fixed the unit tests for South migrations, r=peterbe

Comment 20

[[DEACTIVATED] Adrian Gaudebert]

Aaaand that's fixed! :)