adds dynamic lister #68748

p0lyn0mial · 2018-09-17T17:12:50Z

What this PR does / why we need it: creates a dynamic lister that returns runtime.Unstructured objects.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Release note:

creates a dynamic lister that returns `runtime.Unstructured` objects. The new lister is under `client-go/dynamic/dynamiclister`.

p0lyn0mial · 2018-09-17T17:13:04Z

/assign @deads2k

p0lyn0mial · 2018-09-17T17:15:03Z

@deads2k this pull brings a dynamic lister that returns runtime.Unstructured objects. I hope this is something you have been asking for.

@deads2k I'm working on creating a dynamic informer that will use the dynamic lister.

p0lyn0mial · 2018-09-17T17:16:16Z

staging/src/k8s.io/client-go/dynamic/lister.go

+	}
+	if !exists {
+		// TODO (p0lyn0mial): figure out how to construct proper Resource for the given object
+		return nil, errors.NewNotFound(schema.GroupResource{Resource: "unstructured"}, name)


not sure how we could make it more informative.

not sure how we could make it more informative.

Ah, I read top to bottom. Just got here. See my comment above.

wenjiaswe · 2018-09-17T20:06:50Z

/cc @jpbetz

p0lyn0mial · 2018-09-19T18:54:53Z

staging/src/k8s.io/client-go/dynamic/interface.go

+// Lister helps list resources.
+type Lister interface {
+	// List lists all resources in the indexer.
+	List(selector labels.Selector) (ret []*unstructured.Unstructured, err error)


not sure if we want to provide a way to list all items or only the ones from a namespace.

not sure if we want to provide a way to list all items or only the ones from a namespace.

I think we have to provide both in this lister since we don't know which is applicable. We probably need a top level Get too.

deads2k · 2018-09-19T19:57:39Z

staging/src/k8s.io/client-go/dynamic/lister.go

+}
+
+// NewDynamicLister returns a new Lister.
+func NewDynamicLister(indexer cache.Indexer) Lister {


Can we require a GVR be provided here? It is implicit for all other listers, but this one doesn't natively know which resource it is dealing with. You'll need it for errors.

deads2k · 2018-09-19T19:59:03Z

staging/src/k8s.io/client-go/dynamic/lister_test.go

+	"k8s.io/client-go/tools/cache"
+)
+
+func TestNamespaceGetMethod(t *testing.T) {


I'd like to a second test for a top level get method too. I'm particularly interested in being sure that it does the right thing (not found) when used on namespaced resources.

deads2k · 2018-09-19T20:00:04Z

This looks like a solid start. A few comments. @p0lyn0mial Once they are fixed, any particular reason to avoid merging it?

deads2k · 2018-09-19T20:00:47Z

staging/src/k8s.io/client-go/dynamic/BUILD

    ],
 )

 go_library(
    name = "go_default_library",
    srcs = [
        "interface.go",
+        "lister.go",


can you place these in a subpackage? client-go/dynamic/dynamiclister

how about client-go/dynamic/lister ?

how about client-go/dynamic/lister ?

dynamiclister will autocomplete in IDEs without conflict. Think http/httptest

oh, nice! Since there is more than one lister in the system it makes sense.

p0lyn0mial · 2018-09-19T21:07:22Z

@p0lyn0mial Once they are fixed, any particular reason to avoid merging it?

sgtm, originally I wanted to add the informer too but I can prepare a separate pull for it.

deads2k · 2018-09-20T13:28:45Z

staging/src/k8s.io/client-go/dynamic/dynamiclister/lister.go

+		return nil, err
+	}
+	if !exists {
+		return nil, errors.NewNotFound(schema.GroupResource{Group: l.gvr.Group, Resource: l.gvr.Resource}, name)


nit: l.gvr.GroupResource()

deads2k · 2018-09-20T13:28:54Z

staging/src/k8s.io/client-go/dynamic/dynamiclister/lister.go

+		return nil, err
+	}
+	if !exists {
+		return nil, errors.NewNotFound(schema.GroupResource{Group: l.gvr.Group, Resource: l.gvr.Resource}, name)


deads2k · 2018-09-20T13:31:04Z

Looks like you need to regenerate bazel. Then squash down.

/lgtm
/hold

holding so the bot doesn't endlessly retest on bad bazel. Remove the hold after you update bazel.

p0lyn0mial · 2018-09-20T20:03:12Z

/hold cancel

deads2k · 2018-09-20T20:10:16Z

/lgtm

p0lyn0mial · 2018-09-21T07:23:34Z

New changes are detected. LGTM label has been removed.

go fmt update

p0lyn0mial · 2018-09-21T07:24:22Z

/test pull-kubernetes-e2e-kops-aws
/test pull-kubernetes-e2e-gce-device-plugin-gpu

p0lyn0mial · 2018-09-25T18:18:54Z

@deads2k please retag the pull.

deads2k · 2018-09-26T20:14:45Z

/lgtm

k8s-ci-robot · 2018-09-26T20:14:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, p0lyn0mial

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:

~~staging/src/k8s.io/client-go/OWNERS~~ [deads2k]

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

p0lyn0mial · 2018-09-26T20:53:46Z

/kind feature

p0lyn0mial · 2018-09-26T20:55:50Z

/retest

k8s-ci-robot requested review from deads2k and lavalamp September 17, 2018 17:13

k8s-ci-robot assigned deads2k Sep 17, 2018

p0lyn0mial commented Sep 17, 2018

View reviewed changes

k8s-ci-robot requested a review from jpbetz September 17, 2018 20:06

p0lyn0mial commented Sep 19, 2018

View reviewed changes

deads2k reviewed Sep 19, 2018

View reviewed changes

deads2k reviewed Sep 20, 2018

View reviewed changes

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 20, 2018

p0lyn0mial changed the title ~~[WIP] adds dynamic lister~~ adds dynamic lister Sep 20, 2018

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 20, 2018

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. 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 Sep 20, 2018

p0lyn0mial force-pushed the dynamic_lister_informer branch from 8665d5b to fe805d6 Compare September 20, 2018 19:36

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 20, 2018

p0lyn0mial force-pushed the dynamic_lister_informer branch from fe805d6 to 01be525 Compare September 20, 2018 19:53

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 20, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 20, 2018

adds dynamic lister

8b43a00

p0lyn0mial force-pushed the dynamic_lister_informer branch from 01be525 to 8b43a00 Compare September 20, 2018 22:10

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 20, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 26, 2018

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Sep 26, 2018

k8s-ci-robot merged commit 23baf57 into kubernetes:master Sep 26, 2018

p0lyn0mial mentioned this pull request Oct 8, 2018

Create dynamic informers and listers #64319

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adds dynamic lister #68748

adds dynamic lister #68748

p0lyn0mial commented Sep 17, 2018 •

edited

p0lyn0mial commented Sep 17, 2018

p0lyn0mial commented Sep 17, 2018

p0lyn0mial Sep 17, 2018

deads2k Sep 19, 2018

wenjiaswe commented Sep 17, 2018

p0lyn0mial Sep 19, 2018

deads2k Sep 19, 2018

deads2k Sep 19, 2018

p0lyn0mial Sep 19, 2018

deads2k Sep 19, 2018

p0lyn0mial Sep 19, 2018

deads2k commented Sep 19, 2018

deads2k Sep 19, 2018

p0lyn0mial Sep 19, 2018

deads2k Sep 20, 2018

p0lyn0mial Sep 20, 2018

p0lyn0mial commented Sep 19, 2018

deads2k Sep 20, 2018

deads2k Sep 20, 2018

deads2k commented Sep 20, 2018

p0lyn0mial commented Sep 20, 2018

deads2k commented Sep 20, 2018

p0lyn0mial commented Sep 21, 2018

p0lyn0mial commented Sep 21, 2018

p0lyn0mial commented Sep 25, 2018

deads2k commented Sep 26, 2018

k8s-ci-robot commented Sep 26, 2018

p0lyn0mial commented Sep 26, 2018

p0lyn0mial commented Sep 26, 2018

adds dynamic lister #68748

adds dynamic lister #68748

Conversation

p0lyn0mial commented Sep 17, 2018 • edited

p0lyn0mial commented Sep 17, 2018

p0lyn0mial commented Sep 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenjiaswe commented Sep 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Sep 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0lyn0mial commented Sep 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Sep 20, 2018

p0lyn0mial commented Sep 20, 2018

deads2k commented Sep 20, 2018

p0lyn0mial commented Sep 21, 2018

p0lyn0mial commented Sep 21, 2018

p0lyn0mial commented Sep 25, 2018

deads2k commented Sep 26, 2018

k8s-ci-robot commented Sep 26, 2018

p0lyn0mial commented Sep 26, 2018

p0lyn0mial commented Sep 26, 2018

p0lyn0mial commented Sep 17, 2018 •

edited