-
Notifications
You must be signed in to change notification settings - Fork 25
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
(PIE-296) Add option to dynamically calculate the 'reports' setting #49
Conversation
splunk_hec is a classthat may have no external impact to Forge modules. This module is declared in 2 of 575 indexed public
|
|
@gsparks Do I need to update any docs with these changes? If so, could you point me to them? |
| @@ -29,12 +29,37 @@ | |||
| ) { | |||
|
|
|||
| if $enable_reports { | |||
| if $reports != '' { | |||
| $reports_setting = $reports | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a deprecation warning here re: the $reports parameter? @bmjen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, let's go ahead and add the warning.
|
We should update the README, |
|
If I'm understanding this right the report processor will now automatically be installed in the PE console when the module is installed rather than having to manually add the splunk_hec class under the PE master. The open source installation steps should be the same except now you don't have to manually list splunk_hec. Do I have that right? |
|
@gsparks Install instructions are the same, the only difference is that if you specify The report processor isn't an open source feature b/c it uses the |
|
@gsparks Nevermind my comment about reports being a PE-only feature. Looks like having the module set up |
This option is enabled when $reports == '' for backwards compatibility. The option's useful because it no longer requires the user to specify _all_ of their report processors in the $reports param. This leads to a better UX especially when the user wants to use another report processor module because they do not have to update the $reports parameter to include that module's report processor. Note that the $reports parameter will eventually be deprecated. Signed-off-by: Enis Inan <enis.inan@puppet.com>
|
@gsparks Updated. |
|
Looks good to me. We can have Barker review if you want, otherwise I'm good to merge. |
|
Yeah I think a review from Barker would be useful. |
|
Actually I think he's on leave. |
|
Ok, feel free to merge then. I think change should be OK b/c it doesn't break anything. |
This option is enabled when $reports == '' for backwards compatibility.
The option's useful because it no longer requires the user to specify
all of their report processors in the $reports param. This leads to a
better UX especially when the user wants to use another report processor
module because they do not have to update the $reports parameter to
include that module's report processor.
Note that the $reports parameter will eventually be deprecated.
Signed-off-by: Enis Inan enis.inan@puppet.com