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

Resource Library #301

Merged
merged 3 commits into from Mar 6, 2018
Merged

Resource Library #301

merged 3 commits into from Mar 6, 2018

Conversation

@ellenkorbes
Copy link
Contributor

@ellenkorbes ellenkorbes commented Feb 16, 2018

Package resource implements tools for the discovery and filtering of resources in an API server.

Tests to be implemented in a subsequent PR.

@k8s-ci-robot
Copy link
Contributor

@k8s-ci-robot k8s-ci-robot commented Feb 16, 2018

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ellenkorbes
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: pwittrock

Assign the PR to them by writing /assign @pwittrock in a comment when ready.

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

@ellenkorbes
Copy link
Contributor Author

@ellenkorbes ellenkorbes commented Feb 17, 2018

/assign @apelisse
/assign @pwittrock

Copy link
Member

@apelisse apelisse left a comment

This is great! Very clean. Thank you!

The following (admittedly ludicrous) example adds a filter to the example above that excludes all resources that do not start with the letter "n":
type letterN struct {

This comment has been minimized.

@apelisse

apelisse Feb 17, 2018
Member

Yeah, there is no reason not to have this as a compiled example.

For example:
p := resource.NewParser(resources, discovery, apiGroup, apiVersion)

This comment has been minimized.

@apelisse

apelisse Feb 17, 2018
Member

Same, I think this could be compiled. We can't test the output, but we can make sure it compiles.

This comment has been minimized.

@ellenkorbes

ellenkorbes Feb 20, 2018
Author Contributor

Since the examples won't pass testing, isn't that possibly gonna create a problem for us later on?

This comment has been minimized.

@apelisse

apelisse Feb 20, 2018
Member

if you don't have the // Output: comment in your example, they won't run, they won't fail. They will be compiled to make sure that they are up-to-date though (which is still valuable to us).

}
func (*letterN) Resource(r *resource.Resource) bool {
return string(r.Resource.Name[0]) == "n"

This comment has been minimized.

@apelisse

apelisse Feb 17, 2018
Member

It'd be weird to have an empty resource name, but that could technically crash ;-)

type letterN struct {
}
func (*letterN) Resource(r *resource.Resource) bool {

This comment has been minimized.

@apelisse

apelisse Feb 17, 2018
Member

Doesn't need to be pointer.

if err != nil {
panic(err)
}
r = r.Filter(&letterN{})

This comment has been minimized.

@apelisse

apelisse Feb 17, 2018
Member

Doesn't have to be address (if you remove the two pointers above)


// resource returns the resource name and true if it is a resource (not a subresource).
// It returns the resource name and false it if it is a subresource.
func (p *parser) resource(resource *v1.APIResource) (string, bool) {

This comment has been minimized.

@apelisse

apelisse Feb 17, 2018
Member

naming return parameters?

}

// isGroupVersionMatch returns false if either group or version doesn't match with the API.
func (p *parser) isGroupVersionMatch(group, version string) bool {

This comment has been minimized.

@apelisse

apelisse Feb 17, 2018
Member

I'm wondering why we need that? If we could get rid of that, that'd simplify the creation of the parser. @pwittrock


// Resource is an API Resource.
type Resource struct {
Resource v1.APIResource

This comment has been minimized.

@apelisse

apelisse Feb 17, 2018
Member

I think we'd rather make these private by default, and change them to public as needed.

This comment has been minimized.

@ellenkorbes

ellenkorbes Feb 20, 2018
Author Contributor

You sure? If I were to do this it'd break my filter examples (they depend on r.Resource.Name).

I tried it anyway and the IDE gave me:

Rename failed: /home/ellen/go/src/k8s.io/kubectl/pkg/framework/resource/resource.go:26:6: renaming this type "Resource" to "resource" would make it unexported /home/ellen/go/src/k8s.io/kubectl/pkg/framework/resource/poc/main.go:38:38: breaking references from packages such as "k8s.io/kubectl/pkg/framework/resource/poc" [...]

I did it by hand then and indeed it broke everything.

undefined: "k8s.io/kubectl/pkg/framework/resource".Resource

There's probably a way to do the filtering differently, but do we really have to?

This comment has been minimized.

@apelisse

apelisse Feb 20, 2018
Member

Sorry if I was unclear. By "these", I meant "all these fields". If this one in particular is needed, let's keep it public, what about the others?


// SubResource is an API subresource.
type SubResource struct {
Resource v1.APIResource

This comment has been minimized.

@apelisse

apelisse Feb 17, 2018
Member

public -> private?

}

// indexResources indexes into maps the resources from the API resource list. It returns a map indexed by resource name, and a map indexed by GroupVersionResource objects.
func (p *parser) indexResources(gvs []*v1.APIResourceList) (map[string][]*Resource, map[schema.GroupVersionResource]*Resource) {

This comment has been minimized.

@apelisse

apelisse Feb 17, 2018
Member

Should the type be "Resources" here (first returned type)?

@apelisse
Copy link
Member

@apelisse apelisse commented Feb 17, 2018

Yeah, you should run gofmt too.

return true
}

// getOpenAPI retrieves a schema object from the API based on a GroupVersionResource triplet.

This comment has been minimized.

@apelisse

apelisse Feb 17, 2018
Member

I'm not sure why this returns the Schema and bool. It's safe and sufficient to check if Schema is nil.

@apelisse
Copy link
Member

@apelisse apelisse commented Feb 20, 2018

This is great. Please fix the test so that we can merge it!

Thanks!

@k8s-ci-robot k8s-ci-robot added size/XXL and removed size/XL labels Feb 25, 2018
@ellenkorbes ellenkorbes force-pushed the ellenkorbes:clientlibmin branch 2 times, most recently from b88a3e6 to 50140c8 Feb 25, 2018
@ellenkorbes
Copy link
Contributor Author

@ellenkorbes ellenkorbes commented Feb 25, 2018

/retest

Copy link
Member

@apelisse apelisse left a comment

Just had time for a cursory look, but I think it's pretty good. I have a bunch of nit, but I don't want these to block.

tst "k8s.io/kubernetes/pkg/kubectl/cmd/util/openapi/testing"
)

var fakeSchema = tst.Fake{Path: filepath.Join("..", "..", "..", "..", "..", "api", "openapi-spec", "swagger.json")}

This comment has been minimized.

@apelisse

apelisse Feb 27, 2018
Member

This can't work :-/

@@ -0,0 +1,33 @@
package(default_visibility = ["//visibility:public"])

This comment has been minimized.

@apelisse

apelisse Feb 27, 2018
Member

I think you can drop these files in general (bazel)

@@ -0,0 +1,141 @@
/*

This comment has been minimized.

@apelisse

apelisse Feb 27, 2018
Member

I'm not sure you need this package at all.

Resource v1.APIResource
apiGroupVersion schema.GroupVersion
schema openapi.Schema
SubResources []*SubResource

This comment has been minimized.

@apelisse

apelisse Feb 27, 2018
Member

Try to sort public and then private fields for readability

@ellenkorbes ellenkorbes force-pushed the ellenkorbes:clientlibmin branch 3 times, most recently from b596d8a to d79f38d Mar 2, 2018
ellenkorbes added 3 commits Mar 2, 2018
@ellenkorbes ellenkorbes force-pushed the ellenkorbes:clientlibmin branch from d79f38d to f893cb6 Mar 6, 2018
Copy link
Member

@apelisse apelisse left a comment

Good job Ellen! Thank you so much 🎆

@k8s-ci-robot k8s-ci-robot merged commit f09aa18 into kubernetes:master Mar 6, 2018
3 checks passed
3 checks passed
cla/linuxfoundation ellenkorbes authorized
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
tide In tide pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.