Ticket #430 (reopened enhancement)

Opened 4 years ago

Last modified 14 months ago

Avoiding the "./manage.py migrate myapp 0001 --fake"

Reported by: smyrman Owned by: andrew
Priority: major Milestone: 1.0
Component: migrations Version: mercurial
Keywords: Cc: smyrman@…

Description

It would be great if one could avoid having to run "./manage.py migrate myapp 0001 --fake" as described here.

Please tell me if what I suggest is stupid, but would it not be possible to have a settings parameter that enabled the following extra functionallety in "./mange.py migrate myapp" (pseudo code):

def migrate(“app”)

“””Call after manage.py syncdb”””
table_name = get_fist_table_name_from_migration_0001(app)
table_list = get_list_of_table_names_from_db()

if table_name in table_list and app not in south's_app_table:

system(“./manage.py migrate myapp 0001 --fake”)

Do you think it will work, ot am I missing something?

If it does work, one could easily start out with an app that doesn't use South, and once there is a need to migrate it, just after a bunch of users has already started using the app, you can add South, push your upgrade to the community. And they would never notice a thing.

Attachments

safe_migration.py (3.8 KB) - added by andrew 4 years ago.
Sindre's proposed solution
autofake_first_patch.diff (7.2 KB) - added by smyrman@… 4 years ago.
A patch ('hg diff') that adds the '--autofake-first' option to the 'migrate' command.
autofake_first_patch2.diff (8.0 KB) - added by smyrman@… 4 years ago.
autofake_first_patch.diff only worked properly for one app with several migrations. Fixed that and (tested more) for this patch.
autofake_first_patch3.diff (8.1 KB) - added by smyrman@… 4 years ago.
Fixed indentation error.
autofake_first_patch4.diff (5.9 KB) - added by smyrman@… 4 years ago.
Fixed Django 1.1 incompatibility. Does not remove trailing whitespaces.
autofake_first_patch5.diff (5.8 KB) - added by smyrman@… 4 years ago.
Uses 'south.db.dbs' to get database connection. Uses South's ORM for a particular migration instead of Django's get_models
autofake_first_patch6__untested.diff (5.9 KB) - added by smyrman@… 4 years ago.
Upgraded patch to merge with current trunk.

Change History

Changed 4 years ago by andrew

Sindre's proposed solution

Changed 4 years ago by smyrman@…

A patch ('hg diff') that adds the '--autofake-first' option to the 'migrate' command.

Changed 4 years ago by smyrman@…

autofake_first_patch.diff only worked properly for one app with several migrations. Fixed that and (tested more) for this patch.

Changed 4 years ago by smyrman@…

Fixed indentation error.

Changed 4 years ago by smyrman@…

Fixed Django 1.1 incompatibility. Does not remove trailing whitespaces.

Changed 4 years ago by smyrman@…

Uses 'south.db.dbs' to get database connection. Uses South's ORM for a particular migration instead of Django's get_models

comment:1 Changed 4 years ago by smyrman@…

Andrew. Is there any chance that the latest patch (autofake_first_patch5.diff) can bee applied? Do you need it to be updated, improved or changed somehow?

Or do you just want to wait?

comment:2 Changed 4 years ago by andrew

Sorry, this bug has been the victim of my lack of free time over the last eight months, it would appear.

Currently, the patch is failing (trunk has moved on), and I still wasn't sure about it in the first place (what with the whole introspection of tables thing). If you want to update it, I'll try and make time to take another look.

Changed 4 years ago by smyrman@…

Upgraded patch to merge with current trunk.

comment:3 Changed 4 years ago by smyrman@…

I can understand the little free time issue=)

You mentioned last time, that as long as '--autofake-first' remained a convinient alternative to '--fake 0001', a little bit introspection might didn't harm that much.

In the newest patch (v6), I i marked the one function that does use introspection with introspection__....

The newest patch (v6) is practically identical to the sufficiently tested v5, except it applies to trunk. v6 itself is however untested, as I have not set up a test project to run it on at the moment.

If you want to apply the patch, I can write a proper test that can be added to your 'tests' folder. As I to have little free time however, I do not care to do this if the patch is not going to be applied anyway;)

-- Sindre

comment:4 Changed 4 years ago by smyrman@…

Yeah, and here is how you can test it (and how I tested it the last time):

$ syncdb migrate appx 0001
$ syncdb migrate appx zero --fake
$ syncdb migrate --autofake-first

comment:5 Changed 4 years ago by andrew

So, I've imported it into a branch in the main South repo; the test suite passes, and my initial fiddling shows it works as expected.

If you could find the time to write a proper test, I should be able to get the time to look at that and merge it all in; I'm reasonably happy with the patch itself, as it's a lot less invasive than I thought it might be.

comment:6 Changed 4 years ago by smyrman@…

Thats nice to hear. I'll report back when I have found the time to write a proper test.

comment:7 Changed 3 years ago by lukeburden@…

I've got a packaged web application which uses reversion. Reversion has just introduced south migrations into it's directory structure. This results in existing instances of this application erroring out during the south migrate step of package upgrades, as reversion tables already exist.

This patch would provide a workaround - is it waiting patiently in a branch for its day to shine? :)

That said, I can think of a couple of ways of solving this problem with a little more precision:

1) Explicitly identify initial migrations (ie, sync db equivalent steps) by class, and have logic to compare the current schema with the schema that the forward step the initial migration would create. If they matched, it would automatically fake the step. If current schema is empty, it would create the tables. If the schemas were both non-empty but different, then would be an appropriate time to yell loudly.

2) Put similar introspective logic in the south.db.db.create_table method. Ie, don't scream about a table already existing if the table you were asked to create would have been identical to one that already exists.

What do you think is the correct way to solve this problem?

comment:8 Changed 3 years ago by smyrman@…

Hi. The only thing missing for the patch is proper tests, I think... I was supposed to write them, but I am afraid I have been kind of buzzy. I started trying to understand the existing tests in South, but then I didn't find the time to actually write them.

Anyway, I think Andrew has a principal of including as little introspection as possible in South. The current code should be enough as long as nobody have manually or by other means tinkered with the database before installing South. Other then that, I think that the explicit identification per class might make sense if it can be implemented rather clean, but I don't know If I will be able to do that:)

If you (or somebody else) have the time to write the tests before I
can get to it, this is the tests I think the patch need:

  • Demonstrate that missing first migrations for one app in a project with multiple apps (e.g. 3) are detected.
  • Demonstrate that missing first migrations for two apps in a projects with multiple apps (e.g. 3) are detected

comment:9 Changed 3 years ago by Luke Plant <L.Plant.98@…>

I've updated the patch, fixing various issues with it, and writing tests. In order to write the tests and the patch properly I first needed to do some cleanup, which I split out into 2 other patches.

Pull request is here:

https://bitbucket.org/andrewgodwin/south/pull-request/24/fix-for-430-with-tests-and-related-cleanup

comment:10 Changed 14 months ago by philipn@…

I've updated Luke's branch to be mergeable with the current master here:

https://github.com/philipn/django-south/tree/autofake-first

Note: See TracTickets for help on using tickets.