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: properly pass (symbolized) sequel opts #37

Merged
merged 5 commits into from
Jun 16, 2020

Conversation

kares
Copy link
Contributor

@kares kares commented Jun 16, 2020

in case of the jdbc input sequel_opts => ... were being passed with string keys (the streaming plugin symbolizes keys).
this is fine e.g. for passing sequel_opts => { "jdbc_properties" => { ... } } as sequel handles the "jdbc_properties" case but officially it's documented as :jdbc_properties.

string keys turned out problematic when trying to disable quoting using: sequel_opts => { quote_identifiers => false } the option wasn't being picked from the Hash, as Sequel only checks for :quote_identifiers key.

also took this opportunity to introduce some sharing between jdbc and jdbc_streaming plugins as I noticed some of the previously fixed issues (e.g. driver loading) could also affect the (lesser used) jdbc_streaming plugin.

@kares kares changed the title Fix: properly pass (symbolize) sequel opts Fix: properly pass (symbolized) sequel opts Jun 16, 2020
Copy link
Contributor

@robbavey robbavey left a comment

Choose a reason for hiding this comment

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

LGTM - test failure is unrelated, and is due to the travis job trying to pull resources for an expired snapshot. (logstash-plugins/.ci#13)

@kares
Copy link
Contributor Author

kares commented Jun 16, 2020

Thanks Rob, just realized this isn't enough for me to be able to set sequel_opts => { quote_identifiers => false }
As false value will come back as "false" (String) and thus considered truthy by Sequel, so need to manually handle booleans.

@kares kares requested a review from robbavey June 16, 2020 15:42
Copy link
Contributor

@robbavey robbavey left a comment

Choose a reason for hiding this comment

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

LGTM again. Thanks for adding an appropriate test

@kares kares merged commit 0c17231 into logstash-plugins:master Jun 16, 2020
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.

3 participants