Switch to South for managing migrations

RESOLVED FIXED in 3.3

Status

Webtools
Elmo
P2
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: peterbe, Assigned: adrian)

Tracking

Trunk
x86
Mac OS X

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
Bye bye Nashvegas. Say hello to South.
(Reporter)

Updated

4 years ago
Priority: -- → P2
(Reporter)

Updated

4 years ago
Assignee: nobody → adrian
(Assignee)

Comment 1

4 years ago
Created attachment 779797 [details] [diff] [review]
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
Attachment #779797 - Flags: review?(peterbe)
(Reporter)

Comment 2

4 years ago
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.
Attachment #779797 - Flags: review?(peterbe) → review-
(Assignee)

Comment 3

4 years ago
Created attachment 781169 [details] [diff] [review]
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?
Attachment #779797 - Attachment is obsolete: true
Attachment #781169 - Flags: review?(peterbe)
(Reporter)

Comment 4

4 years ago
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".
Attachment #781169 - Flags: review?(peterbe) → review+

Comment 5

4 years ago
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
(Assignee)

Comment 6

4 years ago
(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. :)
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.3
(Assignee)

Comment 7

4 years ago
> 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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 8

4 years ago
Created attachment 781597 [details] [diff] [review]
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. )
Attachment #781597 - Flags: review?(peterbe)
(Reporter)

Comment 9

4 years ago
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.
Attachment #781597 - Flags: review?(peterbe)
Attachment #781597 - Flags: review+
Attachment #781597 - Flags: feedback?(l10n)

Comment 10

4 years ago
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
(Assignee)

Comment 11

4 years ago
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. :)
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
(Assignee)

Comment 12

4 years ago
This is still broken: I didn't run the unit tests before merging my work, and I must apologize for this. Patch incoming.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 13

4 years ago
Created attachment 782198 [details] [diff] [review]
patch.diff

Fixed the migration unit test.
Attachment #782198 - Flags: review?(peterbe)

Comment 14

4 years ago
I think I want mbdb.models.Property.value to be indexed, I'm actually having searches on that.
(Reporter)

Comment 15

4 years ago
Comment on attachment 782198 [details] [diff] [review]
patch.diff

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

With this, now the whole test suite works?
Attachment #782198 - Flags: review?(peterbe) → review+

Comment 16

4 years ago
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.
Attachment #781597 - Flags: feedback?(l10n) → feedback-
(Reporter)

Comment 17

4 years ago
: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

4 years ago
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

4 years ago
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
(Assignee)

Comment 20

4 years ago
Aaaand that's fixed! :)
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.