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

Adding InfluxDB 0.9 Support #13

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

berglh
Copy link
Contributor

@berglh berglh commented Jul 27, 2015

I moved to InfluxDB version 0.9 last week and updated this container to work with the new version. I've left the default version as InfluxDB 0.8 for the meantime, as applications like Grafana are still finalising 0.9 support. The plugin for StatsD does have full support for 0.9 now, so I thought it was worth updating this package. The changes I made for this commit are:

  • Updated to node:0.12.7-slim container
  • Updated to npm plugin statsd-influxdb-backend v0.60 for InfluxDB v0.9 support
  • Added generic-pool plugin for node as it was missing in node:0.12.7-slim
  • Added git install to Dockerfile as it was not inbuilt in node:0.12.7 slim
  • Added evironmental variables to config.js for InfluxDB v0.9 support
  • Updated README.md with additional information related to InfluxDB versions and new environment variables

I've tested this container to collect stats and visualise the data with Grafana 2.0.2 using the preliminary InfluxDB v0.9 support. Everything is working correctly.

Do you think it might be worthwhile adding a changelog and git tags to track the version changes? It's pretty simple container so it might not be worthwhile.

@premist
Copy link
Member

premist commented Sep 11, 2015

Hi @berglh, Thank you so much for your pull request! 👍 My apologies for checking this late.
It seems like the latest version of the InfluxDB is 0.9.3. If you can check that this works with the 0.9.3, I'll go ahead and merge this.

@@ -20,6 +20,8 @@
influxdb: {
host: host,
port: port,
version: process.env.INFLUXDB_VERSION || "0.8",
ssl: process.env.INFLUXDB_SSL || "false",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tested this PR with influxdb 0.9.5 and it works great for me except for the line that sets the ssl value. It needs to look like the following code instead so the final value is a boolean rather than a string.

      ssl: (process.env.INFLUXDB_SSL || "false").toLowerCase() === "true",

@ryanleary
Copy link

Can we get this merged in?

@premist
Copy link
Member

premist commented Feb 22, 2016

Hi @berglh, can you confirm that this works on the latest version of InfluxDB? According to @jeremiahsnapp's comment, it seems like some adjustments are needed.

@berglh
Copy link
Contributor Author

berglh commented Mar 8, 2016

Sorry I missed this notification. It has been working for me for quite sometime, however, I have not tested the SSL functionality of the config as @jeremiahsnapp has pointed out. Our InfluxDB instance is not configure to use SSL, so I would need to do a lot of modification to verify this.

…to node:4.1.1-slim and Docker tested in Docker 1.10
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.

4 participants