Skip to content
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

Testing for migrations from sys.argv is too naive #18

Open
intgr opened this issue Jun 3, 2021 · 3 comments
Open

Testing for migrations from sys.argv is too naive #18

intgr opened this issue Jun 3, 2021 · 3 comments

Comments

@intgr
Copy link

intgr commented Jun 3, 2021

Currently this project uses the following test to check whether we're migrating:

    if not set(sys.argv) & {"makemigrations", "migrate", "showmigrations"}:
        return DeprecatedField(return_instead)

This check can be too naive, however:

  • We directly call MigrationAutodetector in our test suite to ensure no missing migrations, but that fails if we add deprecate_field() anywhere.
  • We have a custom manage.py deploy command which among other things calls call_command("migrate", interactive=False). This now prints warnings:
Your models in app(s): '...' have changes that are not yet reflected in a migration, and so won't be applied.

I haven't looked into better solutions yet, just letting you know of this problem.

@flixx
Copy link
Member

flixx commented Jun 4, 2021

Hi @intgr understood.

Probably additionally, something like this could be used to check if we are inside djangos migration module...

import inspect

def is_inside_migration():
    for frame_info in inspect.stack():
        if frame_info[1].endswith("django/db/migrations/autodetector.py"):
            return True
    return False

Not sure if django/db/migrations/autodetector.py would be the best choice though.
Do you know a file that is for sure in the stack when running migrations?

@intgr
Copy link
Author

intgr commented Jun 5, 2021

I think that won't work. A bigger issue is that with this syntax:

class MyModel(models.Model):
    field1 = deprecate_field(models.CharField())

The deprecate_field() function is executed at import-time. Imports may come from arbitrary locations, at which time it's not known whether the field is going to be used for accessing data or migrating the schema. Indeed during the life of one Python process, like my test suite, it may be used both ways.

So I'm afraid returning different things based on a condition inside deprecate_field() function is not a good solution. You might have to return DeprecatedField always and do something conditional in there. But I'm guessing this brings more complications.

@christianbundy
Copy link

Could we use something simple like an environment variable for this? It's janky, but for folks who have complicated migrations configurations that aren't just ./manage.py migrate it might be useful to have an escape hatch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants