close
The Wayback Machine - https://web.archive.org/web/20200914233447/https://github.com/salsita/node-pg-migrate/pull/530
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

Allow 'Down' migrations in .sql files #530

merged 7 commits into from Dec 11, 2019

Conversation

@TheJetOU
Copy link
Contributor

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.

TheJetOU added 2 commits 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.
templates/migration-template.sql Outdated Show resolved Hide resolved
@TheJetOU TheJetOU force-pushed the TheJetOU:sql-down branch from 47d0ae2 to f00abc6 Dec 7, 2019
src/runner.ts Outdated Show resolved Hide resolved
src/runner.ts Outdated Show resolved Hide resolved
@dolezel
Copy link
Collaborator

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
18 checks passed
18 checks passed
ci/circleci: install Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
ci/circleci: test-cockroach-1 Your tests passed on CircleCI!
Details
ci/circleci: test-cockroach-2 Your tests passed on CircleCI!
Details
ci/circleci: test-config Your tests passed on CircleCI!
Details
ci/circleci: test-env-vars Your tests passed on CircleCI!
Details
ci/circleci: test-node-10 Your tests passed on CircleCI!
Details
ci/circleci: test-node-8 Your tests passed on CircleCI!
Details
ci/circleci: test-password-1 Your tests passed on CircleCI!
Details
ci/circleci: test-password-2 Your tests passed on CircleCI!
Details
ci/circleci: test-pg-10 Your tests passed on CircleCI!
Details
ci/circleci: test-pg-11 Your tests passed on CircleCI!
Details
ci/circleci: test-pg-9 Your tests passed on CircleCI!
Details
ci/circleci: test-schema Your tests passed on CircleCI!
Details
ci/circleci: test-schemas Your tests passed on CircleCI!
Details
ci/circleci: test-typescript-customrunner Your tests passed on CircleCI!
Details
ci/circleci: test-typescript-migration Your tests passed on CircleCI!
Details
@Shinigami92
Copy link
Contributor

Shinigami92 commented Dec 11, 2019

  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
Collaborator

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
Contributor

Shinigami92 commented Dec 11, 2019

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
Collaborator

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
Contributor

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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.