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

UPSTREAM: 31163: add resource filter handling #10574

Merged
merged 1 commit into from
Sep 26, 2016
Merged

UPSTREAM: 31163: add resource filter handling #10574

merged 1 commit into from
Sep 26, 2016

Conversation

juanvallejo
Copy link
Contributor

@juanvallejo juanvallejo commented Aug 22, 2016 •
edited
Loading

Fixes: #9901
Upstream: kubernetes/kubernetes#31163

Resources are currently filtered (in order to prevent printing) at print
time in their HumanReadablePrinter handlers. This design makes it not
possible to filter objects when they are printed using any other
printer, such as YAML, JSON, or the NamePrinter.

This patch removes any filters previously added at the printer level for
pods and adds a way to define resource-specific filters before they are
sent to a printer handler. A woking filter handler for pods has also been
implemented.

Filters affect resources being printed through the HumanReadablePrinter,
name printer, YAML, JSON, and --template printers.

Print pods with different outputs

$ oc get pods

NAME                    READY     STATUS              RESTARTS   AGE
database-1-h9dl1        1/1       Running             5          3d
hello-openshift         1/1       Running             4          3d
idling-echo-1-0bpfv     2/2       Running             6          3d
idling-echo-1-i01c2     2/2       Running             6          3d
mysql-master-1-deploy   0/1       Error               0          3d
mysql-slave-1-deploy    0/1       Error               0          3d
wordpress               0/1       ContainerCreating   0          3d

$ oc get pods --show-all=false -o name

pod/database-1-h9dl1
pod/hello-openshift
pod/idling-echo-1-0bpfv
pod/idling-echo-1-i01c2
pod/wordpress

$ oc get pods --show-all=false --template='{{(.metadata.name)}} '

database-1-h9dl1  hello-openshift  idling-echo-1-0bpfv  idling-echo-1-i01c2  wordpress

cc @fabianofranz @smarterclayton @deads2k @liggitt @rhcarvalho

@juanvallejo juanvallejo force-pushed the jvallejo_filter-resources-before-printing branch from 6f099ee to e9d59ac Compare August 22, 2016 19:48
@smarterclayton
Copy link
Contributor

Please open this upstream and let's do this review there.

smarterclayton
smarterclayton reviewed Aug 22, 2016
View reviewed changes
vendor/k8s.io/kubernetes/pkg/kubectl/cmd/get.go
if err != nil {
return err
for ix := range infos {
filter := kubectl.NewResourceFilter(filterOptionsForCommand(cmd, allNamespaces))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you creating one filter per info?

Copy link
Contributor

Choose a reason for hiding this comment

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

You have to track whether the filter removed anything to trigger "was filtered out" message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You have to track whether the filter removed anything to trigger "was filtered out" message.

Ah, did this upstream although since I am creating a filter per every info I had to approach it differently to how they had it. Would a better implementation be to determine if a runtime.Object is a list of resources, then either return a list with unfiltered pods / remove filtered pods from that list?

@juanvallejo juanvallejo changed the title [WIP] UPSTREAM: 0000: add resource filter handling [WIP] UPSTREAM: 31163: add resource filter handling Aug 22, 2016
@juanvallejo juanvallejo force-pushed the jvallejo_filter-resources-before-printing branch from e9d59ac to c52667f Compare August 22, 2016 21:22
@juanvallejo juanvallejo mentioned this pull request Aug 26, 2016
Merged
@juanvallejo juanvallejo force-pushed the jvallejo_filter-resources-before-printing branch from bbb1a29 to 8397d49 Compare September 2, 2016 21:58
@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 9, 2016
@juanvallejo juanvallejo force-pushed the jvallejo_filter-resources-before-printing branch from 89a0ef9 to 340de34 Compare September 12, 2016 19:08
@juanvallejo
Copy link
Contributor Author

[test]

@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 12, 2016
@juanvallejo juanvallejo force-pushed the jvallejo_filter-resources-before-printing branch 4 times, most recently from c0ac1f5 to 0cf11da Compare September 13, 2016 14:37
@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 17, 2016
@juanvallejo juanvallejo force-pushed the jvallejo_filter-resources-before-printing branch from 09fe0bd to 74e848c Compare September 19, 2016 21:08
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 19, 2016
@juanvallejo juanvallejo force-pushed the jvallejo_filter-resources-before-printing branch 2 times, most recently from f3c62af to 2b9e40a Compare September 20, 2016 21:36
@juanvallejo juanvallejo changed the title [WIP] UPSTREAM: 31163: add resource filter handling UPSTREAM: 31163: add resource filter handling Sep 23, 2016
@juanvallejo
Copy link
Contributor Author

@smarterclayton @fabianofranz Upstream merged, PTAL

@fabianofranz
Copy link
Member

LGTM, please squash.

@juanvallejo
UPSTREAM: 31163: add resource filter handling
e4a3e8b 
UPSTREAM: kubernetes/kubernetes#31163
Fixes openshift#9901

Resources are currently filtered (in order to prevent printing) at print
time in their HumanReadablePrinter handlers. This design makes it not
possible to filter objects when they are printed using any other
printer, such as YAML, JSON, or the NamePrinter.

This patch removes any filters previously added at the printer level for
pods and adds a way to define resource-specific filters before they are
sent to a printer handler. A woking filter handler for pods has also
been
implemented.

Filters affect resources being printed through the HumanReadablePrinter,
YAML, JSON, and `--template` printers.
@juanvallejo juanvallejo force-pushed the jvallejo_filter-resources-before-printing branch from 2b9e40a to e4a3e8b Compare September 23, 2016 18:16
@juanvallejo
Copy link
Contributor Author

conformance test flaked on #9548 re[test]

@juanvallejo
Copy link
Contributor Author

conformance test flaked on #11079 re[test]

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to e4a3e8b

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/9284/)

@fabianofranz
Copy link
Member

[merge]

@openshift-bot
Copy link
Contributor

openshift-bot commented Sep 26, 2016 •
edited
Loading

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/9284/) (Image: devenv-rhel7_5076)

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to e4a3e8b

@openshift-bot openshift-bot merged commit 9790e8b into openshift:master Sep 26, 2016
@juanvallejo juanvallejo deleted the jvallejo_filter-resources-before-printing branch September 26, 2016 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@fabianofranz fabianofranz

Labels
priority/P2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@juanvallejo @smarterclayton @fabianofranz @openshift-bot @rhcarvalho @danmcp