-
Notifications
You must be signed in to change notification settings - Fork 945
Adds support for JSON configuration values #733
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
Conversation
lib/forever/cli.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer not to add lodash as a dependency for a single function. Lets use this instead: https://github.com/sindresorhus/object-assign
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually using _.pick() as well, but I'll see what I can do.
|
Good start @tkambler. Made some comments. Could you also add test coverage for this? We need it. |
|
👍 expect some updates tomorrow. |
|
@indexzero Updates in place. |
|
Moving in the right direction! Any idea why the tests are failing? I believe |
|
Was going to ask you the same thing. Everything passes locally... |
lib/forever/cli.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I am being super picky here but just because we got rid of lodash doesn't mean we have to throw functional programming out the window 😄
jsonConfig = Object.keys(jsonConfig)
.reduce(function (acc, k) {
if (!~configKeys.indexOf(k) && k !== 'script') { acc[k] = jsonConfig[k]; }
return acc;
}, {});|
Passes locally for me as well. Very strange indeed. I wonder what Travis is doing special. I'll see if I can investigate, but if not lets get this merged anyway. The tests are in a bit of a sordid place anyway. |
|
👍 |
|
@tkambler after rereading #260, #591, and #600 a consistent theme was being able to start multiple processes from a single file. Based on your PR it seems if the JSON file was an Array this would work just fine: // my-app/development.json
[{
"uid": "app",
"append": true,
"watch": true,
"script": "index.js",
"sourceDir": "/Users/tim/Desktop/app"
}, {
"uid": "other-service",
"append": true,
"watch": true,
"script": "other-service.js",
"sourceDir": "/Users/tim/Desktop/app"
}]Thoughts? |
|
Absolutely. As a matter of fact, I had intended to submit a separate PR for precisely that once this got merged, but if you'd like me to go ahead and add support for that now, it would obviously be trivial at this point. |
|
No, I'll leave it up to you after this merges 👍 💪 💯 |
Adds support for JSON configuration values
|
Is there a way to pass command line parameters via the config.json? eg |
|
It is now: #741 |
|
This fix went out in |
This PR adds support for JSON configuration files. Consider the following example:
This PR allows this application to be started with:
It also supports the use of absolute paths:
While creating this PR and digging through the source code, it occurred to me that I'm a little unclear as to the intended use case for forever, given its current state. Forever seems to want to load global configuration values from
~/.forever/config.json; however, wouldn't this prevent forever from being used with multiple apps at the same time? What if I have five separate processes that I want to monitor?