Refactor service storage to remove registry wrapper by smarterclayton · Pull Request #59510 · kubernetes/kubernetes

smarterclayton · 2018-02-07T22:02:29Z

This exposes the correct table exporter to the API endpoint, which is a prereq for server side GET to beta. Removing the use of the registry simplifies a few complex changes but results in test abstractions changing.

Part of #58536

k8s-ci-robot · 2018-02-07T22:02:30Z

@smarterclayton: Adding do-not-merge/release-note-label-needed because the release note process has not been followed.

Details

One of the following labels is required "release-note", "release-note-action-required", or "release-note-none".
Please see: https://git.k8s.io/community/contributors/devel/pull-requests.md#write-release-notes-if-needed.

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.

smarterclayton · 2018-02-07T22:03:12Z

@liggitt @kubernetes/sig-api-machinery-pr-reviews refactoring service storage so that Services have the correct API metadata

smarterclayton · 2018-02-07T22:04:12Z

The test should return a 406 for a backend which does not implement metadata should break because of this change, at which point I need to change which resource it hits.

jpbetz · 2018-02-08T21:07:54Z

cc @caesarxuchao

k8s-ci-robot · 2018-02-10T20:47:02Z

@smarterclayton: Adding do-not-merge/release-note-label-needed because the release note process has not been followed.

Details

One of the following labels is required "release-note", "release-note-action-required", or "release-note-none".
Please see: https://git.k8s.io/community/contributors/devel/pull-requests.md#write-release-notes-if-needed.

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.

liggitt · 2018-02-10T20:57:24Z

pkg/registry/core/service/storage/rest.go

don't inline this... that prevents the compiler from helping us notice if one of the methods we override changes signatures and our overridden version gets bypassed.

liggitt · 2018-02-10T21:06:25Z

pkg/registry/core/service/storage/storage.go

why the name change?

There are two storage REST types. One is the raw underlying layer, one is the wrapper. Rather than use "storage" and "rest", and given that rest is our consistently used name now, used "REST" for the wrapper that is consumed and "GenericREST" for the inner wrapper.

liggitt · 2018-02-10T21:11:44Z

pkg/registry/core/service/storage/rest_test.go

this was a duplicate name/namespace? what was this doing before?

the first one in the list was getting chosen it looks like by name. someone added the second name, the test passed, went on their way. will dig in a bit more.

sttts · 2018-02-12T07:47:34Z

staging/src/k8s.io/apiserver/pkg/endpoints/installer.go

is this switch still needed?

k8s-ci-robot · 2018-02-19T21:10:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: smarterclayton

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~api/OWNERS~~ [smarterclayton]
~~docs/api-reference/OWNERS~~ [smarterclayton]
~~pkg/registry/OWNERS~~ [smarterclayton]
~~staging/src/k8s.io/apiserver/OWNERS~~ [smarterclayton]
~~test/OWNERS~~ [smarterclayton]

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

smarterclayton · 2018-02-20T21:10:24Z

Any other comments?

soltysh

The changes lgtm, but I'll let somebody from @kubernetes/sig-api-machinery-pr-reviews to push it forward

sttts · 2018-02-22T17:09:51Z

pkg/registry/core/service/storage/rest.go

nit: services

sttts · 2018-02-22T17:13:27Z

pkg/registry/core/service/storage/rest.go

type assertions for these would be nice

just moved. nvmd

sttts · 2018-02-22T17:22:50Z

pkg/registry/core/service/storage/rest_test.go

what does it protect? please put on-top of the block it belongs to.

Was leftover from a much earlier version of this test (this file was moved, but then refactored because it talked to a registry instead of to a rest storage). I'm going to remove it, no threaded operations appear to be performed anywhere in here.

sttts · 2018-02-22T17:24:37Z

pkg/registry/core/service/storage/rest.go

sttts · 2018-02-22T17:27:26Z

Some nits. Otherwise lgtm.

