Skip to content

Conversation

HaraldNordgren
Copy link
Contributor

@HaraldNordgren HaraldNordgren commented Aug 5, 2018

(Depends on and includes #85)

This is an idea that I have been thinking about for a while. It would be nice if migration libraries supported a feature to verify all your up and down migrations and that they work together. A first step in a straightforward integration test scenario would be to continously run migrations

  • Up
  • Down
  • Up

to make sure generally that you can go all the way up, all the way back down and again all the way up. I imagine this catches most cases of bad down migrations.

However, there are special cases where Up-Down-Up is not enough. Consider the following scenario:

001_create_card.up.sql:

CREATE TABLE cards (
  pin text
);

001_create_card.down.sql:

DROP TABLE cards;

002_pin_code.up.sql:

UPDATE cards
SET pin=''
WHERE pin IS NULL;

ALTER TABLE cards
  ALTER COLUMN pin SET NOT NULL;

002_pin_code.down.sql:

-- Instead of dropping the 'NOT NULL' contraint, the whole column is dropped
ALTER TABLE cards
  DROP COLUMN pin;

There is a bug in 002_pin_code.down.sql. The column is dropped instead of dropping the 'NOT NULL' contraint. However, when running all down migrations in a sequence it would go unnoticed since 001_create_card.down.sql drops the entire table. The only way to catch this migration bug is to run 002_pin_code.down.sql directly followed by 002_pin_code.up.sql.

I believe therefor that it would be useful to provide a tool that iterates through all up and down migration scenarios to uncover hidden incompatibles between migrations. Therefore I would like to create an all-combinations subcommand.

@HaraldNordgren HaraldNordgren changed the title Create 'all-combinations' command Create 'all-combinations' subcommand Aug 5, 2018
@coveralls
Copy link

Pull Request Test Coverage Report for Build 148

  • 14 of 46 (30.43%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-2.7%) to 45.872%

Changes Missing Coverage Covered Lines Changed/Added Lines %
cli/commands.go 0 7 0.0%
migrate.go 12 22 54.55%
cli/main.go 0 15 0.0%
Totals Coverage Status
Change from base Build 138: -2.7%
Covered Lines: 700
Relevant Lines: 1526

💛 - Coveralls

@dhui
Copy link
Member

dhui commented Aug 7, 2018

I don't think such a command has a place in the migrate CLI. The command is complicated and dangerous if accidentally run in production.

Devs are responsible for testing their up and down migrations. If an up/down migration pair is correctly written and tested, then the sequence of pairs should always result in the same schema, barring any unexpected side-effects.

In your example, it's not the responsibility of migrate to catch the SQL bug. It's the responsibility of the dev and the code reviewer to catch it.

@HaraldNordgren
Copy link
Contributor Author

Thanks for your response.

The command is no more dangerous than running migrate down in production.

But sure, it needs to be used very carefully I agree. I half-expected this kind of pushback on whether or not a command like this belongs in the library. Since it's a test-only tool I can understand the position. Clearly, any single person or company that wants to try a similar "all combinations" test can write it themselves. But logically that can be extended to any part of any software library.

@dhui dhui closed this Aug 7, 2018
@HaraldNordgren HaraldNordgren deleted the all-combinations branch September 18, 2018 10:00
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.

3 participants