fix(inputs.puppetagent): Update default summary path in puppetagent#19257
fix(inputs.puppetagent): Update default summary path in puppetagent#19257Mihitoko wants to merge 2 commits into
Conversation
31bffee to
0f774fb
Compare
|
Note: the previous lint job failed and from the build output it tried to post a comment. |
skartikey
left a comment
There was a problem hiding this comment.
plugins/inputs/puppetagent/puppetagent.go:89-90 - the plugin still falls back to the old path (/var/lib/puppet/state/last_run_summary.yaml) when location is not set, so after this PR the documented example and the actual default behavior diverge: a user with a bare [[inputs.puppetagent]] section gets the old path while the docs show the new one. Please update the fallback in Init in this same PR to the same value the sample shows, and given that the official AIO packages default publicdir to /opt/puppetlabs/puppet/public (PUP-10627) and are how most Puppet 7/8 deployments are installed, I would make that shared value /opt/puppetlabs/puppet/public/last_run_summary.yaml (see item 2). Since this changes behavior the PR should then be retitled fix(inputs.puppetagent): .... The risk window is small: on any Puppet 7+ host the old fallback already fails with a stat error, and Puppet 6 is long EOL.
| ## Location of puppet last run summary file | ||
| location = "/var/lib/puppet/state/last_run_summary.yaml" | ||
| ## Location of Puppet last run summary file (Puppet default: $publicdir/last_run_summary.yaml) | ||
| location = "/var/cache/puppet/public/last_run_summary.yaml" |
There was a problem hiding this comment.
the concrete example uses the distro-packaged flavor (/var/cache/puppet/public/...); per item 1 I would switch it to the AIO path /opt/puppetlabs/puppet/public/last_run_summary.yaml. The $publicdir parenthetical already covers users on other packagings; if desired, a single README prose line can mention the distro-packaged variant.
0f774fb to
2c467cb
Compare
|
Download PR build artifacts for linux_amd64.tar.gz, darwin_arm64.tar.gz, and windows_amd64.zip. 📦 Click here to get additional PR build artifactsArtifact URLs |
|
Hey 👋 Also i decided to make the statement more generic by saying that $publicdir might be different rather then hard mentioning the path the debian package uses, because it really seems highly dependent on how the distro packages puppet. |
srebhan
left a comment
There was a problem hiding this comment.
Thanks for your contribution @Mihitoko! Unfortunately, we cannot update the default location as we try do guarantee backward compatibility in Telegraf and your change would break everyone using a Puppet version before 7!
What I would like to see instead is a comment in the sample.conf file saying
## Location of puppet last run summary file
## Use the following path for puppet version 7+ ($publicdir/last_run_summary.yaml)
## location = "/opt/puppetlabs/puppet/public/last_run_summary.yaml"
# location = "/var/lib/puppet/state/last_run_summary.yaml"
Summary
This fixes #19256.
Update the path to the new default to make it clear which file is meant.
I also added a hint to necessary puppet config adjustments to get the plugin to work correctly.
Checklist
Related issues
resolves #19256