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

Migrations with CLI and without knexfile #2884

Merged
merged 3 commits into from Nov 6, 2018
Merged

Conversation

@aurium
Copy link
Contributor

@aurium aurium commented Oct 28, 2018

Allows to create and run migrations.

closes #2819

Allows to create and run migrations.

closes #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);

This comment has been minimized.

@kibertoad

kibertoad Oct 28, 2018
Collaborator

did you run prettier formatting before commit?..

This comment has been minimized.

@aurium

aurium Oct 28, 2018
Author Contributor

No. Is it necessary?

This comment has been minimized.

@kibertoad

kibertoad Oct 29, 2018
Collaborator

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 =

This comment has been minimized.

@kibertoad

kibertoad Oct 28, 2018
Collaborator

Would you consider replacing all vars in this file with let/const?

This comment has been minimized.

@aurium

aurium Oct 28, 2018
Author Contributor

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.

This comment has been minimized.

@kibertoad

kibertoad Oct 29, 2018
Collaborator

Fair enough :)

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'

This comment has been minimized.

@kibertoad

kibertoad Oct 28, 2018
Collaborator

since we depend on Lodash anyway, you could just use _.isObject

This comment has been minimized.

@aurium

aurium Oct 28, 2018
Author Contributor

Thanks! Changing...

bin/cli.js Outdated
function mkConfigObj(env, commander) {
env.configuration = {
ext: 'js',
[commander.env || process.env.NODE_ENV || 'development']: {

This comment has been minimized.

@kibertoad

kibertoad Oct 28, 2018
Collaborator

this is very difficult to read. Could you please split it somewhat? Inline condition inside key name resolution is heavy :)

This comment has been minimized.

@aurium

aurium Oct 28, 2018
Author Contributor

true. :-)

bin/cli.js Outdated
@@ -35,11 +35,29 @@ function checkLocalModule(env) {
}
}

function initKnex(env) {
function mkConfigObj(env, commander) {
env.configuration = {

This comment has been minimized.

@kibertoad

kibertoad Oct 28, 2018
Collaborator

so this is mutating passed params? Can this be done in a more immutable way?

This comment has been minimized.

@aurium

aurium Oct 28, 2018
Author Contributor

Done!

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.')

This comment has been minimized.

@kibertoad

kibertoad Oct 28, 2018
Collaborator

"whitout " -> typo

This comment has been minimized.

@aurium

aurium Oct 28, 2018
Author Contributor

corrected

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.')

This comment has been minimized.

@kibertoad

kibertoad Oct 28, 2018
Collaborator

same

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);

This comment has been minimized.

@kibertoad

kibertoad Oct 28, 2018
Collaborator

do we need to pass commander instance for internal calls? can't we pass resolved params instead?

This comment has been minimized.

@aurium

aurium Oct 28, 2018
Author Contributor

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 = (

This comment has been minimized.

@kibertoad

kibertoad Oct 28, 2018
Collaborator

might make sense to extract to internal function with self-explanatory name or at least add comment with explanation

This comment has been minimized.

@aurium

aurium Oct 28, 2018
Author Contributor

Now using initKnex(env, commander.opts()). looks more explicit on its intention.

@@ -0,0 +1,42 @@
#!/bin/bash -e

This comment has been minimized.

@kibertoad

kibertoad Oct 28, 2018
Collaborator

wouldn't it be simpler to just write a Jake (https://www.npmjs.com/package/jake) script within usual unit test?

@kibertoad
Copy link
Collaborator

@kibertoad kibertoad commented Oct 28, 2018

@aurium Thank you for your contribution!

@kibertoad
Copy link
Collaborator

@kibertoad kibertoad commented Oct 28, 2018

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.
@kibertoad
Copy link
Collaborator

@kibertoad kibertoad commented Oct 29, 2018

@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).

@kibertoad
Copy link
Collaborator

@kibertoad kibertoad commented Nov 2, 2018

LGTM! @elhigu, what do you think?

@elhigu
Copy link
Member

@elhigu elhigu commented Nov 6, 2018

Hey... awesome client tests! 🎉 Now we can start finishing all the other client PRs which were deferred because of missing tests 😊

@elhigu
Copy link
Member

@elhigu elhigu commented Nov 6, 2018

And great work everyone!

@elhigu elhigu merged commit bb5da4b into knex:master Nov 6, 2018
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 84.708%
Details
mwilliammyers added a commit to voxjar/knex that referenced this pull request Dec 12, 2018
* 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`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants