Skip to content

Conversation

@manicolosi
Copy link

We suggest that this should become version 1.0.0.

image

@jakemcc
Copy link
Contributor

jakemcc commented May 27, 2015

Not going to pretend I've looked at all of this yet but could this have been implemented similar to the config-remote extension?

@manicolosi
Copy link
Author

Hmm. Not sure what the config-remote extension is all about, but if you're asking whether we could have split the etcd support into a separate extension library, then I think we can. When we began there wasn't the extension points needed to do that, but I think we landed on having that.

If lots of people feel strongly that it should be a separate extension library, then we can split it out.

@jakemcc
Copy link
Contributor

jakemcc commented May 28, 2015

I believe config-remote was added so config values could be pulled from a file served by nginx. My understanding is that it ended up as an extension since it didn't completely fit with being a general config library.

I don't feel strongly about where etcd should belong. It was more of a question based off just a quick glance through. I didn't have a great idea of what all was added.

Does this basically make the tag #config/etcd available in config.edn files and those are looked up from an etcd instance?

@jakemcc
Copy link
Contributor

jakemcc commented May 28, 2015

I like the change of getting rid of defconfig! and having them always required unless a default is given.

@manicolosi
Copy link
Author

@jakemcc: It adds the #config/etcd reader tag that can be used in edn files (or #config/env with #config/edn, etc). It also allows etcd to be a "source" for configuration instead of a config.edn file. Those are the two major things anyway.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multiple derefing like this is not thread safe. The value of the atom could be changed between your derefs.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch. :)

@pjstadig
Copy link
Contributor

There's a lot going on here including several breaking changes:

  1. The #config/lookup tag.
  2. Requiring every config to be derefed. (breaking)
  3. Pulling configs dynamically at runtime. (breaking; runs against the current model/philosophy of the library)
  4. Adding etcd as a source.
  5. Changing the semantics of defconfig so that values are always required. (breaking)
  6. A bootstrap main namespace for loading an edn file into etcd.
  7. Removal of the outpace.config.bootstrap namespace. (breaking) Does the code in this PR allow for loading config from the classpath? As I recall that was the main motivator for adding the bootstrapping.

I'd prefer to see this broken up into several PRs with smaller change sets so we can discuss each change separately.

@manicolosi
Copy link
Author

Discussed with @pjstadig and we've decided to close this for now.

@manicolosi manicolosi closed this May 28, 2015
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

Successfully merging this pull request may close these issues.

5 participants