Fixed #29198 -- Added `--plan` option to migrate command. #9968

cgdeboer · 2018-05-19T15:56:30Z

Summary

Initial Stab at the ticket #29198 (https://code.djangoproject.com/ticket/29198)

Highlights

Show the planned migrations in the order they will be applied with each of the operations in the order they will be applied.
Ensure that we respect the direction of the migration when describing the operation (i.e backwards and forwards)
If an operation in the reverse migration plan is irreversible, say so (with warning text style). See Fig 3
Try to describe SQL and Python operations with a little bit of detail.

Fig 1: Forwards

Fig 2: Backwards

Fig 3: Backwards with Warning

carltongibson

Hi @cgdeboer. Thanks for this. Looking good.

I've rebased, squashed the commits and adjusted the commit message to follow the guidelines.

I just had a couple of comments at this stage...

carltongibson · 2018-07-11T09:03:23Z

django/db/migrations/operations/utils.py

+        action = operation.sql if backwards is not True else operation.reverse_sql
+        prefix = ""
+    else:
+        action = "No further Detail"


I'm not sure about the utility of --> No further Detail. In this case it might be better to just leave it off.

(Maybe it's useful. Thoughts?)

Initially, I was thinking about each action saying at least something to preserve the cadence of the output, but I agree it probably is completely superfluous. I'll remove it.

docs/ref/django-admin.txt

+.. django-admin-option:: --plan
+
+Shows the order of unapplied migration operations Django will apply for the
+given ``migrate`` command. Unlike the similar ``showmigrations --plan``


cgdeboer · 2018-07-11T13:40:09Z

@carltongibson thanks for squashing, I added a couple commits to address your comments.

carltongibson · 2018-07-12T10:00:19Z

tests/migrations/test_commands.py

+        self.assertEqual(
+            "Planned operations:\n"
+            "migrations.0003_third\n"
+            "    Undo Create model Author --> \n"


I know it's slightly more complex but I had imagined we'd not have the --> if there was nothing after it.

What do you think?

Fair point, I'll get that cleaned up.

carltongibson

Hi @cgdeboer.

I like this. I think it should be useful.

Are you able to squash the commits again? (It seems like you re-merged master, or the single commit, back into your feature branch: you needed to reset to the branch here that I pushed to.

If your fork is origin locally:

git fetch origin
git reset --hard origin/ticket_29198

But now we need to squash and git push -f ... again. Are you able to do that?
)

carltongibson · 2018-07-12T10:03:01Z

tests/migrations/test_commands.py

+            "    Raw SQL operation --> SELECT * FROM migrations_salamander\n",
+            out.getvalue()
+        )
+        call_command("migrate", "migrations", "zero", verbosity=0)


The other test cases have a comment here:

# Cleanup by unmigrating everything

I think that's probably worthwhile (since I did a double-take myself reviewing even now).

I'll add that , re-squash and get the commit message in line. Long time user, first time contributor (to django), so pardon the missteps, and thanks for your patience.

cgdeboer · 2018-07-13T00:06:00Z

@carltongibson got those comments resolved, commits squashed and cleaned up, etc. Tests are passing locally, not sure if I've done something to offend Jenkins, but he doesn't look completely satisfied.

cgdeboer · 2018-07-16T16:59:10Z

@carltongibson let me know if I need that squashin' right.

carltongibson

Hi @cgdeboer.

This looks great.

I just left a couple of small comments. Can you address those and rebase, squash and force push again? (Hopefully the CI run should pass this time: failures are unrelated.)

From there we should be able to get this in. Thanks for the contribution!

carltongibson · 2018-07-19T09:13:58Z

docs/ref/django-admin.txt

+``migrate --plan`` command shows only the migrations, and operations, to be
+performed.
+
+This is intended for users to have an extra sanity check before performing


Not quite sure about the phrasing here. Maybe:

This is intended to allow users to verify operations before performing a more advanced set of migrations.

carltongibson · 2018-07-19T09:14:11Z

tests/migrations/test_commands.py

+        Tests --plan output of migrate command
+        """
+        out = io.StringIO()
+


Remove the blank line.

carltongibson · 2018-07-19T09:14:32Z

tests/migrations/test_commands.py

+        call_command("migrate", "migrations", verbosity=0)
+
+        out = io.StringIO()
+


Remove the blank line

carltongibson · 2018-07-19T09:16:20Z

django/db/migrations/operations/utils.py

@@ -14,6 +14,31 @@ def is_referenced_by_foreign_key(state, model_name_lower, field, field_name):
    return False


+def verbose_describe(operation, backwards):


I'm not 100% convinced on the function name here. Maybe someone has a suggestion?
(Maybe it's fine.)

carltongibson

OK. This looks great. Thanks @cgdeboer.

(The CI failure is unrelated.)

timgraham · 2018-07-28T18:37:31Z

django/core/management/commands/migrate.py

@@ -54,6 +55,10 @@ def add_arguments(self, parser):
            '--run-syncdb', action='store_true',
            help='Creates tables for apps without migrations.',
        )
+        parser.add_argument(
+            '--plan', action='store_true', dest='plan',


dest='plan', is unneeded as that's the default value from --plan (eac9ab7)

timgraham · 2018-07-28T18:41:08Z

django/core/management/commands/migrate.py


+        if options['plan']:
+            self.stdout.write("Planned operations:", self.style.MIGRATE_LABEL)


Please use single quotes throughout the patch except if the string contains a single quote.

I certainly can do that, though I was trying to emulate the pattern seen below in the file in master.

Lines 156, 187, 192, 195, 309, etc, all use double quotes, even when there are no single quotes.

Please confirm you'd like to shift from that pattern.

Yes, for new code we're trying to follow the style guide.

timgraham · 2018-07-28T18:42:45Z

django/db/migrations/operations/utils.py

@@ -14,6 +14,31 @@ def is_referenced_by_foreign_key(state, model_name_lower, field, field_name):
    return False


+def describe_operation(operation, backwards):


I'm thinking this would be better organized as a method of the migrate command.

Sure. I'll move it there.

timgraham · 2018-07-28T18:45:13Z

django/db/migrations/operations/utils.py

@@ -14,6 +14,31 @@ def is_referenced_by_foreign_key(state, model_name_lower, field, field_name):
    return False


+def describe_operation(operation, backwards):
+    """
+    Returns a string, intended for `stdout.write`, that describes


verb style: Return

timgraham · 2018-07-28T18:45:26Z

django/db/migrations/operations/utils.py

+        action = ""
+        prefix = "Undo " if backwards is True else ""
+    if action is None:
+        error = True


This branch is untested.

True... will fix.

timgraham · 2018-07-28T18:49:58Z

docs/ref/django-admin.txt

+Shows the order of unapplied migration operations Django will apply for the
+given ``migrate`` command. Unlike the similar ``showmigrations --plan``
+command which lists all migrations (even applied ones), the
+``migrate --plan`` command shows only the migrations, and operations, to be


drop commas around "and operations"

timgraham · 2018-07-28T18:50:37Z

docs/ref/django-admin.txt

+``migrate --plan`` command shows only the migrations, and operations, to be
+performed.
+
+This is intended to allow users to verify operations before performing a more


I don't think this sentence adds value.

Fair enough. I'll rm it.

timgraham · 2018-07-28T18:56:24Z

django/db/migrations/operations/utils.py

+    else:
+        action = "{}".format(action.replace("\n", ""))
+        error = False
+    action = action if len(action) < 40 else '{}...'.format(action[:37])


Please add a test for the truncation logic and make sure that all branches in this function are exercised by tests. This could use django.utils.text.Truncator

I'll take a look.

timgraham · 2018-07-28T18:57:46Z

django/core/management/commands/migrate.py

@@ -54,6 +55,10 @@ def add_arguments(self, parser):
            '--run-syncdb', action='store_true',
            help='Creates tables for apps without migrations.',
        )
+        parser.add_argument(
+            '--plan', action='store_true', dest='plan',
+            help='Shows a list of ordered migration actions that Django will perform.',


ordered -> the ?

just to confirm here. this comment is about the use of the "zero article" vs the "definite article" ? We want:

Shows a list of the ordered migration actions that Django will perform.

Correct?

I think "ordered" is also unnecessary (i.e. why would they be out of order ;-) ).

Honestly, I thought 'ordered’ would be a good cue. Since the semi-related showmigrations command does not list migrations in order by default.

I can ditch ordered and we will presume the reader knows that any actions will be ordered

timgraham · 2018-07-28T19:02:14Z

django/db/migrations/operations/utils.py

+        action = "{}".format(action.replace("\n", ""))
+        error = False
+    action = action if len(action) < 40 else '{}...'.format(action[:37])
+    styled_action = " -> {}".format(action) if action else action


Perhaps this would be simpler:

if action: action = ' -> ` + string

I don't think the variable name needs to change.

cgdeboer · 2018-07-29T03:12:31Z

@timgraham I think I got all the comments addressed.

timgraham

Looking better. I force pushed a few cosmetic updates.

How about displaying a message instead just "Planned operations:" if there isn't anything to do?

timgraham · 2018-07-30T14:04:01Z

tests/migrations/test_commands.py

@@ -268,6 +268,68 @@ def test_showmigrations_plan(self):
        # Cleanup by unmigrating everything
        call_command("migrate", "migrations", "zero", verbosity=0)

+    @override_settings(MIGRATION_MODULES={"migrations": "migrations.test_migrations_plan"})


Please use also use single quotes in the test where possible.

timgraham · 2018-07-30T14:05:33Z

django/core/management/commands/migrate.py

