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

Fix CWD parameter support for CLI #2935

Merged
merged 3 commits into from Dec 3, 2018
Merged

Fix CWD parameter support for CLI #2935

merged 3 commits into from Dec 3, 2018

Conversation

@kibertoad
Copy link
Collaborator

@kibertoad kibertoad commented Dec 1, 2018

No description provided.

@kibertoad kibertoad requested a review from elhigu Dec 1, 2018
kibertoad referenced this pull request Dec 1, 2018
* 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
Copy link
Collaborator Author

@kibertoad 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
Member

@elhigu 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
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 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
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants