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.

Sign up for GitHub

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

Jump to bottom

✨ knex-migrator v2 #7605

Merged
merged 12 commits into from
Nov 7, 2016
Merged

✨ knex-migrator v2 #7605

merged 12 commits into from
Nov 7, 2016

Conversation

kirrg001
Copy link
Contributor

@kirrg001 kirrg001 commented Oct 20, 2016 •
edited
Loading

refs #7489
After this PR there is only one TODO left: see #7489.

Please see the PR in knex-migrator repo:
See TryGhost/knex-migrator#11

This PR shows the usage of running migrations within Ghost. (added an example migration)
This PR integrates the usage of resetting the database within Ghost.
You can checkout this branch and play with it.

current versioning

  • using knex-migrator requires a .knex-migrator file
  • this must contain a current version your project is on
  • so when Ghost is on 1.0.X, we expose 1.0
  • knex-migrator won't run any migrations newer then this version

hooks

We can hook into the whole process of knex-migrator.
For example:
Before migrations start, initialise the models.
After migrations, destroy database connection.

TODO

  • final test
  • 1.0, 1.1, 1.2 exists, then i import a database - what happens? -> Right now: when importing a JSON database file, Ghost would set the current Ghost version to the database version. Because we don't import the database version (see importer). I've added a task to Migrations: reset & improvements #7489 to test importing/exporting databases and change behaviour if needed
  • replace package.json knex-migrator tarball (when knex-migrator is released)
  • remove example migration before merge

@kirrg001 kirrg001 force-pushed the 1.0.0-dev/knex-migrator-migrate-reset branch from e66c7fb to 2bf64ce Compare October 21, 2016 11:13
@kirrg001
Copy link
Contributor Author

This is ready for a review. Happy about any feedback 🕵🏻

@ErisDS
Copy link
Member

ErisDS commented Oct 21, 2016 •
edited
Loading

I have reviewed this and TryGhost/knex-migrator#11, although that PR broke GitHub for me 😂

This is all looking amazing 🎉 , the code is maturing really nicely and I'm excited about how much this solution has evolved and grown up 👍 I especially like the details around being able to exclude some migrations & the flexibility in the commands, I think this is going to help us to move a lot faster! 💯

I have only 3, small pieces of feedback.

  1. The name of the config file is not ideal, due to npm ignoring dotfiles, as discussed here
  2. I think the behaviour of the reset needs further consideration.
  3. The message currently output on the CLI when you need to run init doesn't mention how to install knex-migrator or where to go to get help with the error. I think that we can improve that error message to provide more guidance and reduce contributor friction.

@kirrg001
Copy link
Contributor Author

  1. The name of the config file is not ideal, due to npm ignoring dotfiles, as discussed here

Agreed!
PR is coming to knex-migrator repo and to Ghost.

  1. I think the behaviour of the reset needs further consideration.

Will look into your comment in the PR TryGhost/knex-migrator#11
👍

  1. The message currently output on the CLI when you need to run init doesn't mention how to install knex-migrator or where to go to get help with the error. I think that we can improve that error message to provide more guidance and reduce contributor friction.

Yeah good feedback.
Added an issue, see TryGhost/knex-migrator#13

@kirrg001 kirrg001 force-pushed the 1.0.0-dev/knex-migrator-migrate-reset branch from fc1f9d9 to 25be625 Compare October 21, 2016 17:06
@kirrg001 kirrg001 mentioned this pull request Nov 1, 2016
Closed
@kirrg001
Copy link
Contributor Author

kirrg001 commented Nov 1, 2016 •
edited
Loading

Added knex-migrator 0.1.1
I would like to merge this for alpha.8 FYI 👍

We have 2 tasks left in the big migration task:

  1. remove all the old code
  2. test export/import of databases

@kirrg001 kirrg001 added this to the 1.0.0-alpha.8 milestone Nov 1, 2016
@kirrg001 kirrg001 changed the title [WIP] ✨ knex-migrator v2 ✨ knex-migrator v2 Nov 1, 2016
@kirrg001 kirrg001 force-pushed the 1.0.0-dev/knex-migrator-migrate-reset branch from 660e3d6 to 9f408dd Compare November 1, 2016 21:08
@kirrg001 kirrg001 mentioned this pull request Nov 1, 2016
Closed
@kirrg001
Copy link
Contributor Author

kirrg001 commented Nov 1, 2016

Travis is red, but it does not show any logs right now. Maybe a bug. Will try to rebuild tomorrow.

@kirrg001 kirrg001 mentioned this pull request Nov 2, 2016
Closed
@kirrg001
Copy link
Contributor Author

kirrg001 commented Nov 2, 2016

Ok travis is green now!

@kirrg001 kirrg001 mentioned this pull request Nov 3, 2016
Closed
@kirrg001
🎨 knex-migrator reset
1281876 
[ci skip]
@kirrg001
✨ add migration example
df81a1b 
- hooks
- 1.0

[ci skip]
@kirrg001
🛠 knex-migrator tarball
962582d 
- remove when released

[ci skip]
@kirrg001
🎨 jscs/jshint
b1c18d9
@kirrg001
🕵🏻 do not drop the database connection when running tests
6de0bc6 
- please read the comments in the commit
@kirrg001
🔥 remove example migration
331f030
@kirrg001
🛠 knex-migrator 0.1.0
31c030d
@kirrg001
🛠 knex-migrator 0.1.1
47ae892 
- fix a single test to ensure we catch the error
@kirrg001
🛠 knex-migrator 0.1.2
007a02c
@kirrg001
🎨 make tests green
26509aa 
- added my keyword: kate-migrations
- i will go over all TODO's when removing the old migrations code
@kirrg001
🛠 knex-migrator update
85d8224
@kirrg001
🛠 knex-migrator 0.2.0
86ba7c5
@kirrg001 kirrg001 force-pushed the 1.0.0-dev/knex-migrator-migrate-reset branch from 44cbb32 to 86ba7c5 Compare November 7, 2016 11:13
@kirrg001
Copy link
Contributor Author

kirrg001 commented Nov 7, 2016

Please review for alpha.8. Ghost-CLI is already using knex-migrator 0.2.0 as local dependency. And Ghost master is using 0.7.x.

@ErisDS ErisDS merged commit bae0de6 into TryGhost:master Nov 7, 2016
geekhuyang pushed a commit to geekhuyang/Ghost that referenced this pull request Nov 20, 2016
@kirrg001 @geekhuyang
✨ knex-migrator v2 (TryGhost#7605)
169d4dc 
* 🎨  knex-migrator reset

[ci skip]

* ✨  add migration example

- hooks
- 1.0

[ci skip]

* 🛠  knex-migrator tarball

- remove when released

[ci skip]

* 🎨  jscs/jshint

* 🕵🏻 do not drop the database connection when running tests

- please read the comments in the commit

* 🔥  remove example migration

* 🛠  knex-migrator 0.1.0

* 🛠  knex-migrator 0.1.1

- fix a single test to ensure we catch the error

* 🛠  knex-migrator 0.1.2

* 🎨  make tests green

- added my keyword: kate-migrations
- i will go over all TODO's when removing the old migrations code

* 🛠  knex-migrator update

* 🛠  knex-migrator 0.2.0
@ErisDS ErisDS removed their assignment Jun 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.0.0-alpha.8
Development

Successfully merging this pull request may close these issues.
2 participants
@kirrg001 @ErisDS