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

Extra Ecto Mix tasks for migrations #131

Merged
merged 6 commits into from
Dec 17, 2013

Conversation

christhekeele
Copy link
Contributor

@christhekeele
Copy link
Contributor Author

Updating the integrations tests.

@ericmj
Copy link
Member

ericmj commented Dec 11, 2013

To fix the warnings we need to purge the modules before running migrations on the same module again. You can add a function like this: https://github.com/elixir-lang/elixir/blob/master/lib/mix/test/test_helper.exs#L52 to one of the test helper files in test/support/.

Don't add questions to commits, rather post them as comments on the PR or ask on IRC. So please rebase the two last comments so the commit messages are nicer.

The rest looks really good, great work!

@ericmj
Copy link
Member

ericmj commented Dec 11, 2013

Im not sure of the usefulness of running or reverting a single migration, maybe we should have a parameter that allows you to run/revert up until a given migration version/name.

@christhekeele
Copy link
Contributor Author

I have to agree migrate.next is a little useless. I'm a sucker for parallelism.

I based rollback on the active_record equivalent, which reverts the latest migration. I get a lot of milage out of it when forging a new migration pre-commit: it allows me to easily go back and modify it before it gets pushed out there, saving me multiple 'corrective' migrations in a single commit.

I'm open to renaming rollback:all into something top-level and expressive, but can't think of a great name that wouldn't get confused with rollback.

I like the idea of extending the 'single' versions to accept a version/name or cardinal integer of how far to go, with the latest/pending as a default, but I'm not sure how to reconcile the syntax for to version_number with migrate/rollback n migrations. Thoughts? Which one would be more useful? Could we fit both into the same invocation?

I'll rebase, purge, and drop migrate.next here in a bit.

@kotedo
Copy link

kotedo commented Dec 11, 2013

I agree, being able to go back and forth until it sits right is important to me, too!

—Kai

On Dec 11, 2013, at 9:33, Christopher Keele notifications@github.com wrote:

I have to agree migrate.next is a little useless. I'm a sucker for parallelism.

I based rollback on the active_record equivalent, which reverts the latest migration. I get a lot of milage out of it when forging a new migration pre-commit: it allows me to easily go back and modify it before it gets pushed out there, saving me multiple 'corrective' migrations in a single commit.

I'm open to renaming rollback:all into something top-level and expressive, but can't think of a great name that wouldn't get confused with rollback.

I like the idea of extending the 'single' versions to accept a version/name or cardinal integer of how far to go, with the latest/pending as a default, but I'm not sure how to reconcile the syntax for to version_number with migrate/rollback n migrations. Thoughts? Which one would be more useful? Could we fit both into the same invocation?

I'll rebase, purge, and drop migrate.next here in a bit.


Reply to this email directly or view it on GitHub.

@ericmj
Copy link
Member

ericmj commented Dec 11, 2013

What do you think of the following tasks:

  • migrate - run up migrations until we are the latest version
  • migrate --version version - run up or down migrations until we are at given version
  • migrate --step steps - run steps number of up migrations
  • rollback - run a single down migration
  • rollback --all - run all down migrations
  • rollback --step steps - run steps number of down migrations

If we want to be able to run a single up migration, we can make migrate do that (like rollback) and make running all up migrations explicit with migrate --all.

This is just a proposal, I would love your inputs.

/cc @josevalim

@kotedo
Copy link

kotedo commented Dec 11, 2013

Hi Eric,

I love it.
Sweet deal.

—Kai

On Dec 11, 2013, at 14:28, Eric Meadows-Jönsson notifications@github.com wrote:

What do you think of the following tasks:

migrate - run up migrations until we are the latest version
migrate --version version - run up or down migrations until we are at given version
migrate --step steps - run steps number of up migrations
rollback - run a single down migration
rollback --all - run all down migrations
rollback --step steps - run steps number of down migrations
If we want to be able to run a single up migration, we can make migrate do that (like rollback) and make running all up migrations explicit with migrate --all.

This is just a proposal, I would love your inputs.

/cc @josevalim


Reply to this email directly or view it on GitHub.

@christhekeele
Copy link
Contributor Author

I like it. My only objection would be with

migrate --version version - run up or down migrations until we are at given version
migrate --step steps - run steps number of up migrations

That means the migrate task isn't consistent about always moving the schema up. If we're going to allow migrate to take you both directions, I'd rather:

  • migrate - run up migrations until we are the latest version
  • rollback - run a single down migration
  • rollback --all - run all down migrations
  • migrate --to version - run up or down migrations until we are at given version
  • migrate --step n - run number of migrations, positive for up, negative for down, zero for contentedness

migrate, rollback, and rollback --all cover the most common migration scenarios. migrate --version version and migrate --step n provide simple mechanisms to handle any other usecase and empower scripts.

@josevalim
Copy link
Member

Here is another proposal to get the best of both so far:

  • migrate - run up migrations until we are the latest version
  • migrate --all - same as migrate
  • migrate --to version - run up guaranteeing we are at the given version (no-op if we already have it)
  • migrate --step steps - run steps number of up migrations
  • rollback - won't work (ask user to use --all or --step 1)
  • rollback --all - run all down migrations
  • rollback --to version - run down guaranteeing we are at the given version (no-op if we already have it)
  • rollback --step steps - run steps number of down migrations

My only question is if rollback should 1) abort or 2) should do --step 1 (different from migrate) or 3) rollback all (a very bad idea).

@ericmj
Copy link
Member

ericmj commented Dec 12, 2013

@josevalim's proposal is good. I think just rollback can rollback a single step (why would you write rollback if not to rollback at least one step?).

Should we support "inexact" versions? For example say i have pending migrations 1, 3 and 5. If i do migrate --to 4 should we run migrations 1 and 3 or just error?

@josevalim
Copy link
Member

Should we support "inexact" versions? For example say i have pending migrations 1, 3 and 5. If i do migrate --to 4 should we run migrations 1 and 3 or just error?

I would say that is fine. It could just work.

@christhekeele
Copy link
Contributor Author

^ This is a force push rebase of my feature branch; there was enough churn to merit it.

@christhekeele
Copy link
Contributor Author

The main remaining issue is that my integration tests are still stomping on existing migrations.

I've tried a number of things to try and resolve this, to little avail.

The naive bumping version numbers + changing migration names doesn't work, for some reason no matter what the migration module is called 5 particular create_migrations always report a conflict when loaded (namely the ones on lines 125, 139, 140, 154, and 155).

I also tried the purge reccommended by @ericmj, wherein create_migration becomes a macro that accumulates the migration's module name into an attribute, and calling purge_migrations in a teardown that tries to delete and purge any registered loaded migrations. Interestingly enough, while that worked great for most of the migrations, it failed because the same 5 mentioned above still reported being duplicated, and the erlang code server reported:

=ERROR REPORT==== 13-Dec-2013::19:22:03 ===
Module 'Elixir.Ecto.MigrationTest.GoodMigration43' must be purged before loading

I'd love help with this: I have no idea what's going on, I'd like to see this out the door, and my brain is a little fried tonight. 😁

@ericmj
Copy link
Member

ericmj commented Dec 14, 2013

In some tests you are running the same migration multiple times. Every time you run a migration it will be loaded. Purging in a teardown doesn't help much because you are already naming the migrations differently. If you run the same migration multiple times you need to purge between runs.

@josevalim
Copy link
Member

Note, you can use this syntax to purge:

test "..." do
  # actual test
after
  purge [Migration1, Migration2]
end

I plan to review this pull request later today and give more insight. Thanks a lot @christhekeele! :D

@christhekeele
Copy link
Contributor Author

you need to purge between runs

Of course, I see it now.

after
  purge [Migration1, Migration2]
end

I didn't know that syntax existed! Cool.

Enum.filter(fn { version, _file } ->
version in versions
end)
|> :lists.reverse
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enum.reverse/1

@josevalim
Copy link
Member

This looks fantastic, excellent code, docs and tests! I have added minor notes, thank you @christhekeele!

@christhekeele
Copy link
Contributor Author

I've moved all option munging into the task level in a series of 4 commits.

The reason why I didn't start off with the logic there is because (linguistically) we have certain expectations when talking about running migrations:

  • A 'migration' is a vector script (it can be run in two directions).
  • 'Running migrations', means running migrations in some direction.
  • 'To migrate' means running migrations upwards.
  • 'Migrating' means running all available migrations upwards.
  • 'To revert' means running migrations downwards.
  • 'Rolling back' means running one migration downwards.

This is clearly insane. The inconsistency is an artifact of what operations we perform with migrations most often.

Each of the four commits moves responsibility for deciding how to run migrations further into the tasks. The result, as can be seen by browsing updated tests, is the Migrator.run's API becomes increasingly straightforward, but requires further and further code to achieve what linguistically we consider to be the 'expected' result.

I'm quite fine with that, but wasn't sure how you felt about the trade-off. You should be able to merge any of the four commits to suite your taste. I'll push them one-by-one to my feature branch so that travis makes sure each one is green.

Chris Keele added 6 commits December 15, 2013 16:03
Exposes new task `ecto.rollback` for reverting migrations, and
adds options `--all`, `--step n`, and `--to version` to both
`ecto.rollback` and `ecto.migrate`.

Deprecates calls to Migrator.run_all, which will for now warn, delegate to
Migrator.run, and behave identically to the old implementation.
- Direction is an explicit parameter
- Defaults are now handled by function clauses
- Allows non-strategy options to be consumed
- Deprecation warning removed
- Docs follow conventions
- 'strategies' vs 'strategy_types' no longer conflated
@christhekeele
Copy link
Contributor Author

Whoops, travis went dark because elixir got updated! Bad timing. On the other hand, I got to know about the release 18 minutes before anyone else! I rebased off of master trying to figure out if the update to deps.lock would fix it before realizing Travis was privvy to the release before I was, sorry for the noise.

@josevalim
Copy link
Member

@christhekeele superb work. Thanks for splitting it in commits! This is on my plate now, I will merge it soon. :) ❤️ 💚 💙 💛 💜

@josevalim josevalim merged commit 10c61ce into elixir-ecto:master Dec 17, 2013
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

Successfully merging this pull request may close these issues.

None yet

4 participants