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

Added knex.migrate.to and knex.migrate.before #513

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sebastianseilund
Copy link

Like described in #506

Example usage in test:

describe('1234_my_change migration', function() {
  //First set the db in the state before the migration we're testing
  before(function() {
    return db.migrate.before('1234_my_change.js')
  })

  describe('up', function() {
    //Trigger `up` to be run
    before(function() {
      return db.migrate.to('1234_my_change.js')
    })

    it('created my_table', function() {
      return knex.schema.hasTable('my_table')
        .then(function(exists) {
          assert(exists)
        })
    })
  })

  describe('down', function() {
    //Trigger `down` to be run
    before(function() {
      return db.migrate.before('1234_my_change.js')
    })

    it('dropped my_table', function() {
      return knex.schema.hasTable('my_table')
        .then(function(exists) {
          assert(!exists)
        })
    })
  })
})

I also cleaned up the migration integration tests a bit. Isolated the tests for each method more and made sure that they each clean up after themselves.

Questions:

  1. Should the name of the version be the complete filename? E.g. do you say knex.migrate.to('12345678_my_thing.js') or knex.migrate.to('12345678')? I prefer the first one, as there is no checks AFAICS whether the integer part is unique.

Let me know if there is anything you want me to look at.

@tkellen
Copy link
Contributor

tkellen commented Oct 2, 2014

Awesome! I'd be partial to passing an integer for the migration number.

@tgriesser
Copy link
Member

Maybe both? Check for the filename, fallback to the integer?

@sebastianseilund
Copy link
Author

I can do that, if you would like it?

@tgriesser
Copy link
Member

Yeah, go for it.

@sebastianseilund
Copy link
Author

Sorry for the long wait. I added a commit that adds support for both full file names and integers now.

This means that:

db.migrate.to('20131019235306_migration_2.js');

Is equivalent to:

db.migrate.to(20131019235306);

If you're happy with the result I can squash the commits?

@voxpelli
Copy link
Contributor

voxpelli commented Jan 5, 2015

Would be great to have this work synced up with the discussion in #279 and the resulting code proposal in #617 which amongst other things does some refactorings to avoid some of the code duplication that happens when new migration methods like these ones are added.

@carera
Copy link

carera commented Aug 10, 2015

Can we have this, please?

@voxpelli
Copy link
Contributor

@carera See discussion in #617 I think

@elhigu
Copy link
Member

elhigu commented Feb 15, 2016

Hi, I started going through old pull requests and get them merged if possible or close otherwise if they are not going to be finished.

@sebastianseilund do you think this is still relevant and useful feature to have? If so, conflicts needs to be fixed and documentation of the new feature added to index.html.

@voxpelli
Copy link
Contributor

Still should be synced up with the discussion in #617 as they require similar extensions to the underlying migration methods, but probably makes sense as a feature request still.

Also still related to whether all of the migration work will move to https://github.com/knex/arctic as @bendrucker has indicated before or whether the current migration system is still open to extensions.

@phamdt
Copy link

phamdt commented Feb 23, 2016

i could definitely use this feature

@sheerun
Copy link

sheerun commented Sep 6, 2016

I took another route and created knex adapter and cli for unzug. It already handles "upto" and downto" logic. The only things to implement were support for batches and nice cli: knex-migrate

@nitish24p
Copy link

anyone working on this?

@elhigu
Copy link
Member

elhigu commented Sep 4, 2017

Closing due to inactivity, if someone likes to implement / fix this, please open another PR an link to this.

@elhigu elhigu closed this Sep 4, 2017
@gpetrov
Copy link
Contributor

gpetrov commented Jan 30, 2021

@elhigu such a shame that this request got closed just as the other #617

we really need "up to" and "down to" methods, just as in https://github.com/tj/node-migrate#running-migrations

with git backing all the version control - you really want to jump from version to version and not do separate up and downs in between.

So such functionality seems highly required these days! Please reopen and finalize!
@sebastianseilund if you are still alive could you post a new PR rebased to the latest changes? :) thanks

@sebastianseilund
Copy link
Author

@gpetrov Alive, but haven't used Knex in several years. You're welcome to finish up the work here :)

@kibertoad
Copy link
Collaborator

@gpetrov It is implemented and should be working. I finally have some time and will check out the API part of it to see if there are issues with it.

@kibertoad kibertoad reopened this Jan 30, 2021
@kibertoad kibertoad self-assigned this Jan 30, 2021
@kibertoad
Copy link
Collaborator

@gpetrov So this is more granular than what we have right now, which is moving single migration at a time? If that is the case, feel free to finish this PR, then it could be merged.

@kibertoad kibertoad removed their assignment Jan 30, 2021
@gpetrov
Copy link
Contributor

gpetrov commented Jan 30, 2021

Yes indeed @kibertoad we need more gradual from, to in sequence indeed, it is shame to loose the good code here, so I will see what I can do.

But are you ready with the whole conversion to classes of Knex? Don't want to rewrite afterwards :)

@kibertoad
Copy link
Collaborator

@gpetrov We've been doing lots of work in that direction lately, and are mostly done. I think #4253 is one of the last major pieces, but I don't think you would be hitting that place within the scope of this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet