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

Conversation

Projects
None yet
3 participants
@aurium
Copy link
Contributor

aurium commented Oct 28, 2018

Allows to create and run migrations.

closes #2819

Aurélio A. Heckert
Migrations with CLI and without knexfile
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

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

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

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

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

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

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

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

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

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Oct 28, 2018

@aurium Thank you for your contribution!

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Oct 28, 2018

CI is failing with "test/migrate-cli.sh: line 4: realpath: command not found" error, btw.

Aurélio A. Heckert
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.
@kibertoad

This comment has been minimized.

Copy link
Collaborator

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

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Nov 2, 2018

LGTM! @elhigu, what do you think?

@elhigu

This comment has been minimized.

Copy link
Collaborator

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

This comment has been minimized.

Copy link
Collaborator

elhigu commented Nov 6, 2018

And great work everyone!

@elhigu elhigu merged commit bb5da4b into tgriesser:master Nov 6, 2018

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 (tgriesser#2884)
* Migrations with CLI and without knexfile

Allows to create and run migrations.

closes tgriesser#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