-
Notifications
You must be signed in to change notification settings - Fork 39.8k
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
Add a server side Get operation #40848
Add a server side Get operation #40848
Conversation
@kubernetes/rh-ux if you had opinions about this in a console (this would be the "other resources" default table) |
36ada4d
to
49237ec
Compare
49237ec
to
a42825c
Compare
a42825c
to
57f5522
Compare
7f03a38
to
13d0e26
Compare
@@ -158,6 +159,9 @@ type Store struct { | |||
// ExportStrategy implements resource-specific behavior during export, | |||
// optional. Exported objects are not decorated. | |||
ExportStrategy rest.RESTExportStrategy | |||
// TableConvertor is an optional interface for transforming items or lists | |||
// of items into tabular output. If unset, the default will be used. | |||
TableConvertor rest.TableConvertor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Barring ExportStrategy
, the rest of this interface is about managing basic CRUD. Can you keep the layers separate so we have one layer for CRUD which can be wrapped by something which can do a TableConversion separately?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ExportStrategy and Tables are the same. Store isn't about CRUD, it's about REST. Export/Table are Representations of State.
fn := func(obj runtime.Object) error { | ||
m, err := meta.Accessor(obj) | ||
if err != nil { | ||
// TODO: skip objects we don't recognize |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A silent skip seems wrong.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, smarterclayton
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this |
@k8s-bot pull-kubernetes-node-e2e test this |
@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this |
@smarterclayton: The following test(s) failed:
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this |
Automatic merge from submit-queue (batch tested with PRs 46432, 46701, 46326, 40848, 46396) |
@smarterclayton I was testing master and discovered the output of kubectl Get command is not aligned. I ran 'git bisect' to locate the commit where alignment got broken and it turned out to be this PR. I'm new to Kubernetes, so didn't follow this code completely, so thought of commenting it here. This is a critical issue and will block 1.7 release. So please take a look and let me know if you need any help with reproducing it. |
Please open an issue against 1.7 and include the details. |
(And assign to me) |
@smarterclayton assigned issue #47093 issue to you. |
Automatic merge from submit-queue (batch tested with PRs 47024, 47050, 47086, 47081, 47013) Wrap HumanReadablePrinter in tab output unless explicitly asked not to `kubectl get` was not properly aligning its output due to #40848 Fixes an accidental regression. In general, we should not accept an incoming tabwriter and instead manage at a higher level. Fix the bug and add a comment re: future refactoring.
…-response-proto-v2 Automatic merge from submit-queue (batch tested with PRs 55637, 57461, 60268, 60290, 60210). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Update get.go to use server-side printing Addresses part of #58536 Adds support for server-side changes implemented in #40848 and updated in #59059 @deads2k per our discussion, opening this as a separate PR. This wires through a per-request use of `as=Table;...` header parameters using the resource builder from the `kube get` command. #### Items to consider going forward: - [ ] Figure out how to handle sorting when dealing with multiple Table objects from the server - [ ] Figure out sorting when dealing with a mixed response from the server consisting of Tables and normal resources (`--sort-by` is handled in this PR by falling back to old behavior) - [ ] Filtering: How should we filter Table objects? Separate filter for rows? Filter on jsonpath? We have access to partial object metadata for each table row - not enough to know how to filter pods, for example but we could request that the original object be included along with each Table.Row by adding an `includeObject` param in the client request. #### Resources that do not yet support Table output - [ ] Namespaces - [ ] Services - [ ] Service catalog resources: https://github.com/kubernetes-incubator/service-catalog/blob/master/pkg/apis/servicecatalog/v1beta1/types.go **Release note**: ```release-note NONE ```
Implement proposal kubernetes/community#363