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.

Sign up for GitHub

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

Jump to bottom

update wide option output #22772

Merged
merged 1 commit into from
Mar 11, 2016
Merged

update wide option output #22772

merged 1 commit into from
Mar 11, 2016
+109 −121

Conversation

adohe-zz
Copy link

Fixes #22742

@adohe-zz
Copy link
Author

@bgrant0607 ptal I just add a quick update. I think there are still some little problems:

  1. printPodTemplate behavior changed a little, a extra tab is add.
  2. kubectl get output is far from perfect, we still have a lot to do, I will think about more and give some better solutions.

@k8s-github-robot
Copy link

Labelling this PR as size/L

@k8s-github-robot k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 10, 2016
@k8s-bot
Copy link

k8s-bot commented Mar 10, 2016

GCE e2e build/test passed for commit a8bd31e67589fbdc8d1e42f95d6c626fa122fdc7.

@k8s-github-robot
Copy link

The author of this PR is not in the whitelist for merge, can one of the admins add the 'ok-to-merge' label?

@adohe-zz adohe-zz mentioned this pull request Mar 11, 2016
Closed
@adohe-zz
Copy link
Author

@bgrant0607 would you mind reviewing this?

@bgrant0607
Copy link
Member

@adohe Sorry! This was buried in my email. Please reference the associated issue in the PR description and cc @kubernetes/kubectl on kubectl PRs.

@bgrant0607 bgrant0607 added this to the v1.2 milestone Mar 11, 2016
@bgrant0607 bgrant0607 added kind/bug Categorizes issue or PR as related to a bug. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. cherrypick-candidate labels Mar 11, 2016
@bgrant0607
Copy link
Member

cc @janetkuo

@bgrant0607
Copy link
Member

@adohe Could you please paste example output into the PR?

bgrant0607
bgrant0607 reviewed Mar 11, 2016
View reviewed changes
pkg/kubectl/cmd/cmd_test.go
// Output:
// NAME DESIRED CURRENT AGE CONTAINER(S) IMAGE(S) SELECTOR
// foo 1 1 10y foo someimage foo=bar
// foo2 someimage
Copy link
Member

Choose a reason for hiding this comment

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

What I want is:

    // NAME      DESIRED   CURRENT     AGE       CONTAINER(S)   IMAGE(S)    SELECTOR
    // foo       1         1           10y       foo,foo2            someimage,someimage2   foo=bar

1 line

No spaces in comma-separated lists, so columns can be cut.

Copy link
Member

Choose a reason for hiding this comment

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

With columns aligned, of course. :-)

Copy link
Member

Choose a reason for hiding this comment

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

We need to eliminate layoutOtherContainers

Copy link
Member

Choose a reason for hiding this comment

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

Or rather convert it to layoutContainers, and use it everywhere we print columns for container names and images.

Copy link
Author

Choose a reason for hiding this comment

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

Ok I will fix it now.

@adohe-zz
Copy link
Author

@bgrant0607 I think this should be what we want:

[tony@localhost kubernetes]$ cluster/kubectl.sh get rc nginx -o wide -L tier --show-labels=true
NAME      DESIRED   CURRENT   AGE       CONTAINER(S)   IMAGE(S)     SELECTOR    TIER      LABELS
nginx     3         3         2s        nginx,perl     nginx,perl   app=nginx   backend   tier=backend

@adohe-zz adohe-zz force-pushed the kubectl_update_wide branch from a8bd31e to f2023d5 Compare March 11, 2016 04:29
@bgrant0607
Copy link
Member

Yes, thanks! Please notify me once you've pushed the changes.

@bgrant0607
Copy link
Member

Actually, I see them now.

bgrant0607
bgrant0607 reviewed Mar 11, 2016
View reviewed changes
pkg/kubectl/resource_printer.go
@@ -714,12 +697,12 @@ func printReplicationController(controller *api.ReplicationController, w io.Writ
); err != nil {
return err
}

if err := layoutContainers(containers, w, options); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

This should only be in wide output?

Copy link
Member

Choose a reason for hiding this comment

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

For all the resource types.

Copy link
Author

Choose a reason for hiding this comment

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

I add a check inside this function, and according to our original code, we put this in wide output.

@bgrant0607
Copy link
Member

@adohe PTAL at my comments. Thanks!

@k8s-bot
Copy link

k8s-bot commented Mar 11, 2016

GCE e2e build/test passed for commit f2023d5cdfc5d11904da1a91f8e49233fc45e03c.

@adohe-zz adohe-zz force-pushed the kubectl_update_wide branch from f2023d5 to 9788101 Compare March 11, 2016 05:38
@adohe-zz
Copy link
Author

In this PR, I don't change the kubectl get ingress behaviour, we can leave it to next-candidate, also layoutContainers is used for all resource type if necessary. @bgrant0607 @bprashanth ptal.

@bgrant0607
Copy link
Member

Great! LGTM. Thanks much for doing this on short notice.

@bgrant0607 bgrant0607 added lgtm "Looks good to me", indicates that a PR is ready to be merged. cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. labels Mar 11, 2016
@bgrant0607
Copy link
Member

Updated PR description

@bgrant0607 bgrant0607 added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Mar 11, 2016
@adohe-zz
Copy link
Author

@bgrant0607 Thanks very much.

@k8s-bot
Copy link

k8s-bot commented Mar 11, 2016

GCE e2e build/test passed for commit 9788101.

@bgrant0607
Copy link
Member

Manually merging.

bgrant0607 added a commit that referenced this pull request Mar 11, 2016
@bgrant0607
Merge pull request #22772 from AdoHe/kubectl_update_wide
5bd45d2 
update wide option output
@bgrant0607 bgrant0607 merged commit 5bd45d2 into kubernetes:master Mar 11, 2016
eparis pushed a commit to eparis/kubernetes that referenced this pull request Mar 11, 2016
@bgrant0607 @eparis
Merge pull request kubernetes#22772 from AdoHe/kubectl_update_wide
d8f9975 
update wide option output
@eparis eparis mentioned this pull request Mar 11, 2016
Merged
@eparis
Copy link
Contributor

eparis commented Mar 11, 2016

This PR was sucessfully cherry picked in PR #22855
please verify that the release-1.2 branch contains these changes as you would expect and contact @eparis if there appear to be problems.

@eparis eparis removed cherrypick-candidate cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. labels Mar 11, 2016
@adohe-zz adohe-zz deleted the kubectl_update_wide branch March 14, 2016 02:02
shyamjvs pushed a commit to shyamjvs/kubernetes that referenced this pull request Dec 1, 2016
@bgrant0607 @eparis
Merge pull request kubernetes#22772 from AdoHe/kubectl_update_wide
8d2add5 
update wide option output
shouhong pushed a commit to shouhong/kubernetes that referenced this pull request Feb 14, 2017
@bgrant0607 @eparis
Merge pull request kubernetes#22772 from AdoHe/kubectl_update_wide
aea8ed4 
update wide option output
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@bgrant0607 bgrant0607

Labels
kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.2
Development

Successfully merging this pull request may close these issues.
10 participants
@adohe-zz @k8s-github-robot @k8s-bot @bgrant0607 @eparis @bprashanth @k8s-teamcity-mesosphere @googlebot @brendandburns @adohe