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

Another InfluxDB 0.9 PR - Fix #31 & #32 + Previous PRs #33

Closed
wants to merge 2 commits into from

Conversation

mikelaws
Copy link

Sorry to muddy the InfluxDB 0.9 waters even more... This PR includes fixes to address SSL/TLS support (http and https URLs - #32), and sprintf formatting in the DB name (#31). This is based on a fork of the pr/29 branch (everything in pr #29 and addressing #24), with all of the great work from @contentfree and @timgriffiths (including your fixes for special character handling discussed in pr #30) merged in as well.

Tests have been added to validate SSL/TLS config handling, and proper handling/buffering/flushing of events destined for a single, and multiple databases (via sprintf formatting in the configuration). Also added a test that exercises sprintf formatting in the "measurement" name. These are in addition to @timgriffiths tests, which appear to provide good coverage for his special character handling fixes and run fine in my environment.

Signed the CLA about a week ago. Currently running against InfluxDB 0.9.6, but should work against any 0.9 release, and should also be forward-compatible with InfluxDB 0.10.x.

If we can get a quick thumbs-up on the latest code changes, perhaps we can get this plugin moving forward and close several PRs and issues in the process. I see that @jordansissel has reviewed some of the earlier code changes, and suggested the bump in major version, which makes sense to me given the breaking changes and new features/fixes.

@elasticsearch-release
Copy link

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run; then say 'jenkins, test it'.

@mikelaws
Copy link
Author

Confirmed working with InfluxDB 0.10.0: able to write to sprintf-formatted database and measurement names, and via HTTP and HTTPS.

Also confirmed the question from @jordansissel on December 9th regarding nil values for @user and @password.value - InfluxDB handles the empty user/password values fine when authentication is not configured (allows anonymous access, as expected).

@mikelaws
Copy link
Author

@suyograo, I didn't realize this project didn't have a maintainer (unless I'm mistaken). Any chance someone on the Elastic side can help nudge this through the process? I'm currently running these changes in prod, but would like to be able to point folks to something more official. Thanks!

@suyograo
Copy link
Contributor

@mikelaws I just merged #29. Does this need a rebase? Can you review please?

@suyograo
Copy link
Contributor

@mikelaws Also, I had published 3.0.0 with #30. Can you bump this version?

@mikelaws mikelaws force-pushed the master branch 2 times, most recently from 6656dfc to 85bc0da Compare February 29, 2016 03:55
@mikelaws
Copy link
Author

@suyograo Thanks! Rebase done. Also bumped the version to 3.1.0 and updated the change log accordingly.

@suyograo
Copy link
Contributor

LGTM, thanks

@elasticsearch-bot
Copy link

Suyog Rao merged this into the following branches!

Branch Commits
master 0aae1fe, c437bc1

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