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

Support struct,array,slice types when sorting kubectl output #25022

Merged
merged 1 commit into from
May 15, 2016
Merged

Support struct,array,slice types when sorting kubectl output #25022

merged 1 commit into from
May 15, 2016
+91 −0

Conversation

zhouhaibing089
Copy link
Contributor

@zhouhaibing089 zhouhaibing089 commented May 1, 2016 •
edited
Loading

Fixes #24328.

Briefly, sorting_printer only take cares of the following type kinds:

  • reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64
  • reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64
  • reflect.Float32, reflect.Float64
  • reflect.String
  • reflect.Ptr

This commit aims to add reflect.Struct, reflect.Slice, reflect.Array.

/cc @bgrant0607

@k8s-bot
Copy link

k8s-bot commented May 1, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in hack/jenkins/job-configs/kubernetes-jenkins-pull/ instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

2 similar comments
@k8s-bot
Copy link

k8s-bot commented May 1, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in hack/jenkins/job-configs/kubernetes-jenkins-pull/ instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@k8s-bot
Copy link

k8s-bot commented May 1, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in hack/jenkins/job-configs/kubernetes-jenkins-pull/ instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed labels May 1, 2016
@zhouhaibing089 zhouhaibing089 force-pushed the sort-fix branch 3 times, most recently from ae5965f to ea3be92 Compare May 1, 2016 09:36
@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 1, 2016
@bgrant0607
Copy link
Member

cc @kubernetes/kubectl

@bgrant0607 bgrant0607 assigned 0xmichalis and unassigned j3ffml May 1, 2016
0xmichalis
0xmichalis reviewed May 2, 2016
View reviewed changes
pkg/kubectl/sorting_printer.go
default:
return false, fmt.Errorf("unsortable type: %v", i.Kind())
}
}

// this is a helper func to compare two structs when the given methods exists
func lessFunc(i, j reflect.Value, methods ...string) (supported, less bool) {
Copy link
Contributor

@0xmichalis 0xmichalis May 2, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

please don't use named returns

Copy link
Contributor

Choose a reason for hiding this comment

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

document them instead in the comment above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, will do this.

@0xmichalis
Copy link
Contributor

Add a test for this in hack/test-cmd.sh

@zhouhaibing089
Copy link
Contributor Author

@Kargakis added a test in hack/test-cmd.sh and changed to using non-named return value.

@zhouhaibing089
Copy link
Contributor Author

@Kargakis addressing your concern by providing a specific function to compare two unversioned.Time values.

0xmichalis
0xmichalis reviewed May 2, 2016
View reviewed changes
pkg/kubectl/sorting_printer.go
@@ -153,11 +155,56 @@ func isLess(i, j reflect.Value) (bool, error) {
return i.String() < j.String(), nil
case reflect.Ptr:
return isLess(i.Elem(), j.Elem())
case reflect.Struct:
// special case handling, unversioned.Time(TODO: add more)
Copy link
Contributor

Choose a reason for hiding this comment

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

remove the TODO, since there is notthing to be done. If the need arises, it will happen:)

@0xmichalis
Copy link
Contributor

@zhouhaibing089 thanks!

One nit about the todo, this LGTM though

@zhouhaibing089
Copy link
Contributor Author

@Kargakis addressed. thanks for your time. :)

@0xmichalis
Copy link
Contributor

@bgrant0607 please add a lgtm label here

@bgrant0607 bgrant0607 added lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels May 2, 2016
@bgrant0607 bgrant0607 changed the title let type struct,array,slice also being considerred when sorting kubectl output Support struct,array,slice types when sorting kubectl output May 2, 2016
@bgrant0607
Copy link
Member

@zhouhaibing089 I changed the title to be more understandable in the release notes.

Thanks for the review, @Kargakis

@zhouhaibing089
Copy link
Contributor Author

@bgrant0607 Thanks for helping change the title, by the way, could you help to add ok-to-merge?

@zhouhaibing089
Copy link
Contributor Author

The logs shows:

test/go/src/k8s.io/kubernetes/test/e2e_node/mirror_pod_test.go:79

    Timed out after 120.000s.
    Expected
        <*errors.errorString | 0xc82021d130>: {
            s: "expected the mirror pod \"static-pod-de79fe7e-156f-11e6-9065-42010af0001e-tmp-node-e2e-3cff3588-e2e-node-ubuntu-trusty-docker9-image\" to be running, got \"Pending\"",
        }
    to be nil

    /var/lib/jenkins/workspace/node-pull-build-e2e-test/go/src/k8s.io/kubernetes/test/e2e_node/mirror_pod_test.go:56
------------------------------

it looks like kind of flake, though I did not find similar issues via a quick search.. @Kargakis please help to trigger a retest.

@0xmichalis
Copy link
Contributor

@k8s-bot test this issue: #24782

@0xmichalis 0xmichalis mentioned this pull request May 14, 2016
Merged
@k8s-bot
Copy link

k8s-bot commented May 15, 2016

GCE e2e build/test passed for commit 09d4d5e.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 5d6c5f5 into kubernetes:master May 15, 2016
@zhouhaibing089 zhouhaibing089 deleted the sort-fix branch June 8, 2016 05:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@0xmichalis 0xmichalis

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@zhouhaibing089 @k8s-bot @bgrant0607 @0xmichalis @k8s-github-robot @j3ffml @googlebot