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

Allow 'Down' migrations in .sql files #530

Merged
merged 7 commits into from
Dec 11, 2019
Merged

Conversation

thejetou
Copy link
Contributor

@thejetou thejetou commented Dec 7, 2019

This commit allows SQL files to have down migrations via comments.

Anything below -- Up migration will be treated as up, anything
below a -- Down migration will be treated as down.

A migration file can have either or both.

This commit allows SQL files to have down migrations via comments.

Anything below `-- Up migration` will be treated as `up`, anything
below a `-- Down migration` will be treated as `down`.

A migration file can have either or both.
templates/migration-template.sql Outdated Show resolved Hide resolved
src/runner.ts Outdated Show resolved Hide resolved
src/runner.ts Outdated Show resolved Hide resolved
@dolezel
Copy link
Contributor

dolezel commented Dec 11, 2019

I would do three things:

  1. write tests
  2. do not explicitly require order of comments
  3. if down migration is not present, trying to run it should fail

@dolezel dolezel merged commit 3c4ec54 into salsita:master Dec 11, 2019
@Shinigami92
Copy link
Collaborator

  1. write tests

👍


  1. do not explicitly require order of comments

👍


  1. if down migration is not present, trying to run it should fail

What do you mean?

-- up migration
insert into ...

-- down migration
-- nothing to do

Do you think that should fail? Why?

@dolezel
Copy link
Contributor

dolezel commented Dec 11, 2019

It should keep current behavior. Where you can't run down migrations on SQL files.
The same as for normal migrations. You can write explicit down migrations, disabled it, or Terry to infer it from up migration. But you can't infer down migration from SQL string. It failed down migrations

@Shinigami92
Copy link
Collaborator

I'm still a little bit confused (and my english skills lags 😅)

I totally understand that we can't infer down migrations from up migrations, this wasn't my point
But I dont think it should fail when there is no SQL found below the down migration-comment

@dolezel
Copy link
Contributor

dolezel commented Dec 11, 2019

I'm probably describing it wrong. It should fail if running down migrations and there is no down migration comment.

@Shinigami92
Copy link
Collaborator

Shinigami92 commented Dec 11, 2019

OK

I dont think so

-- up migration
insert into ...

should work, imo

edit:
so IMO i mean follwoing should all work

example 1

-- up migration
insert into ...

same as example 1

insert into ...

example 2

-- down migration
delete from ...

example 3

-- down migration
delete from ...

-- up migration
insert into ...

example 4

-- down migration
delete from ...

-- up migration

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