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

sequelize bin support options file + migration path #1540

Merged
merged 3 commits into from Apr 5, 2014

Conversation

3 participants
@codeinvain
Contributor

codeinvain commented Mar 22, 2014

migrationsPath configuration enabled via cli
loading all cli switches via json configuration file (via --options)

codeinvain added some commits Mar 22, 2014

sequelize bin support options file + migration path
migrationsPath configured via cli

loading all cli switches via json configuration file , loaded via --options
@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Mar 23, 2014

Whats the reason for this exactly?
And do you mind providing a unit test or two aswell?

@codeinvain

This comment has been minimized.

Contributor

codeinvain commented Mar 23, 2014

There are two changes intertwined in this commit, sorry about that

  1. I'm calling the executable every time with several configuration options
    , it was tedious so instead i have one configuration file populating
    different cli options. for example my default 'config' file named
    /config/database.json
  2. I'm using a different migrations folder path /db/migrate instead of
    /migrations

added tests to test/sequelize.executable.test.js

d.

.................................................................
Daniel Cohen
Software is my life.
mobile +972 (0) 54 4799147
http://www.dnastudio.co.il
http://www.codeinvain.com
http://blogs.microsoft.co.il/blogs/danielisimo

On Sun, Mar 23, 2014 at 9:48 AM, Mick Hansen notifications@github.comwrote:

Whats the reason for this exactly?
And do you mind providing a unit test or two aswell?

Reply to this email directly or view it on GitHubhttps://github.com//pull/1540#issuecomment-38376346
.

@@ -0,0 +1,6 @@
{

This comment has been minimized.

@mickhansen

mickhansen Mar 24, 2014

Contributor

Could you please move this file to test/config instead.

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Mar 24, 2014

I'm not overly familiar with migrations, @sequelize/owners?

@sdepold

This comment has been minimized.

Member

sdepold commented Mar 24, 2014

I like the idea! Can you please move the options file for the tests and add support for setting the path to the options file via environment variable as well?

Probably OPTIONS_PATH á la:

OPTIONS_PATH=config/options.json sequelize -m
.option('-U, --url <url>', 'Database url. An alternative to a config file')
.option('-o,--cli-options <options_file>', 'Specifies lib options from file.')

This comment has been minimized.

@sdepold

sdepold Mar 24, 2014

Member

I would vote for

'-o, --options-path <options-path>' ...

@sdepold sdepold merged commit 8cc1fbf into sequelize:master Apr 5, 2014

1 check passed

default The Travis CI build passed
Details
@sdepold

This comment has been minimized.

Member

sdepold commented Apr 5, 2014

I renamed the options from cli-options to options-path and skipped the environment variable as I'm not sure if anyone needs that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment