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

fix(telegraf) ensure off-cluster influxdb url is quoted #182

Merged
merged 1 commit into from
Mar 27, 2017

Conversation

croemmich
Copy link
Contributor

When using an off-cluster influxdb, the outputs.influxdb.urls array is set improperly, as the singular config value is pulled from the secret and not quoted. See below:

Building config.toml!
Finished building toml...
###########################################
###########################################
...
[[outputs.influxdb]]
  urls = [https://influx01:8086]
  database = "deis"
  precision = "ns"
  timeout = "5s"
   username = "deis" 
   password = "..." 
 ...
###########################################
###########################################
2017/03/13 04:49:40 E! Error parsing config.toml, toml: line 17: parse error

This could be fixed a few ways. The safest and most strait-forward would be to only allow a single url and executing the | quote filter on the value. However, it appears the original author intended on keeping the ability to add multiple urls. This fix, although slightly hack-ish, allows for proper handling of the off-cluster influx configuration or the quoted default (or other) configuration.

@deis-admin
Copy link

Thanks for the contribution! Please ensure your commits follow our style guide. This code will be tested once a Deis maintainer reviews it.

@deis-bot
Copy link

@jchauncey and @sstarcher are potential reviewers of this pull request based on my analysis of git blame information. Thanks @croemmich!

@bacongobbler
Copy link
Member

related: #164

@vdice vdice requested a review from jchauncey March 13, 2017 17:25
@vdice vdice added this to the v2.13 milestone Mar 13, 2017
@jchauncey
Copy link
Member

So we are currently moving away from using our own influxdb chart to the one in the stable repo. This will allow us to more easily consume upstream changes directly from influx rather than having to incorporate them into our chart. Because of this I want to hold off on merging this change until we get the new chart in place and the migration.

@bacongobbler
Copy link
Member

ping @jchauncey, any word on the influx/grafana/telegraf migration?

@jchauncey
Copy link
Member

jchauncey commented Mar 22, 2017 via email

@jchauncey
Copy link
Member

manually tested this by the way and it looks good.

@mboersma mboersma merged commit 556bca8 into deis:master Mar 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants