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

Fix CWD parameter support for CLI #2935

Merged
merged 3 commits into from Dec 3, 2018

Conversation

Projects
None yet
3 participants
@kibertoad
Copy link
Collaborator

kibertoad commented Dec 1, 2018

No description provided.

kibertoad added some commits Dec 1, 2018

@kibertoad kibertoad requested a review from elhigu Dec 1, 2018

kibertoad referenced this pull request Dec 1, 2018

Fix knexfile resolution. Add missing test (#2923)
* Fix knexfile resolution. Add missing test

* Try fixing Jake test execution

* Avoid having non-test files in jake folder

* Fix test compatibility with Node 6

* Fix the fix
@@ -11,6 +11,7 @@ const argv = require('minimist')(process.argv.slice(2));
const fs = Promise.promisifyAll(require('fs'));
const cliPkg = require('../package');
const { resolveClientNameWithAliases } = require('../lib/helpers');

This comment has been minimized.

@joselcvarela

joselcvarela Dec 1, 2018

@kibertoad
Hi again,
I just install your branch with npm like this npm i tgriesser/knex#fix/cli-cwd and when run npx knex migrate:latest inside knexfile.js folder it brokes on this line saying Cannot find module '../lib/helpers'.
While I debug it, find out that it is trying to resolve this file relative to my project folder instead of node_modules/knex

This comment has been minimized.

@joselcvarela

joselcvarela Dec 3, 2018

@kibertoad
Sorry, I was wrong because I was trying to run without lib folder. I just forked and change to your branch, ran npm run babel and find out the problem is in commander package, because I am passing --knexfile as argument like this npx knex --knexfile ./database/knexfile.js migrate:latest something and opt.knexfile is undefined.
So as a fix I just put './knexfile.js' as default value in commander knexfile argument and now it works

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Dec 3, 2018

@joselcvarela Can you try "--knexfile=./database/knexfile.js" and see if it makes any difference? Also try putting it after "migrate:latest". Would appreciate if you could report back if either of them would work.

@elhigu

elhigu approved these changes Dec 3, 2018

Copy link
Collaborator

elhigu left a comment

Looks good 👌 Only thing I found (again) confusing with this one, was the name of that one test spec file which is called knexfile.js 😬 But that is not in scope of this PR, so it can be left as is.

@kibertoad kibertoad merged commit 8dc0a71 into master Dec 3, 2018

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 84.99%
Details

@kibertoad kibertoad deleted the fix/cli-cwd branch Dec 3, 2018

mwilliammyers added a commit to voxjar/knex that referenced this pull request Dec 12, 2018

Fix CWD parameter support for CLI (tgriesser#2935)
* Fix cwd support for knex-cli. Add test

* Remove redundant code

* Remove commented out code
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment