-
Notifications
You must be signed in to change notification settings - Fork 102
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
feat(service): Add docker service ls --filter option #561
feat(service): Add docker service ls --filter option #561
Conversation
c9c5e1e
to
c54749c
Compare
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.
Many thanks for the pull, request, this is a cool feature. We just need a few changes and it will good to merge :)
@@ -353,13 +353,14 @@ def logs( | |||
else: | |||
return "".join(x[1].decode() for x in iterator) | |||
|
|||
def list(self) -> List[Service]: | |||
def list(self, filters: Dict[str, str] = {}) -> List[Service]: |
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.
Can you update the docstring with an example? Thanks!
@@ -45,6 +45,25 @@ def test_get_list_of_services(docker_client: DockerClient): | |||
assert [my_service] == list_of_services | |||
|
|||
|
|||
@pytest.mark.usefixtures("swarm_mode") | |||
def test_filters(docker_client: DockerClient): |
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.
Can you try to filter with a label that does not exist and verify that no services are returned? That would ensure that the function is correctly removing services from the result.
c54749c
to
6f65fb6
Compare
@gabrieldemarmiesse thanks for your quick feedback. Done, let me know if everything is ok. |
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.
Many thanks!
Head branch was pushed to by a user without write access
6f65fb6
to
9ffdd5e
Compare
@gabrieldemarmiesse i've had to rebase because it wasn't up to date. Can you enable auto-merge again? |
e45f0cd
into
gabrieldemarmiesse:master
docker service ls --filter option