+        a migration operation.
+        """
+        if hasattr(operation, 'code'):
+            action = operation.code.__doc__ if not backwards else operation.reverse_code.__doc__


It looks like operation.reverse_code.__doc__ will fail with AttributeError if operation.reverse_code is None.

cgdeboer · 2018-07-31T21:56:06Z

How about displaying a message instead just "Planned operations:" if there isn't anything to do?

Following the pattern of other areas of the migrate management command. It seems a pattern like:

Planned operations:
  No planned migration operations.

follows the pattern of:

Running migrations:
  No migrations to apply.

updated and re-pushed

timgraham

The two fixes look good, but please add tests for them also.

timgraham · 2018-08-02T14:45:44Z

tests/migrations/test_commands.py

+        """Tests migrate --plan output."""
+        out = io.StringIO()
+        # Show the plan up to the third migration.
+        call_command("migrate", "migrations", "0003", plan=True, stdout=out, no_color=True)


Please use single quotes throughout the test.

cgdeboer · 2018-08-02T22:28:05Z

Done and pushed.

cgdeboer changed the title ~~WIP: Fixed #29198~~ Fixed #29198 May 21, 2018

cgdeboer changed the title ~~Fixed #29198~~ Fixed #29198 -- Add a "--plan" option to the migrate command May 22, 2018

carltongibson force-pushed the ticket_29198 branch from 03e7513 to 3986f06 Compare July 11, 2018 09:09

carltongibson changed the title ~~Fixed #29198 -- Add a "--plan" option to the migrate command~~ Fixed #29198 -- Added --plan option to migrate command. Jul 11, 2018

carltongibson reviewed Jul 11, 2018

View reviewed changes

carltongibson reviewed Jul 12, 2018

View reviewed changes

cgdeboer force-pushed the ticket_29198 branch 2 times, most recently from b874e05 to 1bfb989 Compare July 13, 2018 00:00

carltongibson reviewed Jul 19, 2018

View reviewed changes

cgdeboer force-pushed the ticket_29198 branch from 1bfb989 to 93d7606 Compare July 19, 2018 20:19

carltongibson approved these changes Jul 20, 2018

View reviewed changes

timgraham reviewed Jul 28, 2018

View reviewed changes

cgdeboer force-pushed the ticket_29198 branch 2 times, most recently from a3d23be to 035ce94 Compare July 29, 2018 02:57

cgdeboer force-pushed the ticket_29198 branch from 035ce94 to 441061a Compare July 29, 2018 20:33

django deleted a comment from cgdeboer Jul 30, 2018

timgraham reviewed Jul 30, 2018

View reviewed changes

timgraham force-pushed the ticket_29198 branch 2 times, most recently from 2d8ee8b to 7c9f632 Compare July 30, 2018 14:39

cgdeboer force-pushed the ticket_29198 branch 2 times, most recently from cdc285b to de65aec Compare August 1, 2018 11:03

timgraham reviewed Aug 2, 2018

View reviewed changes

cgdeboer force-pushed the ticket_29198 branch from de65aec to 6ab7711 Compare August 2, 2018 22:15

timgraham force-pushed the ticket_29198 branch from 6ab7711 to 9f8ad52 Compare August 3, 2018 19:07

Fixed #29198 -- Added migrate --plan option.

058d33f

timgraham force-pushed the ticket_29198 branch from 9f8ad52 to 058d33f Compare August 3, 2018 19:23

timgraham merged commit 058d33f into django:master Aug 3, 2018

		call_command("migrate", "migrations", verbosity=0)

		out = io.StringIO()

		@@ -14,6 +14,31 @@ def is_referenced_by_foreign_key(state, model_name_lower, field, field_name):
		return False


		def verbose_describe(operation, backwards):


		if options['plan']:
		self.stdout.write("Planned operations:", self.style.MIGRATE_LABEL)

		@@ -14,6 +14,31 @@ def is_referenced_by_foreign_key(state, model_name_lower, field, field_name):
		return False


		def describe_operation(operation, backwards):

Fixed #29198 -- Added --plan option to migrate command. #9968

Fixed #29198 -- Added --plan option to migrate command. #9968

Conversation

cgdeboer commented May 19, 2018 • edited

Summary

Highlights

Fig 1: Forwards

Fig 2: Backwards

Fig 3: Backwards with Warning

carltongibson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

cgdeboer commented Jul 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carltongibson left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgdeboer commented Jul 13, 2018

cgdeboer commented Jul 16, 2018

carltongibson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carltongibson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgdeboer commented Jul 29, 2018

timgraham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgdeboer commented Jul 31, 2018

timgraham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgdeboer commented Aug 2, 2018

Fixed #29198 -- Added `--plan` option to migrate command. #9968

Fixed #29198 -- Added `--plan` option to migrate command. #9968

cgdeboer commented May 19, 2018 •

edited

carltongibson left a comment •

edited