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

+ added MongoDb checker #6

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

Conversation

LouAdrien
Copy link

Hello, I created a checker to get configuration from a mongodb collection.
I added minimal documentation and test, if it proves useful to you i might work more onto adding a docker container with the test data and / or merging it to this fork to make it embedded directly.

Tell me if anything is not in order

@@ -83,6 +83,8 @@ If you notice something that you feel is broken or missing in configure feel fre
|JSON|***[builtin]*** https://github.com/paked/configure |`NewJSON(io.Reader)`| Retrieves values from an `io.Reader` containing JSON |
|Flag|***[builtin]*** https://github.com/paked/configure |`NewFlag()`| Retrieve flagged values from `os.Args` in a `--x=y` format|
|HCL|***[builtin]*** https://github.com/paked/configure |`NewHCL(io.Reader)`| Retrieve values from an `io.Reader` containing HCL|
|MongoDb|***[external]*** https://github.com/LouAdrien/configuremongo |`NewMongo("mongodb://127.0.0.1:27017/test_conf_db","confs")`| Retrieve values from an `io.Reader` containing HCL|
Copy link
Owner

Choose a reason for hiding this comment

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

Just a couple things to fix here:

  1. replace NewMongo("mongodb://127.0.0.1:27017/test_conf_db","confs") with NewMongo(dburl, dbname)
  2. add a proper description to replace Retrieve values from an io.Reader containing HCL. Maybe something like retrieve values from a MongoDB database

@paked
Copy link
Owner

paked commented Mar 6, 2018

I just wanted to say: it's awesome that you're using configure, I hope it's serving you well.

I've left a comment above about the contents of this PR, once it's been addressed we'll be good to go.

Personally, I don't think it'd make sense to merge your MongoDB checker into the main library -- I'd really like to keep external dependencies to a minimum (we don't have any vendor lock files checked in at the moment and I'd like to keep it that way. Nice and simple.).

I realise in the past that I approved the HCL checker being merged into here, but I guess my stance has changed in retrospect (I won't be removing it, in case someone else is currently relying on it. If someone ever overhauls the library at all and it makes sense to draw a line in the sand for "version 2" it might be something to reconsider then).

@LouAdrien
Copy link
Author

Hello, sorry I was in a rush an went to quickly on the readme update... Will update this properly soon.

As you wish for the merging into the main library. I agree the dependencies involved might be too much... However i think it's just a download issue, the compiler will probably not include the unused dependencies so the binary size won't be affected if they re not used.
On the other hand, considering the current relatively "small" size of this project, I think that effort for people to actually include another repo just for this use case might be a bit too much.

Just a thought :)

@paked
Copy link
Owner

paked commented Mar 7, 2018

Ping me when you're ready for another review.

RE minimising dependencies. it's not so much about output binary size, but instead:

  1. preventing something like left-pad happening.
  2. keeping the library being completely go getable, no need to worry about dependency locking
  3. making sure the library is maintainable going into the future. the smaller and more concise the codebase is the better.

I don't think adding a dependency on your GitHub project is too much of a stretch for people who want to have MongoDB checkers.

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.

2 participants