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

Proposal: docker ps: add fields for ordering and column selection #10255

Closed
wants to merge 2 commits into from

Conversation

codemac
Copy link
Contributor

@codemac codemac commented Jan 22, 2015

  • api/client/commands.go: Refactor CmdPs to use a fields list of
    characters to determine which columns to print on docker ps
    invocation.

This adds an ability for the docker command to print the columns of
output in arbitrary order.

This is nominally related to #7477

Signed-off-by: Jeff Mickey j@codemac.net

@codemac
Copy link
Contributor Author

codemac commented Jan 22, 2015

This is my first contribution to the docker project, I apologize for any errors.

This adds a -fields to docker ps that allows you to choose which, and in what order the columns are printed. Haven't finished testing locally yet, but wanted to see if this is the completely wrong direction. This code is small and simple, so any feedback including rewrites is appreciated.

@jessfraz
Copy link
Contributor

Thanks for your first contribution! We are super excited to have you! Now since your PR is changing the UX of docker ps it is going to need UX review. Seeing as we are in the middle of the release process, about to make release candidate binaries and try and test and fix all bugs that are found it might take a little longer for us to get to this PR. But we promise we are here and this seems really awesome. Just don't want you to be concerned if there is a slight lull in review :)

@jessfraz jessfraz added the UX label Jan 22, 2015
@codemac
Copy link
Contributor Author

codemac commented Jan 22, 2015

Yea, more than anything the type of feedback I'd like is if this is the approach people agree with.

The amount of docker ps | tail -n +2 | awk '{print $NF}' occurring in my life is too damn high :)

Thank you for the prompt response!

@jessfraz jessfraz changed the title docker ps: add fields for ordering and column selection Proposal: docker ps: add fields for ordering and column selection Jan 22, 2015
@codemac
Copy link
Contributor Author

codemac commented Feb 18, 2015

Please let me know what thoughts are, I can rebase/update if necessary.

@SvenDowideit
Copy link
Contributor

oh +1 - I was just wanting this for docker image too

@codemac
Copy link
Contributor Author

codemac commented Mar 13, 2015

This is my monthly ping!

Also - the windows builder failed pretty fantastically, on all kinds of things that don't look like my fault.

@LK4D4
Copy link
Contributor

LK4D4 commented Mar 31, 2015

Code is little unreadable with all this next stuff, but idea is okay.
@tiborvass should we move to code-review?

@tiborvass
Copy link
Contributor

@LK4D4 although I understand the idea and the reason, I'm not 100% sure we want this (not saying I'm sure we don't want this :P )

Ping @crosbymichael @jfrazelle @estesp @duglin @icecrime

@icecrime
Copy link
Contributor

I personally don't feel a need for this, so I'm a "weak no".

@thaJeztah
Copy link
Member

Different proposal, but just for showing (selected) labels in docker ps #11943 might be good to compare the use-cases and see if they make sense together?

}

if *size {
*fields = *fields + "z"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you need to make sure 'z' isn't already in there?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, do you purposely want to allow --fields=cccc ? Guess if that's what the user wants.....

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was my consideration that it could be desired, but doesn't seem in line with what I think a user would suspect. I'll uniq the letters that are set in *fields, choosing the earliest existence.

@duglin
Copy link
Contributor

duglin commented Apr 4, 2015

I actually like the idea and wouldn't mind seeing it on other tabular-output commands.
Major re-base is needed though - sorry - lots of changes these days.

@duglin
Copy link
Contributor

duglin commented Apr 7, 2015

I guess before I make more comments on the code we need to exit design review :-)
I'm +1 on it. @tiborvass ?

@thaJeztah
Copy link
Member

I wonder if we only want the shorthand (single-chars) to specify the columns, I can imagine they will be hard to remember without checking the help/man. Perhaps also --fields=id,image,status

Would using a comma-separated list be good idea in general (also for single-chars), as it leaves more room for future expansion (I think)

@tiborvass
Copy link
Contributor

