(PIE-296) Use pe_ini_subsetting to automatically add report processor #51
Conversation
splunk_hec is a classthat may have no external impact to Forge modules. This module is declared in 2 of 575 indexed public
|
| } | ||
| # The subsetting resource automatically adds the 'splunk_hec' report | ||
| # processor to the reports setting if it hasn't yet been added there. | ||
| pe_ini_subsetting { 'enable splunk_hec': |
There was a problem hiding this comment.
We should probably have an acceptance test for this to make sure it works in practice.
There was a problem hiding this comment.
I was asked some of Principal Engineers to please not use the pe_init_settings module in modules published to the forge. They basically want that module as a private dependency for PE internal stuff only. That's one of the reasons why if you try to use this module against Pupperware, this resource will fail. It's not included in that image build because they are confident no one like us should be using it and have no intention of adding it into the container. I know we aren't using that container, but it's an indication of their attitude regarding this resource.
There was a problem hiding this comment.
@RandomNoun7 Yes, agreed. However, the module already used PE module resources (e.g. before this, it was pe_ini_setting). I left it as-is b/c I didn't want to risk breaking existing Splunk-hec users by introducing a sudden dependency to the inifile module.
We should definitely remove the PE-specific resources in the future though. Am also happy to do that now if we want to talk to Bryan about this (specifically whether the removal warrants a major release or if we can still do a minor one).
There was a problem hiding this comment.
I forgot this was splunc_hec that was already using pe_init_setting. Yeah I can agree suddenly removing it in a change like this isn't necessary.
There was a problem hiding this comment.
@ekinanp working on adapting Bills acceptance framework to use a container published by splunk. Will look into adding acceptance tests once that's up and working, so don't worry about that for now. We'll definitely get acceptance tests up before releasing.
There was a problem hiding this comment.
Kevin R. may be the best person to ask regarding the potential disruption by requiring the inifile module.
| } | ||
| # The subsetting resource automatically adds the 'splunk_hec' report | ||
| # processor to the reports setting if it hasn't yet been added there. | ||
| pe_ini_subsetting { 'enable splunk_hec': |
There was a problem hiding this comment.
Kevin R. may be the best person to ask regarding the potential disruption by requiring the inifile module.
This still works, and also fixes a (strange) bug where the 'store' report processor was being included in the list of report processors when used with a live PE environment. Signed-off-by: Enis Inan <enis.inan@puppet.com>
|
Rebased. |
|
will merge once tests pass. |
This still works, and also fixes a (strange) bug where the 'store'
report processor was being included in the list of report processors
when used with a live PE environment.
Signed-off-by: Enis Inan enis.inan@puppet.com