Skip to content
This repository was archived by the owner on Sep 18, 2021. It is now read-only.

Conversation

Marak
Copy link

@Marak Marak commented Apr 15, 2012

Hi there!

This pull request contains commits that enable the use nconf for configuration of the ospriet application.

While there is nothing wrong with the current way ospriet handles configuration, I feel these changes could help increase the extendable of the application.

  1. Storing configuration data in a config.json allows third-party programs to interact with ospriet configuration
  2. nconf has built in support for pulling configuration from argument parsing ( argv ) and ENV variables
  3. nconf has the ability to dynamically read / write configuration data at run-time

see: https://github.com/flatiron/nconf

Note: A documentation update will be required if this pull request is merged. I'd be happy to update the documentation accordingly

Not sure if this will get merged, but I wanted to outline a few changes and I figured code would be the easiest way to explain.

Let me know if you have any questions or comments!

@caniszczyk
Copy link
Contributor

Any thoughts on this one @couch ?

@Marak
Copy link
Author

Marak commented Apr 21, 2012

It's more of an idea than a complete pull request. You'd probably want to tweak it a bit.

At a minimal, I think moving the configuration data to outside of the application logic ( via config.json ) is a good idea.

I've been working on some generic node.js application installer / configuration logic which operates under the assumption that the application will load configuration from an external source ( in this case, a file ).

@Marak
Copy link
Author

Marak commented Apr 22, 2012

I'm thinking, you'll probably want to create a config folder that has a production.json and a development.json file, which nconf would load based on the current process.ENV. You'll probably also want to setup a hierarchical configuration so that your config.json pulls in by default and merges in with the ENV based configuration ( nconf also can handle this )

Whenever you have time, let me know your thoughts and I'd be glad to tweak the pull request.

@dustinsenos
Copy link
Collaborator

@Marak I'm interested if your suggestions are nconf specific or just suggested changes. I'm unfortunately not familiar with nconf.

We do have a working implementation similar to what you've mentioned above. We have a config folder which has a base application.js (updated) which is loaded at application startup and then extended by a configuration file with a name that matches the environment passed through ENV.

The config folder: https://github.com/twitter/ospriet/tree/master/app/config
The logic to load our configs: https://github.com/twitter/ospriet/blob/master/server.js#L30

Is this what you're after?

@Marak
Copy link
Author

Marak commented Apr 22, 2012

Changes are to make application more configurable. nconf is good choice, since it has several really useful features related to configuration management for application.

To answer your question, no that is not what I am after.

Currently application configuration is stored in Common JS files and logic for application configuration is coupled to application logic. This is not very extendable.

Moving configuration to externally loaded JSON file allows application to be configured from external sources. Moving configuration system to use multi-transport dynamic getters ( as nconf does ) is more powerful, as you can swap out configuration source for application without having to change any code ( for instance you might want to pull application configuration from a database instead ).

The current way configuration is setup makes it hard to programmatically update configuration for application. This is very important when trying to streamline the process of application installation and configuration, as the configuration will probably be set via some sort of installation wizard ( opposed to manually editing application code source by hand ).

@Marak
Copy link
Author

Marak commented Apr 22, 2012

@dustinsenos - Just to note, you mentioned application.json file. This file is actually called application.js and is not JSON, but Common JS.

https://github.com/twitter/ospriet/blob/master/app/config/application.js

@dustinsenos
Copy link
Collaborator

@Marak that makes much more sense. Thanks for the clarification. And yes, sounds like an excellent direction to go. It appears Ospriet is reproducing a small fraction of what nsconf enables, and in a much more coupled way.

Would love to see us move in this direction. As you've mentioned an installation wizard would be great for ospriet, and it sounds like this would be excellent first steps to get there.

(My bad with the Common JS / JSON, I shouldn't have relied on memory when I typed that comment)

@Marak
Copy link
Author

Marak commented Apr 22, 2012

Cool!

I'll add some more commits to this pull request so that it's on parity with ospriet current environment detecting / merging logic, should be easy.

@couch
Copy link
Contributor

couch commented Apr 23, 2012

@Marak Apologies for the delay in response here, was wrapped up in hackweek last week. As @dustinsenos mentioned, I think adopting nconf would be great. Will watch for your latest commits and we'll go from there!

@CLAassistant
Copy link

CLAassistant commented Nov 16, 2019

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants