Fix cli migrate:make sqlite dependency

[dstarke]

This is a proposed fix for #5102 .

Recent changes to how command line options are prioritized relative to configured options in the CLI exposed a bug in migrate:make. If a command line client option was not supplied, the CLI would use sqlite3 instead of whatever is configured in the configuration file. This causes an error in environments where the optional sqlite3 peer dependency is not installed.

This PR alters the CLI initialization for commands that don't care about the client (migrate:make and seed:make) to skip an early check for the presence of a client option, and supply a default value of sqlite3 later in the initialization process where it won't override a configured value.

[kibertoad]

Is it possible to add CLI test for this?

[dstarke]

Is it possible to add CLI test for this?

I don't think this can be done easily with the current CLI testing framework. The error this is attempting to fix only occurs when sqlite3 is not installed, and I don't see how to make that condition happen in a test.

The configuration parameter that causes the problem has (I think) no observable effect on the output of the affected commands, so the testing methodology used for other tests of the CLI -- running a command then checking its output -- doesn't work here.

In more fine-grained unit tests of the internals of the cli code, it might be possible to intercept and check the derived initialization parameters to knex based on various input and configuration setups. I don't see any existing tests of this type for the CLI currently, so figuring out how to do that that would be something of a project (for me, at least, with no prior experience in this repository).

That test would look something like this:

• create a configuration file with a client option of pg
• run the command line migrate:make command with no client option specified (or simulate running that command)
• intercept the knex initialization and check that the client option it was passed was in fact pg

[dstarke]

This issue should be fixed by #5109 . Thanks!

[dstarke]

Unfortunately, #5109 did not fix the issue, and its implementation has conflicts with this one. Also, this solution addressed a lot of issues that the proposed fix in #5111 does not.

My suggestion would be to roll back the previous change, and apply this one. If someone wants to help with a test case, I would happily include it, but the fact that the test case in the original accepted solution did not catch the error illustrates my earlier notes about how difficult it is to construct a test for this problem.

[OlivierCavadenti]

@dstarke can you rebase ? I am quite sorry about conflicts with my PR by the way.

[dstarke]

@OlivierCavadenti I'll look at it now.

[coveralls]

Coverage increased (+0.004%) to 92.277% when pulling a5055bc on dstarke:fix-cli-migrate-make-sqlite-dependency into f2a7527 on knex:master.

[dstarke]

@OlivierCavadenti Ok, I think I rebased this correctly. This should roll back only your changes to the CLI and CLI utilities, and replace them with the strategy originally proposed in my earlier implementation. The test you added is still present, but as discussed above, I don't think it actually captures this problem, and I'm not sure how to add one that does.

[OlivierCavadenti]

ok for me. Can you check @kibertoad ?

[kibertoad]

@dstarke For test: shouldn't it be fairly simple to pass some kind of a fake client and expect it to fail because fake client is preserved?

[dstarke]

@kibertoad I added some tests that work as you described. In order to get the error message propagated to output correctly, I had to wrap the commands for migrate:make and seed:make in a try/catch block to catch and exit with the error (as some other commands do).

[fuzzysaj]

Glad this is working again. Any reason why 1.0.7 is marked as "March 13" on https://knexjs.org/#changelog ?

[kibertoad]

thx, will fix