-
Notifications
You must be signed in to change notification settings - Fork 116
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
add option to search all nodes and services filtering by metadata #161
base: master
Are you sure you want to change the base?
Conversation
well, this is disappointing. i thought this worked. the query url gets constructed correctly, but it looks like faraday eats the nested query parameters. i'm not sure how to get faraday to accept these params but am looking. so the url gets constructed like so:
but then the consul log shows just the last node-meta param:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, than you for the PR
lgtm |
@rockpapergoat Did you succeeded into faraday accepting multiple filters? |
i got diverted from this at work for a bit and haven't revisited it, but i believe it's still an issue. the changes i made most likely won't work as expected until sorting out the faraday portion. |
looks like this will do it. i'll try to test this sometime soon. https://github.com/lostisland/faraday#changing-how-parameters-are-serialized |
yeah, that option works. i'll add something to docs to show how to use it. |
Great! Send the patch, we'll merge it |
searching for multiple metadata fields should work for nodes. i'm not sure if it works for services yet, even though it looks like it should. i don't think this requires any changes to docs, as i rolled the faraday requests option in if metadata is passed.
i updated the code. tests fail for a few ruby versions, though it looks like some of them are linting issues. do we need to fix those first? tests pass on ruby 2.5.1 here for me. |
@rockpapergoat rebase and I'll merge it |
@rockpapergoat Can you rebase this PR? Thanks) |
These small changes add options to the node and service
get_all
methods to allow including metadata filters in the URL passed to the consul API. I updated the spec tests and docs to reflect this change, but let me know if you need anything else here. Thanks!