Framework: Cascading Configs #876

mjangda · 2015-11-27T02:03:46Z

Problem

We have a config file for each environment we support; that's seven different files as of now. More could be added in the future. However, because each config file lives on its own, it means that we need to duplicate entries across each of the files even though in many cases, the values are exactly the same, regardless of the environment.

(Suggested) Solution

We can start with a base config file that will contain all our shared config values. We can then merge in environment-specific config values via ${env}.json files. As an added bonus, we can allow local-only overrides via ${env}.local.json (gitignored) in case developers want to change things locally without touching the main configs.

The PR extracts the config reading bits into a new module to reduce the duplicated bits between the server config and the client config generator.

We use something like this for Mesh and it works really well.

Looking for feedback on this before I expand it any further (tests, etc.).

gziolo · 2015-11-27T06:19:55Z

config/reader.js

+		configPaths = [
+			'__base.json',
+			opts.env + '.json',
+			opts.env + '.local.json'


*.local.json seems to be a nice addition 👍

gziolo · 2015-11-27T06:44:12Z

I really like this PR! Thanks for taking care of it :)

I have one more comment. At the moment we reference config module not only in /server/ and /client/ code where we have respective config modules. The trick is that we reference config module also in /shared/ folder, which enforces to add /client/ folder to NODE_PATH for files that import config and are tested with Mocha. It would be awesome to solve also this issue some day.

Also please note that we duplicated 3 functions in config files on client and server:

config
isEnabled
anyEnabled

Of course this is a scope for another PR, but I couldn't resist to add it here :)

artpi · 2015-11-27T08:17:57Z

Mo, I really really like this idea and I was arguing for this exact approach and remember @rralian and @gwwar were against it.
As I recall, the biggest concern was that at runtime you are not really sure what is the current config
Right now you just have to look in one file

I like what you did though, but maybe we need a bit more discussion?

artpi · 2015-11-27T08:19:25Z

And by discussion I mean: waiting a bit until people wake up from food coma / long weekend :)

gziolo · 2015-11-27T08:24:36Z

As I recall, the biggest concern was that at runtime you are not really sure what is the current config
Right now you just have to look in one file

I don't think this is an issue, because client/config/regenerate.js creates one config file for you (look at client/config/index.js) :)

artpi · 2015-11-27T08:26:44Z

I see it and as I said - I like it, this is awesome!
Just pointing out what I heard :)

mjangda · 2015-12-02T17:39:41Z

Also please note that we duplicated 3 functions in config files on client and server

Yep, I resisted that in this PR but was thinking of tackling it if/after this ships :)

mjangda · 2015-12-02T17:54:20Z

If we decide that the cascading configs is a no-go, I'll just extract the config/reader bits into a separate PR as I think that's still useful.

rralian · 2015-12-02T21:43:43Z

I was arguing for this exact approach and remember @rralian and @gwwar were against it.

@artpi I don't remember such a discussion (if you can find it that would be helpful). I think we probably just misunderstood each other one way or another. I don't have any concerns jumping out at me about this. I agree that it would be nice to move reader.js out of /config so /config only contains .json files. I also would like to find a different name for reader.js. Considering the reader is a significant feature of calypso, that's bound to cause confusion. I know my first thought when looking at the list of files was "why would the reader have a separate config file?".

It looks like there are other items we could move into shared.json. Maybe that's what you meant about wanting early feedback before going further @mjangda. But languages is a huge list that could be moved into shared.json. It probably doesn't belong in config at all considering it's not something we're likely to "configure" by environment. Although maybe it makes more sense as a configuration now that Calypso is public and fork-able.

mjangda · 2015-12-02T22:33:19Z

It looks like there are other items we could move into shared.json

Yeah, I moved discover_blog_id as an example. Definitely lots more we could move to the shared file like languages, as you mention.

gziolo · 2016-01-26T13:13:29Z

Any progress on this one?

Our config file are getting out sync very quickly. See this: https://github.com/Automattic/wp-calypso/pull/2782/files. Those PR would mitigate most of the issues.

mjangda · 2016-01-29T05:29:57Z

Got sidetracked by other stuff but going to pick this up again today.

mjangda · 2016-02-02T03:13:00Z

Okay, took a another shot at this:

Config files live in /config.
Config logic now lives entirely in /server/config.
/client/config just contains the generated index.js.
Renamed reader.js to parser.js to avoid confusion.
parser.js is now pretty standalone and all environment-specific stuff has been stripped out to be passed in as args instead.
- Bonus: duplicate functions between client and server are now not duplicated anymore.
Now includes some tests for the parser (not the greatest suite, but better than nothing?).

I have not moved any other config values either. I think that should be left for other PRs.

Plus, the readme files need an update.

/cc @artpi @gziolo @rralian

rralian · 2016-02-15T16:58:06Z

@mjangda I'm sorry this has languished for so long. I just walked through everything and I think this is really great. Tests all look good and the logic makes sense. I agree that we'll want some additional documentation to explain the additional features and how to use them. But I wouldn't hold up this PR for the documentation.

One thing though, I think that feature flags should still be set specifically for each environment. I realize your PR would leave this convention in place, but it should probably be called out in the documentation that we would not want "features" to live in the _shared config ever.

Looks like the PR needs a rebase. If that's clean and good I'd give this a 👍. If the rebase is more involved, let me know and I can give this another look when you're done.

rralian · 2016-02-15T17:39:52Z

BTW, one of the reasons I'm eager for this PR is so I can enable the React visualizer for performance tuning via:

ENABLE_FEATURES=render-visualizer make run

And allow for cascading config files. This allows us to start with a base file shared across all environments, an environment-specific config file, and then local overrides.

The discover_blog_id is now defined in out base file instead of each individual config.

shared is a clearer name. We also retain the `_` to sort this file at the top of most editors / file browsers.

And pass in a config path instead of traversing

Let's keep everything related to reading/parsing/building config in one place so it's easier to maintain. This way, the client config folder can just be purely generated.

Previously, we were defining the config functions in both the module and the regenerate file. We can just reuse the definition from the module.

Since it's necessary for the parser.

Rather than accessing env vars directly in the module. Makes the module testable and less-env specific.

mjangda · 2016-02-16T17:48:07Z

Thanks @rralian! The rebase was pretty clean (just small config updates) so going to merge and will look into docs shortly.

Framework: Cascading Configs

mjangda added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it Framework [Status] In Progress labels Nov 27, 2015

gziolo reviewed Nov 27, 2015
View reviewed changes

This was referenced Jan 26, 2016

Desktop App: Sync configuration files #2782

Merged

Framework: include file with default config values #1128

Closed

mjangda force-pushed the try/config-overrides branch 2 times, most recently from 86d7782 to f948137 Compare February 2, 2016 02:49

mjangda added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Feb 2, 2016

rralian added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Feb 15, 2016

nylen mentioned this pull request Feb 15, 2016

config: alphabetize feature flags #3309

Merged

mjangda added 16 commits February 16, 2016 12:14

Extract config reading into shared lib

8b3f7e8

And allow for cascading config files. This allows us to start with a base file shared across all environments, an environment-specific config file, and then local overrides.

Ignore .local.json config files

b628623

Extract a common config value into base

c8700db

The discover_blog_id is now defined in out base file instead of each individual config.

Fix require path for root config/reader

b473e72

rename __base => _shared

cbbc840

shared is a clearer name. We also retain the `_` to sort this file at the top of most editors / file browsers.

Add warning about ES6

39d4c3d

Make some var names more explicit

ff41549

Move config reader/parser into server dir

f52e3be

And pass in a config path instead of traversing

Move regenerate with the server config

10c1912

Let's keep everything related to reading/parsing/building config in one place so it's easier to maintain. This way, the client config folder can just be purely generated.

Re-use fucntion definitions from module

b9d8bab

Previously, we were defining the config functions in both the module and the regenerate file. We can just reuse the definition from the module.

eslint: fix jsdoc

465830d

Pass configPath as its own param

3e3a9f8

Since it's necessary for the parser.

Pass ENABLE and DISABLE features into parser

92a9ab9

Rather than accessing env vars directly in the module. Makes the module testable and less-env specific.

Add some tests for parser

f2bbe03

Better assertions for enabled/disabled tests

a06193f

Rename regenerate file for more clarity

8c0fb1c

mjangda force-pushed the try/config-overrides branch from f948137 to 8c0fb1c Compare February 16, 2016 17:17

rm flickr api key from MAS config

b68047c

mjangda added a commit that referenced this pull request Feb 16, 2016

Merge pull request #876 from Automattic/try/config-overrides

f5e708f

Framework: Cascading Configs

mjangda merged commit f5e708f into master Feb 16, 2016

mjangda deleted the try/config-overrides branch February 16, 2016 17:48

scruffian removed the [Status] Ready to Merge label Feb 16, 2016

gziolo mentioned this pull request May 17, 2016

Config: Extract languages to shared config and remove obsolete muse_supported_themes #5413

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Framework: Cascading Configs #876

Framework: Cascading Configs #876

mjangda commented Nov 27, 2015

gziolo Nov 27, 2015

gziolo commented Nov 27, 2015

artpi commented Nov 27, 2015

artpi commented Nov 27, 2015

gziolo commented Nov 27, 2015

artpi commented Nov 27, 2015

mjangda commented Dec 2, 2015

mjangda commented Dec 2, 2015

rralian commented Dec 2, 2015

mjangda commented Dec 2, 2015

gziolo commented Jan 26, 2016

mjangda commented Jan 29, 2016

mjangda commented Feb 2, 2016

rralian commented Feb 15, 2016

rralian commented Feb 15, 2016

mjangda commented Feb 16, 2016

Framework: Cascading Configs #876

Framework: Cascading Configs #876

Conversation

mjangda commented Nov 27, 2015

Problem

(Suggested) Solution

gziolo Nov 27, 2015

Choose a reason for hiding this comment

gziolo commented Nov 27, 2015

artpi commented Nov 27, 2015

artpi commented Nov 27, 2015

gziolo commented Nov 27, 2015

artpi commented Nov 27, 2015

mjangda commented Dec 2, 2015

mjangda commented Dec 2, 2015

rralian commented Dec 2, 2015

mjangda commented Dec 2, 2015

gziolo commented Jan 26, 2016

mjangda commented Jan 29, 2016

mjangda commented Feb 2, 2016

rralian commented Feb 15, 2016

rralian commented Feb 15, 2016

mjangda commented Feb 16, 2016