Skip to content
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

Closed
wants to merge 4 commits into from

Conversation

martinseener
Copy link
Contributor

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

@sparrc
Copy link
Contributor

sparrc commented May 26, 2016

why do you need this? the config file clearly states that url is required: https://github.com/influxdata/telegraf/blob/master/etc/telegraf.conf#L1179

@martinseener
Copy link
Contributor Author

martinseener commented May 26, 2016

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
Copy link
Contributor

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)

@sparrc
Copy link
Contributor

sparrc commented May 26, 2016

Sure, please also update the SampleConfig function to remove the # required comment

…G feature line as well as removed #required tag in the sample config
@martinseener
Copy link
Contributor Author

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!

@sparrc
Copy link
Contributor

sparrc commented May 26, 2016

please rebase your changes off master and re-push

@sparrc
Copy link
Contributor

sparrc commented May 26, 2016

nvm, I've got it

@sparrc sparrc closed this in 42d7fc5 May 26, 2016
@martinseener
Copy link
Contributor Author

Thanks alot!!! Next time i'll do proper PRs 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RabbitMQ Input doesn't use default URL but instead gives protocol scheme error
2 participants