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

Add a server side Get operation #40848

Merged
merged 3 commits into from
Jun 2, 2017

Conversation

smarterclayton
Copy link
Contributor

@smarterclayton smarterclayton commented Feb 2, 2017

Implement proposal kubernetes/community#363

The Kubernetes API supports retrieving tabular output for API resources via a new mime-type `application/json;as=Table;v=v1alpha1;g=meta.k8s.io`.  The returned object (if the server supports it) will be of type `meta.k8s.io/v1alpha1` with `Table`, and contain column and row information related to the resource.  Each row will contain information about the resource - by default it will be the object metadata, but callers can add the `?includeObject=Object` query parameter and receive the full object.  In the future kubectl will use this to retrieve the results of `kubectl get`.

@k8s-github-robot k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. release-note-label-needed labels Feb 2, 2017
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 2, 2017
@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 2, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@smarterclayton
Copy link
Contributor Author

@kubernetes/rh-ux if you had opinions about this in a console (this would be the "other resources" default table)

@smarterclayton smarterclayton added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 2, 2017
@k8s-github-robot k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 2, 2017
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 19, 2017
@k8s-github-robot k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 19, 2017
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 27, 2017
@smarterclayton smarterclayton force-pushed the serverside_get branch 2 times, most recently from 7f03a38 to 13d0e26 Compare February 27, 2017 19:09
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 27, 2017
@@ -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
Copy link
Contributor

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?

Copy link
Contributor Author

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
Copy link
Contributor

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.

@k8s-github-robot
Copy link

[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 /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label May 31, 2017
@smarterclayton smarterclayton added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. release-note-label-needed labels May 31, 2017
@smarterclayton smarterclayton added this to the v1.7 milestone May 31, 2017
@smarterclayton
Copy link
Contributor Author

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

@smarterclayton
Copy link
Contributor Author

@k8s-bot pull-kubernetes-node-e2e test this

@smarterclayton
Copy link
Contributor Author

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this
@k8s-bot pull-kubernetes-verify test this

@smarterclayton
Copy link
Contributor Author

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Jun 1, 2017

@smarterclayton: The following test(s) failed:

Test name Commit Details Rerun command
Jenkins Bazel Build 6e4960a link @k8s-bot bazel test this
Jenkins unit/integration cf94dfb2e7641ff3a0ed52aa9c21a3a8c10114d1 link @k8s-bot unit test this
Jenkins verification cf94dfb2e7641ff3a0ed52aa9c21a3a8c10114d1 link @k8s-bot verify test this
pull-kubernetes-cross 7ce63eb link @k8s-bot pull-kubernetes-cross test this

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.

@smarterclayton
Copy link
Contributor Author

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 46432, 46701, 46326, 40848, 46396)

@k8s-github-robot k8s-github-robot merged commit 97a5d37 into kubernetes:master Jun 2, 2017
@droot
Copy link
Contributor

droot commented Jun 5, 2017

@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.

@smarterclayton
Copy link
Contributor Author

Please open an issue against 1.7 and include the details.

@smarterclayton
Copy link
Contributor Author

(And assign to me)

@droot
Copy link
Contributor

droot commented Jun 7, 2017

@smarterclayton assigned issue #47093 issue to you.

k8s-github-robot pushed a commit that referenced this pull request Jun 7, 2017
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.
k8s-github-robot pushed a commit that referenced this pull request Feb 23, 2018
…-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
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants