search

deis / monitor

Monitoring for Deis Workflow
https://deis.com
MIT License
22 stars 32 forks source link

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

Closed croemmich closed 7 years ago

croemmich commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

bacongobbler commented 7 years ago

related: https://github.com/deis/monitor/pull/164

jchauncey commented 7 years ago

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 commented 7 years ago

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

jchauncey commented 7 years ago

It's dependent right now on a change I'm putting in helm

On Mar 22, 2017 11:55 AM, "Matthew Fisher" notifications@github.com wrote:

ping @jchauncey https://github.com/jchauncey, any word on the influx/grafana/telegraf migration?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/deis/monitor/pull/182#issuecomment-288445814, or mute the thread https://github.com/notifications/unsubscribe-auth/AAaRGNqh7ZtGWwlHNnlkY0wqhW8EOcQyks5roUSEgaJpZM4Ma3mU .

jchauncey commented 7 years ago

manually tested this by the way and it looks good.