Still not sure if I want this. But if I did, I agree with @thaJeztah on separating actual words with commas. I don't like the single-chars.

Even though unix philosophy would say no to this PR, I agree it's not as easy to awk the output. One could say just use the API though. Maybe there's room for this. I just hope we're not opening a can of worms where people can point to this PR saying "but he got to have his --fields PR merged".

@icecrime icecrime added the status/needs-attention Calls for a collective discussion during a review session label Apr 21, 2015
@duglin
Copy link
Contributor

duglin commented Apr 29, 2015

@tiborvass can you elaborate on why on you think the unix philosophy would say no to this PR? The linx 'ps' command itself has this feature.

@cpuguy83
Copy link
Member

How about just docker ps --json to spit out the literally json data instead of building the table?

* api/client/ps.go: Refactor CmdPs to use a fields list of
  characters to determine which columns to print on `docker ps`
  invocation.

This adds an ability for the docker command to print the columns of
output in arbitrary order.

Signed-off-by: Jeff Mickey <j@codemac.net>
@aluzzardi
Copy link
Member

What I'd like to see from this:

  • Labels: As in, add columns based on a user specified labels
  • Naming. Right now, columns are mapped based on hardcoded names, which is fine. I'd like to be able to rename them (as in, CONTAINER ID -> ID) and this is especially true for labels.

Use case:

docker ps --fields "c=ID,com.docker.swarm.id=Swarm ID,m"

/cc @vieux

@codemac
Copy link
Contributor Author

codemac commented May 19, 2015

@aluzzardi see 59c4561

@codemac codemac force-pushed the dockerps branch 3 times, most recently from 5ac2690 to 1e11070 Compare May 19, 2015 00:17
* api/client/ps.go: named label support

Signed-off-by: Jeff Mickey <j@codemac.net>
@codemac
Copy link
Contributor Author

codemac commented May 19, 2015

Well, I updated for naming the columns, but not any epic --format, --template or other work. I'd like to see a clear description of what is wanted from the steak-holders in this docker project who actually have a say. I've updated to mostly include what @aluzzardi asked for.

@aluzzardi
Copy link
Member

Thanks @codemac. The only missing requirement we (Swarm) have is to include container labels.

ping @docker/core-maintainers @vieux for feedback

@calavera
Copy link
Contributor

As @thaJeztah mentioned, and as we discussed in the maintainers meeting, any effort for custom formatting must:

  1. Be more descriptive than single letters.
  2. Follow Docker conventions about extra formatting by exposing a go template.

I really appreciate your PR, it made us aware of some improvements needed in this command. Please, refer to #12931 as it fulfills these two requirements and it also supports what the Swarm team needs.

@duglin
Copy link
Contributor

duglin commented May 25, 2015

@codemac where are we with this?

@icecrime
Copy link
Contributor

@codemac Hey! Are you still available to rebase and pursue this PR? If that's not the case, maybe @calavera can you plese carry (meaning we keep your commits in and you still get credited for the work, but we take it on to completion)? Thanks!

@calavera
Copy link
Contributor

@icecrime see #10255 (comment)

I really don't care about having any credit in the commit, but we should not deviate from other commands that already support the --format option.

@cpuguy83
Copy link
Member

@codemac Did you want to make the suggested changes?

@icecrime
Copy link
Contributor

Ping @docker/core-maintainers: if somebody wants to carry that, I think it's a good time.

@arun-gupta
Copy link
Contributor

Any ETA of when this will be available?

@estesp
Copy link
Contributor

estesp commented Jul 16, 2015

I'm going to carry this--sorry for the delay as everything else has come up this week. Of course, it will still require review and merge before it makes its way into master, so no prediction on an actual date.

@estesp
Copy link
Contributor

estesp commented Jul 17, 2015

Carried in PR #14699 , closing.

@estesp estesp closed this Jul 17, 2015
@thaJeztah
Copy link
Member

Thanks @estesp!

calavera added a commit that referenced this pull request Jul 22, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet