RFC: WIP: Pipeline configuration improvements #380
Conversation
|
So this allows you to declare a pipeline anywhere in your site's Puppet code. That's cool. I like it. |
| @@ -151,7 +151,7 @@ | |||
| $settings = {}, | |||
| $startup_options = {}, | |||
| $jvm_options = [], | |||
| Array $pipelines = [], | |||
| Variant[Boolean,Array[Hash,1]] $pipelines = false, | |||
| Boolean $manage_repo = true, | |||
There was a problem hiding this comment.
Would it be possible to do this without overloading the $pipelines variable? Having a variable that can be either an array or a boolean is a bit odd and it kinda goes against Puppet's continuing march toward type safety.
Also, what would happen if I declared an array of pipelines and also added another one as a defined type elsewhere? Can I do that with this implementation?
I think the ideal API would allow:
- 0 or more pipelines passed as an array to the class
- 0 or more pipelines declared as defined types
Ideally, neither of these should require a flag to "turn them on" and they should be able to co-exist. Do you think it's possible?
There was a problem hiding this comment.
The good news is that you like the idea of being able to declare pipelines outside of the main class. The tricky bit has been making the implementation backwards compatible and compatible with people not using multiple pipelines. Maybe I will need a separate flag parameter. I'll see how I can make this better. Maybe we could deprecate the old Array implementation and support passing a hash of logstash::pipeline resources to the main class instead? That's quite a common pattern for being able to create defined types from a module's base class.
Also, what would happen if I declared an array of pipelines and also added another one as a defined type elsewhere? Can I do that with this implementation?
That does work :)
|
Dear @alexjfisher, thanks for the PR! This is Vox Pupuli Tasks, your friendly Vox Pupuli Github Bot. I noticed that your pull request has CI failures. Can you please have a look at the failing CI jobs? |
|
Dear @alexjfisher, thanks for the PR! This is Vox Pupuli Tasks, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase? You can find my sourcecode at voxpupuli/vox-pupuli-tasks |
This isn't finished. If it looks like I'm going in the right direction (or can be steered that way!), I still have to add docs, examples and tests etc. Hopefully there's enough here for some early feedback. :)
I wanted a way that I could add extra pipelines without having to modify the
pipelinesparameter passed to the base class each time.This adds a defined type for adding pipelines and uses concat to write the pipelines.yml file. If you specify a
contentorsourceit'll also create thelogstash::configfilewith the correct path (path.config).It aims to be backwards compatible. (An array of pipeline configs will still be written to the pipelines.yml as before).