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 error with CLI trying to select environment config for "basic configuration" #1101

Merged
merged 2 commits into from Dec 22, 2015

Conversation

@galenandrew
Copy link
Contributor

@galenandrew galenandrew commented Dec 17, 2015

Issue

Per the docs for CLI knexfile.js, the following configuration should be supported:

Basic configuration:

module.exports = {
  client: 'pg',
  connection: process.env.DATABASE_URL || {
    user: 'me',
    database: 'my_app'
  }
};

Per the following conditional block in /src/bin/cli.js#L56-L59 (introduced in #527) there is no check to make sure config[environment] exists.

  if (environment) {
    console.log('Using environment:', chalk.magenta(environment));
    config = config[environment];
  }

This results in an undefined config when using the basic configuration.

Pull Request Details

This pull request includes:

  • config assignment defaults to itself when config[environment] does not exist
  • update to the docs indicating that NODE_ENV is also used to set the environment in Knex CLI
    (this was my issue originally…I did not know the CLI environment defaults to NODE_ENV)

NOTE: I branched off master, but after following the CONTRIBUTING guidelines other files in lib showed changes after building that my code would not have affected. My assumption is that the latest build in master was dirty so I have only included src changes in this PR.

@rhys-vdw
Copy link
Member

@rhys-vdw rhys-vdw commented Dec 22, 2015

Fixes #1103.

rhys-vdw added a commit that referenced this pull request Dec 22, 2015
Fix error with CLI trying to select environment config for "basic configuration"
@rhys-vdw rhys-vdw merged commit ddafbf9 into knex:master Dec 22, 2015
@rhys-vdw
Copy link
Member

@rhys-vdw rhys-vdw commented Dec 22, 2015

@galenandrew, thanks for the PR!

re #823:

This of course made the migrations and CLI work properly (although does not follow the docs), but then introduced the pooling error because the config object being passed into require('knex')(config) included the nested environment configs.

I'm thinking that Knex() might also accept a "non-basic" config (ie. { development: .., production: ...}) for complete parity between knexfile.js and Knex(). What do you think @galenandrew, @tgriesser?

@galenandrew galenandrew deleted the galenandrew:fixEnvConfigErrorCLI branch Dec 22, 2015
@galenandrew
Copy link
Contributor Author

@galenandrew galenandrew commented Dec 22, 2015

@rhys-vdw I love that idea (updating Knex() to accept a non-basic config). What are your thoughts on having it fallback to looking for a knexfile if no config is passed into the Knex() constructor?

@rhys-vdw
Copy link
Member

@rhys-vdw rhys-vdw commented Dec 22, 2015

@galenandrew I considered that and think that it is probably not a good idea. I'd prefer for it to be supplied explicitly. IMO Knex() should create a connection-less QueryBuilder interface, rather than searching for a default connection silently. (Currently this is achieved by calling Knex({}).)

@galenandrew
Copy link
Contributor Author

@galenandrew galenandrew commented Dec 23, 2015

Ah…you make a good point.

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