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

add option to search all nodes and services filtering by metadata #161

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

Conversation

rockpapergoat
Copy link

@rockpapergoat rockpapergoat commented Jan 27, 2018

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!

@rockpapergoat
Copy link
Author

rockpapergoat commented Feb 8, 2018

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:

nodes = Diplomat::Node.get_all(meta: {role: 'foo', az: 'us-east-1d'})
"/v1/catalog/nodes?node-meta=role:foo&node-meta=az:us-east-1d"

but then the consul log shows just the last node-meta param:

2018/02/08 15:47:39 [DEBUG] http: Request GET /v1/catalog/nodes?node-meta=az%3Aus-east-1d (131.244µs) from=127.0.0.1:56308

Copy link
Member

@pierresouchay pierresouchay left a 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

@kamaradclimber
Copy link
Contributor

lgtm

@pierresouchay
Copy link
Member

@rockpapergoat Did you succeeded into faraday accepting multiple filters?

@rockpapergoat
Copy link
Author

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.

@rockpapergoat
Copy link
Author

looks like this will do it. i'll try to test this sometime soon.

https://github.com/lostisland/faraday#changing-how-parameters-are-serialized

@rockpapergoat
Copy link
Author

yeah, that option works. i'll add something to docs to show how to use it.

@pierresouchay
Copy link
Member

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.
@rockpapergoat
Copy link
Author

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.

@pierresouchay pierresouchay added the needs-rebase Needs rebase the PR label Dec 19, 2018
@pierresouchay
Copy link
Member

@rockpapergoat rebase and I'll merge it

@Jesterovskiy
Copy link
Contributor

@rockpapergoat Can you rebase this PR? Thanks)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-rebase Needs rebase the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants