Skip to content
This repository has been archived by the owner on Sep 25, 2020. It is now read-only.

Add datacenter support for common.json #26

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

freeqaz
Copy link

@freeqaz freeqaz commented Jun 24, 2015

Adds support for common.DATACENTER.json which is useful to reduce duplication of configuration.

r: @Raynos @lxe @rajeshsegu @malandrew

@Raynos
Copy link
Contributor

Raynos commented Jun 24, 2015

👎 what does common datacenter even mean.

datacenter is a production thing only; you only have a datacenter in production.

@freeqaz
Copy link
Author

freeqaz commented Jun 24, 2015

I keep hitting a wall when testing datacenter-specific stuff. The model that we're using makes it hard to test something, since we keep a test.json environment. So if I want to test, I've got to create test.datacenter.json and keep it in sync with production.datacenter.json. It makes it hard to ensure that the production config is well tested, because it's never actually asserted against.

Having common.datacenter.json allows production to utilize the same set of datacenter-specific config that the test environment is asserting against.

@lxe
Copy link
Contributor

lxe commented Jun 24, 2015

You can just overload the config for any wacky tests with --config=/var/config/some-file.json flag

@rajeshsegu
Copy link

I like the idea of common.pek1.json for the same reason of avoiding duplicates between both test.pek1.json and production.pek1.json.

@lxe tests needs to be written against real config data otherwise it would not be good tests.

@Raynos
Copy link
Contributor

Raynos commented Jun 24, 2015

@freeqaz test.json is a terrible idea; don't do it.

If you want to test stuff use the seedConfig feature; that's the correct way to test stuff.

We really should remove the test.json feature.

@freeqaz
Copy link
Author

freeqaz commented Jun 24, 2015

You would load production and then override the config with test-specific things?

I don't see seedConfig in the readme. I'll check out the source.

@Raynos
Copy link
Contributor

Raynos commented Jun 25, 2015

The option is called "seed". Not "seedConfig"

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.

4 participants