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

Projects
None yet
2 participants
@galenandrew
Contributor

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

This comment has been minimized.

Collaborator

rhys-vdw commented Dec 22, 2015

Fixes #1103.

rhys-vdw added a commit that referenced this pull request Dec 22, 2015

Merge pull request #1101 from galenandrew/fixEnvConfigErrorCLI
Fix error with CLI trying to select environment config for "basic configuration"

@rhys-vdw rhys-vdw merged commit ddafbf9 into tgriesser:master Dec 22, 2015

@rhys-vdw

This comment has been minimized.

Collaborator

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

This comment has been minimized.

Contributor

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

This comment has been minimized.

Collaborator

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

This comment has been minimized.

Contributor

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