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

Support Table and PartialObjectMetadata on watch #71548

Merged
merged 1 commit into from Mar 20, 2019

Conversation

@smarterclayton
Copy link
Contributor

commented Nov 29, 2018

Clean up the code paths that lead to objects being transformed and output
with negotiation. Remove some duplicate code that was not consistent. Now,
watch will respond correctly to Table and PartialObjectMetadata requests.

/kind bug

TODO

  • Add unit tests
  • Add e2e test
  • Only return Table headers on the first object
  • During early output negotiation, filter unsupported encodings
Watch will now support converting response objects into Table or PartialObjectMetadata forms.
@smarterclayton

This comment has been minimized.

Copy link
Contributor Author

commented Nov 29, 2018

/assign @liggitt

First two commits are from #71542

@fedebongio

This comment has been minimized.

Copy link
Contributor

commented Nov 29, 2018

/cc @mbohlool

@k8s-ci-robot k8s-ci-robot requested a review from mbohlool Nov 29, 2018

@fedebongio

This comment has been minimized.

Copy link
Contributor

commented Nov 29, 2018

@k8s-ci-robot k8s-ci-robot requested a review from wenjiaswe Nov 29, 2018

@caesarxuchao

This comment has been minimized.

Copy link
Member

commented Dec 5, 2018

/assign

@smarterclayton smarterclayton force-pushed the smarterclayton:watch_converted branch from d15569b to 847b2bb Mar 8, 2019

@smarterclayton

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2019

Added a CRD table test and rebased, I can take your patch for partial negotiation.

@liggitt

This comment has been minimized.

Copy link
Member

commented Mar 8, 2019

on master, this works:

curl 'http://localhost:8080/api/v1/namespaces' -H "Accept: application/yaml;as=Table;v=v1beta1;g=meta.k8s.io"
apiVersion: meta.k8s.io/v1beta1
kind: Table
metadata:
  resourceVersion: "327"
  selfLink: /api/v1/namespaces
columnDefinitions:
- description: 'Name must be unique within a namespace. Is required when creating
    resources, although some resources may allow a client to request the generation
    of an appropriate name automatically. Name is primarily intended for creation
    idempotence and configuration definition. Cannot be updated. More info: http://kubernetes.io/docs/user-guide/identifiers#names'
  format: name
  name: Name
  priority: 0
  type: string
- description: The status of the namespace
  format: ""
  name: Status
  priority: 0
  type: string
- description: |-
    CreationTimestamp is a timestamp representing the server time when this object was created. It is not guaranteed to be set in happens-before order across separate operations. Clients may not set this value. It is represented in RFC3339 form and is in UTC.

    Populated by the system. Read-only. Null for lists. More info: https://git.k8s.io/community/contributors/devel/api-conventions.md#metadata
  format: ""
  name: Age
  priority: 0
  type: string
rows:
...

on this branch:

curl 'http://localhost:8080/api/v1/namespaces' -H "Accept: application/yaml;as=Table;v=v1beta1;g=meta.k8s.io"

{
  "kind": "Status",
  "apiVersion": "v1",
  "metadata": {},
  "status": "Failure",
  "message": "only the following media types are accepted when converting to meta.k8s.io/v1beta1, Kind=Table: application/json",
  "reason": "NotAcceptable",
  "code": 406
}

for non-watch, we need coverage of the content types we currently support

@liggitt

This comment has been minimized.

Copy link
Member

commented Mar 8, 2019

I can take your patch for partial negotiation.

yeah, I updated it at https://github.com/liggitt/kubernetes/commits/not-acceptable

@smarterclayton smarterclayton force-pushed the smarterclayton:watch_converted branch 2 times, most recently from 4c86aa1 to 857504b Mar 8, 2019

Support Table and PartialObjectMetadata on watch
Clean up the code paths that lead to objects being transformed and output with negotiation.
Remove some duplicate code that was not consistent. Now, watch will respond correctly to
Table and PartialObjectMetadata requests. Add unit and integration tests.

When transforming responses to Tables, only the first watch event for a given type will
include the columns. Columns will not change unless the watch is restarted.

Add a volume attachment printer and tighten up table validation error cases.

Disable protobuf from table conversion because Tables don't have protobuf because they
use `interface{}`

@smarterclayton smarterclayton force-pushed the smarterclayton:watch_converted branch from 857504b to 3230a0b Mar 8, 2019

@smarterclayton

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2019

/retest

1 similar comment
@smarterclayton

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2019

/retest

@liggitt

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

This lgtm. I'm inclined to hold it for 1.15 at this point.

@liggitt

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

/lgtm
/approve
/milestone v1.15

@k8s-ci-robot k8s-ci-robot added the lgtm label Mar 12, 2019

@k8s-ci-robot k8s-ci-robot added this to the v1.15 milestone Mar 12, 2019

@liggitt

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

/remove-label api-review

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, smarterclayton

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@liggitt

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

/hold cancel

@liggitt

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

does this PR also fix the issue #62175 was trying to address?

@fejta-bot

This comment has been minimized.

Copy link

commented Mar 20, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@smarterclayton

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

@k8s-ci-robot k8s-ci-robot merged commit 6f9bf5f into kubernetes:master Mar 20, 2019

17 checks passed

cla/linuxfoundation smarterclayton authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-godeps Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details

@liggitt liggitt removed this from In progress in API Reviews Mar 29, 2019

@wojtek-t wojtek-t referenced this pull request Jul 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.