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

✨ use knex-migrator #57

Merged
merged 4 commits into from Oct 18, 2016
Merged

Conversation

kirrg001
Copy link
Contributor

@kirrg001 kirrg001 commented Oct 13, 2016

TODO

  • test this code
  • knex-migrator package.json
  • forward path to knex migrator where to find the .knex-migrator file

Check database health before starting Ghost and run knex-migrator init if database was never initialised.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 81.448% when pulling e1b8f5d on kirrg001:feature/knex-migrator into d01852e on TryGhost:master.

@@ -45,6 +46,13 @@ module.exports = BaseCommand.extend({
// TODO: rethink this
return this.runCommand('config', 'paths.contentPath', path.join(process.cwd(), 'content'), {environment: environment})
.then(function () {
knexMigrator = new KnexMigrator();

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

@kirrg001
Copy link
Contributor Author

@acburdine the new Ghost-CLI version won't run with an older Ghost alpha version when merging this PR

@coveralls
Copy link

Coverage Status

Coverage decreased (-2.7%) to 78.788% when pulling 5a26c85 on kirrg001:feature/knex-migrator into d01852e on TryGhost:master.

@kirrg001 kirrg001 force-pushed the feature/knex-migrator branch 2 times, most recently from d1ddb33 to 17b259b Compare October 17, 2016 15:54
@kirrg001 kirrg001 changed the title [WIP] ✨ use knex-migrator ✨ use knex-migrator Oct 17, 2016
@coveralls
Copy link

Coverage Status

Coverage remained the same at 78.788% when pulling 17b259b on kirrg001:feature/knex-migrator into 596a668 on TryGhost:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 78.788% when pulling 17b259b on kirrg001:feature/knex-migrator into 596a668 on TryGhost:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 78.788% when pulling c17dcb6 on kirrg001:feature/knex-migrator into 596a668 on TryGhost:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 78.788% when pulling 70195fa on kirrg001:feature/knex-migrator into 596a668 on TryGhost:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 78.788% when pulling 89229ba on kirrg001:feature/knex-migrator into 596a668 on TryGhost:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 78.788% when pulling 79fb9b5 on kirrg001:feature/knex-migrator into 596a668 on TryGhost:master.

@acburdine
Copy link
Member

@kirrg001 I went ahead and changed the code to what I was asking (because needing to get this merged) - but here's why:

The cli has to require each command file (so it can get all of the necessary variables like command name, arguments, options, etc). Because of this, adding a bunch of require statements to the top of the file increases load times, especially when the command is not even being run (for example, if you run ghost config - you don't even need ghost start or its dependency on "knex-migrator", yet with your code knex-migrator would still be loaded).

Basically, this is probably over-ambitious optimization, but as it was a quick change and the same practice is followed everywhere else in the code, I think it's necessary.

I'm willing to revert this change in the future if there's good reason to, though 😄

@acburdine
Copy link
Member

So I pulled this locally to test - and it's not working - neither sqlite nor mysql dependencies are installed, so ghost start doesn't work. And I'm not sure that having ghost-cli depend on both sqlite and mysql is a good idea... 😕

@kirrg001
Copy link
Contributor Author

So I pulled this locally to test - and it's not working - neither sqlite nor mysql dependencies are installed, so ghost start doesn't work. And I'm not sure that having ghost-cli depend on both sqlite and mysql is a good idea... 😕

ghost start might not work because of a missing PR in Ghost?
https://github.com/TryGhost/Ghost/pull/7585/files

kirrg001 and others added 4 commits October 18, 2016 09:47
- check database health before starting Ghost
- run knex-migrator init if database was never initialised
@coveralls
Copy link

Coverage Status

Coverage remained the same at 78.788% when pulling 7feba78 on kirrg001:feature/knex-migrator into 4ecc2e3 on TryGhost:master.

@acburdine acburdine merged commit 79d4abb into TryGhost:master Oct 18, 2016
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.

None yet

3 participants