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

2351 CLI sets exit-code 1 if the command supplied was not parseable #2358

Merged
merged 2 commits into from Jan 18, 2018

Conversation

@P-Seebauer
Copy link
Contributor

@P-Seebauer P-Seebauer commented Nov 28, 2017

See issue #2351.

Sorry, I haven't found tests for the bin/cli.js file.

Reference to a stack exchange question, that is somewhat related .

@elhigu
Copy link
Member

@elhigu elhigu commented Nov 29, 2017

Sorry, I haven't found tests for the bin/cli.js file.

There hasn't been tests for cli.js so far. Someone has to be the first one to write them 😅

bin/cli.js Outdated
@@ -186,7 +186,8 @@ function invoke(env) {
commander.parse(process.argv);

Promise.resolve(pending).then(function() {
commander.help();
commander.outputHelp();
exit('');

This comment has been minimized.

@elhigu

elhigu Dec 10, 2017
Member

Maybe some more specific error message about command not found could be given.

This comment has been minimized.

@P-Seebauer

P-Seebauer Dec 22, 2017
Author Contributor

Done. Also realized then that chalk and console.error were called in the wrong order…

Also wasn't sure what your policy is regarding rebasing/amending.
Sorry if I got that wrong.

This comment has been minimized.

@elhigu

elhigu Dec 29, 2017
Member

Eventually PR goes in as a squash merge so rebase/amend/etc. doesn't really matter anything goes.

@elhigu elhigu mentioned this pull request Dec 10, 2017
@P-Seebauer P-Seebauer force-pushed the P-Seebauer:2351-exit-code-on-failure branch 2 times, most recently from d32cef1 to 44ac1a1 Dec 22, 2017
@P-Seebauer
Copy link
Contributor Author

@P-Seebauer P-Seebauer commented Jan 10, 2018

The failed integration doe snot look like it was caused by my changes (failed to install oracledbinstall)
is there a way to restart the CI?

@elhigu
Copy link
Member

@elhigu elhigu commented Jan 11, 2018

@P-Seebauer There was a problem that oracle stopped providing binaries to some node versions. After rebase tests should run fine.

@P-Seebauer P-Seebauer force-pushed the P-Seebauer:2351-exit-code-on-failure branch from 44ac1a1 to bee0e1e Jan 16, 2018
@P-Seebauer
Copy link
Contributor Author

@P-Seebauer P-Seebauer commented Jan 17, 2018

I rebased and repushed.
But it does seem like travis did not want to run the tests…

https://travis-ci.org/tgriesser/knex/builds/329612776

@elhigu elhigu merged commit 15706c0 into knex:master Jan 18, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@P-Seebauer P-Seebauer deleted the P-Seebauer:2351-exit-code-on-failure branch Jan 29, 2018
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

2 participants