Skip to content

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

Open
rockpapergoat wants to merge 4 commits into
WeAreFarmGeek:masterfrom
rockpapergoat:master
Open

add option to search all nodes and services filtering by metadata#161
rockpapergoat wants to merge 4 commits into
WeAreFarmGeek:masterfrom
rockpapergoat:master

Conversation

@rockpapergoat

@rockpapergoat rockpapergoat commented Jan 27, 2018

Copy link
Copy Markdown

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

rockpapergoat commented Feb 8, 2018

Copy link
Copy Markdown
Author

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

@pierresouchay pierresouchay left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
Contributor

lgtm

@pierresouchay

Copy link
Copy Markdown
Member

@rockpapergoat Did you succeeded into faraday accepting multiple filters?

@rockpapergoat

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Author

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

@pierresouchay

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

@rockpapergoat rebase and I'll merge it

@Jesterovskiy

Copy link
Copy Markdown
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