Load config files from a directory as well as from an initial file. #154
Conversation
| @@ -94,6 +94,8 @@ include them in your config.: | |||
| ] | |||
| } | |||
|
|
|||
| You can also pull in an entire directory of JSON configs with the `-config-dir` option. | |||
There was a problem hiding this comment.
I'd be happy if the -config flag accepted a directory instead of using a separate flag. Logstash does this today by accepting a file or directory as the config file path.
There was a problem hiding this comment.
Yep - consistency between tools is always a good thing.
There was a problem hiding this comment.
I think you can use os.Stat to check the config file exists, it should also return whether or not it's a directory (http://golang.org/pkg/os/#Stat). That way you could switch between reading a file and a directory. I would write a patch, but this is the first time I've read Go
There was a problem hiding this comment.
Don't worry about it - hoping to attack this again on the train home this afternoon :)
|
This is a feature that would be really helpful. I'm adding logstash-forwarder through my puppet config, but different servers will forward different logs, so it would be good to have a directory with syslog.json, apache.json, mysql.json etc rather than rewriting a single conf file. |
|
@mipearson thanks for your continued work on improving this project. :) |
|
No problem! New commits should resolve both issues raised. Let me know what you think. |
|
After discovering a couple of issues with this change in my own testing I decided to add a series of test cases. This code is now running in production in @bikeexchange. |
|
Hi Jordan - anything else you want me to do on this branch before you can merge it in? |
|
Any progress on this pull request? This feature would be really good with logging inside docker since we can add the main config for the forwarder in the base container while app containers can then include there own config with tagging etc to route logs correctly. |
|
Hi, If your can't wait for Jordan to merge this, we are using this branch already in production with no issues. Sent from my iPad
|
|
this is a very useful enhancement, any progress on it? I was thinking to do it myself but this one is good enough. |
|
It's complete and already in use in a production environment, just waiting on a merge. Sent from my iPad
|
|
@jordansissel what's happening with this merge? I'd really like to bring my local version of logstash-forwarder in line with master so that I can debug our other logstash issues from a known state. |
|
The status is two things - One, so far we've got folks saying "I Like this idea" but almost nobody actually saying "I tested this in production and it was awesome". Second, that I haven't tested this myself, either. I'll try to make time for this now. |
|
Manual test notes:
As discussed in IRC, can you rebase this against master (and preferrably squash to a single commit?). Also toss things through gofmt if you haven't already :) |
|
Commits squashed, empty files now ignored. Note that one of your new tests was a duplicate of mine. I've favoured mine, as it also tests for comment stripping and is shorter. |
|
This looks good - but I question why we allow network configuration in multiple files. It gives the potential false impression that the network config in one config file only applies to file definitions in that file. Maybe it should only be the files definitions which are allowed as those accumulate. Whereby the network config is merged and that just feels non-intuitive and very likely superfluous to requirements. |
|
Partly, we don't - any attempt to supply the network configuration twice will result in an error. Secondly - it was easier to implement it this way rather than enforcing "you may only specify the network in one file". Thirdly - this matches how apache / etc configs work, in that it doesn't matter where a configuration variable is specified, it'll get loaded in regardless. |
|
Apache you need to use "Include" to reference additional configs. So you have a main config and additionals which are distinct. The v host behaviour locks stuff to IPs etc so you generally don't get much undesired bleeding of config. Here you would bleed the servers config across files so they potentially affect all other configs by having that server selected instead. I guess my concern stems from the -config - there is no main config file that could have network config. So the network config has to go somewhere. I hadn't thought of that, sorry, so I can now appreciate there's no simple way to force network config to a specific file, since none are specific, with this design. The way I did it in Log Courier was added an "includes" section to main config with globs for additional file configs. Those files contained only the array segment It can always be improved down the line! Just thought I'd throw my thoughts in the pot. 👍 |
|
Yep, understood. Originally the change had separate "--config" and On Thu, Jul 10, 2014 at 6:32 PM, driskell notifications@github.com wrote:
Michael Pearson |
|
hey @jordansissel, this version is now running in production on ~5 hosts. |
|
Fixed merge with master. |
|
What's holding this up from being merged to master? |
|
More testing & doing the actual merge process. |
|
What's the definition of "more testing"? One more person trying it out locally? Ten? |
|
More than just me and @mipearson would be sweet :P |
|
Or I can just merge it without more feedback. |
|
I am will to bet my career and online reputation on the fact that it works fine on my machine. YMMV, no warranty implied (even under local laws), may cause retinal scarring and birth defects in men between the ages of 31 and 32.5. |
|
I am already using this in production: |
|
@mipearson haha; got it! No warranty for any retinal scars. :P |
|
I'll work on merging this tomorrow! @mipearson if you rebase this on master before then, that'd be sweet, but otherwise I'll do the merging madness :) |
|
got config.go rebased; working on logstash-forwarder.go as time permits |
|
Ah, I was planning on attacking that a bit later today. Regards, Michael Pearson
|
* Load multiple configuration files from a directory * Ignore empty configuration files * Filter '#' style comments from JSON configs
mipearson
force-pushed
the
MultipleConfigFiles
branch
from
0d042b3
to
a1fe078
Compare
|
Okay, fully rebased. That was painful :( |
Load config files from a directory as well as from an initial file.
|
@mipearson your intense efforts are much appreciated. <3 |
Ref #136
Naive implementation - once I'm more comfortable with Go testing I can add some tests.