-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Fixes #1277 Use the DefaultURL parameter if no url is explicitly set by the user #1278
Conversation
why do you need this? the config file clearly states that |
It does, yes. But why do you specify a DefaultURL in the go source when not using it. It also simplifies usage of the plugin for everyone significantly since most users will run this input plugin locally on the rabbit instance and are fine with the default, so they can save time for adding configuration! So you should either accept this PR (would appreciate it) or remove the unused DefaultURL parameter from the source if its never been used. Thanks ;) Edit: I only made this change to make the usage of this plugin easier!! If you want that i change the telegraf.conf to reflect that url is now optional i can add this to this PR too! (As well for the changelog, sorry i forgot that in first place) |
@@ -146,7 +146,11 @@ func (r *RabbitMQ) Gather(acc telegraf.Accumulator) error { | |||
} | |||
|
|||
func (r *RabbitMQ) requestJSON(u string, target interface{}) error { | |||
u = fmt.Sprintf("%s%s", r.URL, u) | |||
url := r.URL |
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.
change to:
if r.URL = "" {
r.URL = DefaultURL
}
u = fmt.Sprintf("%s%s", r.URL, u)
Sure, please also update the SampleConfig function to remove the |
…G feature line as well as removed #required tag in the sample config
I've made the changes and hope i didn't miss anything. I've also already signed the CLA before i made the PR. Thank you! |
please rebase your changes off master and re-push |
nvm, I've got it |
Thanks alot!!! Next time i'll do proper PRs 👍 |
This fixes #1277 hopefully by checking if a url is set by the user and if not the DefaultURL is used which is fine for lots of RMQ installations