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

Allow filtering by host on list_services #476

Merged
merged 4 commits into from Mar 11, 2022
Merged

Allow filtering by host on list_services #476

merged 4 commits into from Mar 11, 2022

Conversation

ghost
Copy link

@ghost ghost commented Feb 4, 2022

When using the list_services, I figured that I needed to filter the list of services received depending on the host
(i.e. only receiving the services running on xxx.xxx.0.22 for example)

I had two options :
Filtering the list post request by using discover()
-> Pro: no additional processing on the registry
-> Con: add a lot of calls to discover() so more demanding for the network, as well as slower execution time.

Filtering the list during the request by sending an additional parameter:
-> Pro: no extra charge on the network, and faster execution time.
-> Con: some processing is needed on the registry

I went with the second option since speed of execution is more critical.

@comrumino comrumino self-requested a review February 25, 2022 16:37
@comrumino comrumino self-assigned this Feb 25, 2022
@comrumino comrumino added the To Start Description reviewed and a maintainer needs "to start" triage label Feb 25, 2022
@comrumino
Copy link
Collaborator

comrumino commented Mar 10, 2022

I'd be more inclined to approve this if there are unit tests. Otherwise, I will likely put it off until I write unit tests for it. But, I do appreciate the contribution 🥇

@ghost
Copy link
Author

ghost commented Mar 10, 2022

I will add some unit tests.

@comrumino comrumino merged commit af63eae into tomerfiliba-org:master Mar 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
To Start Description reviewed and a maintainer needs "to start" triage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant