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

same file overrides #10

Open
lsnape opened this issue May 16, 2016 · 7 comments
Open

same file overrides #10

lsnape opened this issue May 16, 2016 · 7 comments

Comments

@lsnape
Copy link

lsnape commented May 16, 2016

Just like Nomad: https://github.com/jarohen/nomad#grouping-hosts-together---environments-041

I like this feature a lot because it means I don't have to spread my config around unnecessarily. At the moment I work around this by doing:

(cprop/load-config :resource "defaults.edn"
                   :merge [(get-in (from-resource "defaults.edn") [:environments "prod"] {})])

But of course defaults.edn gets read and parsed twice. Not the end of the world since load-config is only called once anyway, but would be nice to have this feature supported if possible.

Cheers!

@tolitius
Copy link
Owner

definitely interesting... :)

I usually use different files for different environments, and -Dconf just points to the one that is needed. But I'd like to hash out the idea on how you see this implemented.

  • an environment.edn file that cprop would look for to apply the overrides
  • a :cprop/environments key within the same config that would have dev/qa/prod overrides similar to lein profiles, which would be controlled by a CPROP_ENV variable
  • another way?

Thanks for the idea.

@lsnape
Copy link
Author

lsnape commented May 24, 2016

The 2 problems I have with storing config across multiple files:

  • Duplication. I've found that config between non-production environments is mostly the same and usually only differs by a few values. Having a default or base config, which cprop supports out of the box, solve this.
  • Visibility. It's very difficult to build a picture of all the options and switches that control an application when the config is spread out. This I've found to be a real issue when getting to grips with an unfamiliar project. There's also the increased possibility of zombie config values being left behind, which can be very unhelpful.

This is why I like the idea of having config in one place, as much as possible; if that is the end goal then you can't do much better than 2. I do, however, realise that by conflating all environments into a single file we lose some flexibility: It's not as easy to just deploy the application with its intended environment; it contains all other environments as well. Since any sensitive config ought to be set through environment variables – and cprop makes this a trivial exercise –, I'm not sure how much of a concern this is, but there may be other reasons why it's a bad idea!

I think 1 also has this issue.

There might be some experimentation to be had around adding per-key overrides. Something like this perhaps:

{:foo {:cprop/env {:test 123, :qa 456}}

I'd be interested to hear your thoughts :)

@tolitius
Copy link
Owner

conflating all environments into a single file

is what feels off. so does {:foo {:cprop/env {:test 123, :qa 456}}, since depending on config, it can get pretty unreadable and harder to maintain.

I think this should work though:

(cprop/load-config :resource "config.edn"
                   :merge [(from-resource "config.env")])

where config.env would be a different file depending on the environment (i.e. classpath) that would override appropriate values.

or

(cprop/load-config :resource "config.edn"
                   :merge [(get-in (from-resource "config.env") [:environments "prod"] {})])

in case you'd like to have a single env based file with "dev/qa/prod" values. but in this case it is decoupled from the main config.

Let me know what you think about this approach.

@lsnape
Copy link
Author

lsnape commented May 30, 2016

I think either of those suggestions is sufficient, with a slight preference towards the latter since it's easier to get a bird's-eye view of all the environment overrides. If you were worried about shipping the application with redundant environment config then the first is preferred.

Incidentally, I came across this recent blog post: https://juxt.pro/blog/posts/aero.html

After reading I'm still unsure about whether keeping overrides in the same file is actually a bad idea or not. And turns out I was completely unaware of the potential security issues of storing passwords and secrets in env vars!

@tolitius
Copy link
Owner

While trying to make configuration explicit Aero introduces an implicit DSL (#profile, #join, #env, ^:ref, #include, etc..). No real need for that.

Keeping values for all environments in the same file might have to do with taste, but I don't really like it:

  • why carry dev/qa/xyz properties in prod: could they be used by mistake?
  • moreover: at times it is prohibited to disclose any prod URLs in dev/qa in dev, in which case they become secrets and most of the env configuration will still live in a separate file

I do get the point about ENV being not secure. But let's be practical:

  • in most cases it is secure enough: anyone with an access to a prod box is already a risk. Plus the point of them not being secure is very debatable.
  • in case ENV variables is not a preferred approach, just pull the values from Consul / etcd / Zookeeper.
  • a separate config.env with secrets will work as well of course, but no need for all that implicit DSL

@lsnape
Copy link
Author

lsnape commented Jun 5, 2016

Yeah Aero's reader macros do seem like overkill, and after some reading around (thanks for the SO link) I don't think the case against environment variables is a good one.

I take the point that some organisations would find keeping prod config alongside dev/qa restrictive – a strong enough reason to promote the use of separate env files.

"in case ENV variables is not a preferred approach, just pull the values from Consul / etcd / Zookeeper."

I'm not so keen on this. If we pull config from another service that the app depends on in order to start, then that introduces a dependency on that service and the (admittedly quite unlikely) potential situation of being unable to deploy anything. DNS failures can also cause problems here.

Back to the main point. I don't think any code change is required here, but I would find some guidance in the readme about environment overrides useful.

Thanks for your input :)

@tolitius
Copy link
Owner

tolitius commented Jun 8, 2016

sure, thank you. great discussion! :)

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

No branches or pull requests

2 participants