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

Add async configuration support. #359

Merged
merged 5 commits into from
Jan 4, 2017

Conversation

wwalser
Copy link
Contributor

@wwalser wwalser commented Sep 27, 2016

Google Cloud's infrastructure doesn't inject metadata into the environment, instead, you have to call an outside service. Because of this, I need a migration solution that offers asynchronous configuration.

This, as I've discovered over the past five hours, is surprisingly difficult primarily because of a combination of:

  1. Gulp doesn't support async configuration and doesn't plan to add it, see Gulp Issue 1505
  2. The config helper API relies on state that is internal to the module and all calls assume the initialization of that state is synchronous.

To maintain existing behavior, this change takes care to initialize config at the start of all gulp tasks but waits until readConfig is called before logging anything or showing errors.

I'm happy to make the documentation changes if that would be helpful. Would you like this in this PR or a separate one?

@wwalser
Copy link
Contributor Author

wwalser commented Sep 27, 2016

I believe I have the tests passing. I had to remove the {logging: false} option from readConfig. It was only used in one place, the version command.

I'm not sure why, but it was causing problems for a subset of tests. After removing the option, all tests pass.

@wwalser
Copy link
Contributor Author

wwalser commented Sep 28, 2016

This passed for me, flaky tests? https://travis-ci.org/wwalser/cli/builds/163213992

@validkeys
Copy link

Perfect timing. I also need for fetching a decrypted key from an external key management store prior to loading my models.

@validkeys
Copy link

Any reason we're not merging this one? Would help me out a lot!

@wwalser
Copy link
Contributor Author

wwalser commented Oct 13, 2016

I haven't added tests or documentation changes which I think would be reasonable requests by a maintainer before landing the PR. However, I'd like to get at least a nod from a maintainer to know that if I spend time on those, it will get landed.

Maybe I should mention them to get attention. Ping: @felixfbecker or @sdepold

@felixfbecker
Copy link

Im not a maintainer of the CLI :)

@wwalser
Copy link
Contributor Author

wwalser commented Oct 13, 2016

Sry mate, I saw you active in the issues area and noticed "sequelize member" in the header of your comments. Do you know whose attention we should be trying to get perhaps?

Thanks :)

@wwalser
Copy link
Contributor Author

wwalser commented Oct 24, 2016

One more try for @sdepold.

@wwalser
Copy link
Contributor Author

wwalser commented Nov 16, 2016

Okay, trying a whole new crew. How about @Americas or @sushantdhiman?

@sushantdhiman
Copy link
Contributor

Sorry for all this delay @wwalser , While this PR looks good to me generally but I am not familiar with sequelize/cli codebase. I will assign this issue to @sdepold and @Americas as they are more invloved with sequelize/cli maintenance

@Americas
Copy link
Collaborator

This PR LGTM too. And I can assure you I will merge this provided everything is good and has tests. You have our blessing 😛

@sdepold
Copy link
Member

sdepold commented Nov 22, 2016

You have my entire trust. Go ahead @sushantdhiman :)

@wwalser wwalser force-pushed the asyncConfig branch 3 times, most recently from 61878ee to 17128e6 Compare November 22, 2016 17:50
@wwalser
Copy link
Contributor Author

wwalser commented Nov 22, 2016

I've added migrate and seed tests which use async configuration. Feel free to code review and land when appropriate.

I'd like to understand the NPM release schedule as well since I'm currently using my own branch of these changes in production.

@wwalser
Copy link
Contributor Author

wwalser commented Nov 23, 2016

Would you like me to squash before you merge?

@sushantdhiman
Copy link
Contributor

Github interface allow squashing so no need. @Americas will merge after review :)

@wwalser
Copy link
Contributor Author

wwalser commented Dec 2, 2016

Ping @Americas

🎱

@wwalser
Copy link
Contributor Author

wwalser commented Dec 29, 2016

ahem… may I get this PR landed for Christmas?

🎄🎄🎅🎅🎅🏽🎅🏽🎁🎁

@wwalser
Copy link
Contributor Author

wwalser commented Jan 4, 2017

@sushantdhiman PR approved, tests written and passing. Please land.

Thanks!

@sushantdhiman sushantdhiman merged commit 0751d61 into sequelize:master Jan 4, 2017
@sushantdhiman
Copy link
Contributor

Thanks for this PR @wwalser , Sorry for this delay

codetriage-readme-bot pushed a commit to codetriage-readme-bot/cli that referenced this pull request Jun 5, 2019
Add short flag `-m` for submit comment flag
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.

6 participants