Migrations with CLI and without knexfile #2884
Conversation
Allows to create and run migrations. closes knex#2819
bin/cli.js
Outdated
| if (!env.configPath) { | ||
| exit('No knexfile found in this directory. Specify a path with --knexfile'); | ||
| if (!env.configuration) { | ||
| if (commander.client) mkConfigObj(env, commander); |
There was a problem hiding this comment.
did you run prettier formatting before commit?..
There was a problem hiding this comment.
No. Is it necessary?
There was a problem hiding this comment.
Well, in this particular case I wonder if it would have removed the one-liner; typically it's encouraged to always enclose then statements in { } :)
bin/cli.js
Outdated
| @@ -52,7 +70,10 @@ function initKnex(env) { | |||
|
|
|||
| var environment = commander.env || process.env.NODE_ENV; | |||
| var defaultEnv = 'development'; | |||
| var config = require(env.configPath); | |||
| var config = | |||
There was a problem hiding this comment.
Would you consider replacing all vars in this file with let/const?
There was a problem hiding this comment.
I could, however there are many "var's" in the same scope and upper. I could do this in another PR to isolate the changes.
bin/cli.js
Outdated
| @@ -52,7 +70,10 @@ function initKnex(env) { | |||
|
|
|||
| var environment = commander.env || process.env.NODE_ENV; | |||
| var defaultEnv = 'development'; | |||
| var config = require(env.configPath); | |||
| var config = | |||
| typeof env.configuration == 'object' | |||
There was a problem hiding this comment.
since we depend on Lodash anyway, you could just use _.isObject
bin/cli.js
Outdated
| function mkConfigObj(env, commander) { | ||
| env.configuration = { | ||
| ext: 'js', | ||
| [commander.env || process.env.NODE_ENV || 'development']: { |
There was a problem hiding this comment.
this is very difficult to read. Could you please split it somewhat? Inline condition inside key name resolution is heavy :)
bin/cli.js
Outdated
| @@ -35,11 +35,29 @@ function checkLocalModule(env) { | |||
| } | |||
| } | |||
|
|
|||
| function initKnex(env) { | |||
| function mkConfigObj(env, commander) { | |||
| env.configuration = { | |||
There was a problem hiding this comment.
so this is mutating passed params? Can this be done in a more immutable way?
bin/cli.js
Outdated
| @@ -90,6 +112,12 @@ function invoke(env) { | |||
| .option('--debug', 'Run with debugging.') | |||
| .option('--knexfile [path]', 'Specify the knexfile path.') | |||
| .option('--cwd [path]', 'Specify the working directory.') | |||
| .option('--client [name]', 'Set DB client whitout a knexfile.') | |||
bin/cli.js
Outdated
| @@ -90,6 +112,12 @@ function invoke(env) { | |||
| .option('--debug', 'Run with debugging.') | |||
| .option('--knexfile [path]', 'Specify the knexfile path.') | |||
| .option('--cwd [path]', 'Specify the working directory.') | |||
| .option('--client [name]', 'Set DB client whitout a knexfile.') | |||
| .option('--connection [address]', 'Set DB connection whitout a knexfile.') | |||
bin/cli.js
Outdated
| @@ -136,8 +164,12 @@ function invoke(env) { | |||
| 'Specify the stub extension (default js)' | |||
| ) | |||
| .action(function(name) { | |||
| var instance = initKnex(env); | |||
| var ext = (argv.x || env.configPath.split('.').pop()).toLowerCase(); | |||
| var instance = initKnex(env, commander); | |||
There was a problem hiding this comment.
do we need to pass commander instance for internal calls? can't we pass resolved params instead?
There was a problem hiding this comment.
There is a method to do that on commander API? Can't find.
| var instance = initKnex(env); | ||
| var ext = (argv.x || env.configPath.split('.').pop()).toLowerCase(); | ||
| var instance = initKnex(env, commander); | ||
| var ext = ( |
There was a problem hiding this comment.
might make sense to extract to internal function with self-explanatory name or at least add comment with explanation
There was a problem hiding this comment.
Now using initKnex(env, commander.opts()). looks more explicit on its intention.
test/migrate-cli.sh
Outdated
| @@ -0,0 +1,42 @@ | |||
| #!/bin/bash -e | |||
|
|
|||
There was a problem hiding this comment.
wouldn't it be simpler to just write a Jake (https://www.npmjs.com/package/jake) script within usual unit test?
|
@aurium Thank you for your contribution! |
|
CI is failing with "test/migrate-cli.sh: line 4: realpath: command not found" error, btw. |
Thanks to @kibertoad comments! The bigger change was to remove the bash script to test knex's cli command, and replace it with a jake file. I believe this jake file may be used as a base for future cli tests.
|
@aurium Tests are failing on node 6 and 8 now. I think "fs.promises" is only available since Node 10, so probably you would have to promisify fs methods manually (e. g. using bluebird). |
|
LGTM! @elhigu, what do you think? |
|
Hey... awesome client tests! 🎉 Now we can start finishing all the other client PRs which were deferred because of missing tests 😊 |
|
And great work everyone! |
* Migrations with CLI and without knexfile Allows to create and run migrations. closes knex#2819 * Apply tweaks to migrate without knexfile Thanks to @kibertoad comments! The bigger change was to remove the bash script to test knex's cli command, and replace it with a jake file. I believe this jake file may be used as a base for future cli tests. * Replace `fs.promises` with `new Promise`
Allows to create and run migrations.
closes #2819