The registry abstraction is unnecessary and adds direct coupling to the core types. By using a wrapper, we carry through the default implementations of the non-mutating operations. The DeleteCollection method is explicitly patched out since it cannot be correctly implemented on the storage currently. As a result, TableConvertor is now exposed. A few other minor refactorings * Corrected the case of some variables * Used functions instead of methods for several helper methods * Removed the legacy Deleter - service was the only remaining consumer

smarterclayton · 2018-02-23T04:27:24Z

Nits addressed, applying label

k8s-github-robot · 2018-02-23T10:34:55Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-ci-robot · 2018-02-23T10:56:48Z

@smarterclayton: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-kops-aws	`98cf7e6`	link	`/test pull-kubernetes-e2e-kops-aws`

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.

Details

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-github-robot · 2018-02-23T10:59:41Z

Automatic merge from submit-queue (batch tested with PRs 60106, 59510, 60263, 60063, 59088). If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot requested review from enj, nikhiljindal and wojtek-t February 7, 2018 22:02

k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Feb 7, 2018

smarterclayton added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Feb 7, 2018

smarterclayton force-pushed the services_table branch from 2e677ed to 7d3415d Compare February 7, 2018 22:18

k8s-ci-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Feb 10, 2018

smarterclayton mentioned this pull request Feb 10, 2018

Bringing server-side printing to beta summary #58536

Closed

16 tasks

smarterclayton force-pushed the services_table branch from 7d3415d to 7f79a43 Compare February 10, 2018 20:52

liggitt reviewed Feb 10, 2018

View reviewed changes

smarterclayton force-pushed the services_table branch 3 times, most recently from 4f0ea8d to 242b836 Compare February 11, 2018 02:06

sttts reviewed Feb 12, 2018

View reviewed changes

sttts added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Feb 12, 2018

smarterclayton force-pushed the services_table branch from 242b836 to d664fc6 Compare February 12, 2018 17:57

smarterclayton force-pushed the services_table branch 5 times, most recently from 3741f2b to 05796ff Compare February 19, 2018 04:28

soltysh added this to the v1.10 milestone Feb 19, 2018

smarterclayton force-pushed the services_table branch from 05796ff to 78d674e Compare February 19, 2018 21:10

smarterclayton force-pushed the services_table branch 2 times, most recently from 2de29ce to b5a43b6 Compare February 22, 2018 16:25

soltysh approved these changes Feb 22, 2018

View reviewed changes

sttts reviewed Feb 22, 2018

View reviewed changes

pkg/registry/core/service/storage/rest.go Outdated

Copy link
Copy Markdown

Contributor

sttts Feb 22, 2018

Choose a reason for hiding this comment

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

nit: services

sttts reviewed Feb 22, 2018

View reviewed changes

pkg/registry/core/service/storage/rest.go Outdated

Copy link
Copy Markdown

Contributor

sttts Feb 22, 2018

Choose a reason for hiding this comment

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

godoc

smarterclayton added 2 commits February 22, 2018 23:26

generated: bazel

98cf7e6

smarterclayton force-pushed the services_table branch from b5a43b6 to 98cf7e6 Compare February 23, 2018 04:26

smarterclayton added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 23, 2018

k8s-github-robot merged commit 3a399c0 into kubernetes:master Feb 23, 2018

Conversation

smarterclayton commented Feb 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Feb 7, 2018

Uh oh!

smarterclayton commented Feb 7, 2018

Uh oh!

smarterclayton commented Feb 7, 2018

Uh oh!

jpbetz commented Feb 8, 2018

Uh oh!

k8s-ci-robot commented Feb 10, 2018

Uh oh!

liggitt Feb 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Feb 19, 2018

Uh oh!

smarterclayton commented Feb 20, 2018

Uh oh!

soltysh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sttts commented Feb 22, 2018

Uh oh!

smarterclayton commented Feb 23, 2018

Uh oh!

k8s-github-robot commented Feb 23, 2018

Uh oh!

k8s-ci-robot commented Feb 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-github-robot commented Feb 23, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

smarterclayton commented Feb 7, 2018 •

edited

Loading

liggitt Feb 10, 2018 •

edited

Loading

k8s-ci-robot commented Feb 23, 2018 •

edited

Loading