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 container names to be supplied to weave ps #526

Merged
merged 2 commits into from Apr 10, 2015

Conversation

bboreham
Copy link
Contributor

@bboreham bboreham commented Apr 8, 2015

So, weave ps will still show you all containers, but weave ps mycontainer will just show you that one.

If you give a nonexistent name you get no output, which is consistent with Unix ps.

@inercia
Copy link
Contributor

inercia commented Apr 8, 2015

+1

@rade
Copy link
Member

rade commented Apr 8, 2015

I am not completely sold in this. docker ps doesn't take a container argument. And filtering by container id is just one 'grep' away. So what's the rationale here? Performance?

@bboreham
Copy link
Contributor Author

bboreham commented Apr 8, 2015

I needed something like this to run smoke-tests for IPAM.

docker ps does have a --filter name=foo option (on master).

But yes, performance. In order to grep the weave ps output you need the short form of the container hex ID, which needs a docker inspect, possibly remotely. Also, it's much more inefficient to obtain details on every container then grep out the ones you don't need.

@rade
Copy link
Member

rade commented Apr 8, 2015

ok. rationale accepted.

AFAICT, when this is fed a name, the output will contain that name, not the container id. Shouldn't the output always contain the id?

Docs could perhaps do with a sentence mentioning the extra functionality.

@bboreham
Copy link
Contributor Author

While putting the hex ID in the output would be consistent, it seems rather a complex implementation to add for little benefit. If the caller of weave ps knows the name of a container and doesn't know the ID, then has to do extra work to be able to match them up.

@bboreham
Copy link
Contributor Author

Doc change added.

rade added a commit that referenced this pull request Apr 10, 2015
Allow container names/ids to be supplied to `weave ps`
@rade rade merged commit 3a83570 into weaveworks:master Apr 10, 2015
@rade rade modified the milestone: 0.10.0 Apr 18, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants