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

Override knexfile options with CLI options #4047

Merged
merged 13 commits into from Mar 30, 2022

Conversation

KoryNunn
Copy link
Contributor

This PR allows overriding options in a passed knexfile with other options passed to the CLI.

@kibertoad
Copy link
Collaborator

There's a test failing, could you please take a look?

@kibertoad kibertoad closed this Oct 5, 2020
@kibertoad kibertoad reopened this Oct 5, 2020
@kibertoad
Copy link
Collaborator

@KoryNunn Could you please also update documentation in https://github.com/knex/documentation?

config.connection = opts.connection;
}

if (opts.migrationsDirectory) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if there is a way to make all overrides case-insensitive. Probably we can do check on lowercased entry pairs.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I can understand, the goal is to lower case the properties in options object before merge with config in knexfile ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But some options are camelCase, no?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmh, I will check that this week.

@kibertoad
Copy link
Collaborator

@KoryNunn Do you need any help with this?

@OlivierCavadenti OlivierCavadenti self-assigned this Jan 19, 2022
@KoryNunn
Copy link
Contributor Author

Hi just a quick update. I no longer use knex, and as such have no capability for valuable input. I'll leave this PR open in case it is of use to anyone else, but also not against it being closed.

@OlivierCavadenti
Copy link
Collaborator

Hi just a quick update. I no longer use knex, and as such have no capability for valuable input. I'll leave this PR open in case it is of use to anyone else, but also not against it being closed.

It's ok no problem at all, I will finish it !

@OlivierCavadenti
Copy link
Collaborator

@kibertoad
Doc here knex/documentation#395
By the way, I assume I can also close #5006 which I have opened but it's a duplicate of this one ?
For your comment with camelCase, I made some tests, but since cli options with commander have fixed case and transformed in camelCase properties, I don't see any problems, and unit test is ok. Maybe I miss something ?

@kibertoad
Copy link
Collaborator

Yeah, looks good, and we can close 5006 too.

@OlivierCavadenti OlivierCavadenti merged commit 0232bec into knex:master Mar 30, 2022
@kibertoad
Copy link
Collaborator

Released in 1.0.5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants