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.

Sign up for GitHub

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

Jump to bottom

add a discovery-client #15659

Merged
merged 1 commit into from Oct 17, 2015
Merged

add a discovery-client #15659

merged 1 commit into from Oct 17, 2015

Conversation

caesarxuchao
Copy link
Member

@lavalamp This is still WIP, it'd be great if you can check if this is in the right direction.

The SupportedResources() functions are copied from Brendan's #15427.

@caesarxuchao caesarxuchao added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Oct 14, 2015
@k8s-bot
Copy link

k8s-bot commented Oct 14, 2015

GCE e2e build/test failed for commit 02040694e21277b3b4c6421be5e5d95ec3cba8bb.

@k8s-github-robot
Copy link

Labelling this PR as size/L

@k8s-github-robot k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 14, 2015
derekwaynecarr
derekwaynecarr reviewed Oct 14, 2015
View reviewed changes
pkg/client/unversioned/discovery_client.go
// It creates a RESTClient based on the passed in config, but it doesn't rely
// on the Version, Codec, and Prefix of the config, because it uses AbsPath and
// takes the raw response.
func (d *DiscoveryClient) ServerAPIVersions() (groupVersions []string, err error) {
Copy link
Member

Choose a reason for hiding this comment

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

why wouldn't this just return an APIGroupList?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. I will let this function return APIGroupList and add a utility function that extracts groupVersions []string

@derekwaynecarr
Copy link
Member

I find the structure of this client a little confusing when I compare to all of our existing clients.

I would expect to see an Interface first.

type APIGroupLister interface {
  List() (*api.GroupList, error)
}

type APIResourceGroupLister interface {
  // maybe if groupVersion is * we can make that all groups
  APIResourcesByGroup(groupVersion string) APIResourceGroupsInterface
  // not needed if we support a wildcard for all
  APIResources() APIResourceGroupsInterface 
}

type APIResourceGroupInterface interface {
 List() (*api.APIResourceList, error)
}

@caesarxuchao
Copy link
Member Author

Sure, I will add the interfaces.

lavalamp
lavalamp reviewed Oct 14, 2015
View reviewed changes
pkg/client/unversioned/discovery_client.go

// NewDiscovery creates a new DiscoveryClient for the given config. This client
// can be used to discover supported resources in the API server.
func NewDiscovery(c *Config) (*ExtensionsClient, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Change return value :)

Also perhaps spell out the name, NewDiscoveryClient(...)

@lavalamp lavalamp assigned krousey and unassigned lavalamp Oct 14, 2015
@lavalamp
Copy link
Member

+1 to Derek's comments

@k8s-bot
Copy link

k8s-bot commented Oct 15, 2015

GCE e2e build/test failed for commit 5a3215c9d0151c662fa82ed109c646aea362b36e.

@k8s-bot
Copy link

k8s-bot commented Oct 15, 2015

GCE e2e build/test failed for commit ae6644cc87a17f696f365b1dfb66ec7076a90a5a.

@caesarxuchao caesarxuchao mentioned this pull request Oct 15, 2015
Merged
@caesarxuchao caesarxuchao force-pushed the discovery-client branch 2 times, most recently from 78f1854 to 6c6be50 Compare October 15, 2015 23:39
@caesarxuchao caesarxuchao changed the title [WIP] add a discovery-client add a discovery-client Oct 15, 2015
@caesarxuchao
Copy link
Member Author

@krousey this is ready. Could you take a look? Thanks.

@k8s-bot
Copy link

k8s-bot commented Oct 15, 2015

GCE e2e test build/test passed for commit 760ef0e1570e986f531bf84545eee7f8710ba4cd.

@k8s-bot
Copy link

k8s-bot commented Oct 15, 2015

GCE e2e test build/test passed for commit 78f18540ed5a028110f16d5cd0b943609e1035fb.

@k8s-bot
Copy link

k8s-bot commented Oct 16, 2015

GCE e2e test build/test passed for commit 6c6be50f2e9a64d47e584f61dde4b1f91af41753.

krousey
krousey reviewed Oct 16, 2015
View reviewed changes
pkg/client/unversioned/testclient/testclient.go
}
c.Invokes(action, nil)
resource, found := c.Resources[groupVersion]
if found {
Copy link
Contributor

Choose a reason for hiding this comment

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

You could just return c.Resources[groupVersion], nil because the default value for a missing value will be nil.

@k8s-bot
Copy link

k8s-bot commented Oct 16, 2015

GCE e2e test build/test passed for commit 0bded6dbf78820cbf545af38e4733b7e0dd98d7a.

@k8s-bot
Copy link

k8s-bot commented Oct 16, 2015

GCE e2e test build/test passed for commit 8ed323ad4cbbe4f9ea78e5d53fd4391e6f908700.

@krousey krousey added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 16, 2015
@k8s-bot
Copy link

k8s-bot commented Oct 16, 2015

GCE e2e test build/test passed for commit 323fb781e96a9cab8ff61d86564fc60ff2fef505.

@k8s-github-robot
Copy link

PR changed after LGTM, removing LGTM.

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 16, 2015
@caesarxuchao caesarxuchao mentioned this pull request Oct 16, 2015
Merged
@caesarxuchao caesarxuchao added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed cla: yes labels Oct 16, 2015
@caesarxuchao
Copy link
Member Author

add back lgtm as it's a squash

@k8s-bot
Copy link

k8s-bot commented Oct 16, 2015

GCE e2e test build/test passed for commit 5859da3.

@caesarxuchao caesarxuchao mentioned this pull request Oct 16, 2015
Closed
@caesarxuchao
Copy link
Member Author

The second build of Shippable does not begin in the entire afternoon...

@caesarxuchao caesarxuchao reopened this Oct 17, 2015
@caesarxuchao caesarxuchao mentioned this pull request Oct 17, 2015
Merged
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Oct 17, 2015

GCE e2e test build/test passed for commit 5859da3.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

k8s-github-robot pushed a commit that referenced this pull request Oct 17, 2015
Merge pull request #15659 from caesarxuchao/discovery-client
68717dd 
Auto commit by PR queue bot
@k8s-github-robot k8s-github-robot merged commit 68717dd into kubernetes:master Oct 17, 2015
@caesarxuchao
Copy link
Member Author

@smarterclayton's comment: #15808 (comment)

This is not using a significant chunk of our generic rest client helper, which means it won't benefit from the standard work we do there. This needs to at minimum use a RESTClient and a request.go to fetch the provided resources, instead of performing http.client calls directly.

janetkuo
janetkuo reviewed Oct 19, 2015
View reviewed changes
pkg/client/unversioned/discovery_client.go
return d.httpClient.Do(req)
}

// APIGroups returns the supported groups, with information like supported versions and the
Copy link
Member

Choose a reason for hiding this comment

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

Is APIGroups the function name? Many of the comments in this file contains incorrect function name.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. I submitted a commit to fix these, I guess the commit is lost during the squash. I'll submit a patch.

@caesarxuchao caesarxuchao mentioned this pull request Oct 26, 2015
Merged
k8s-github-robot referenced this pull request Oct 27, 2015
Merge pull request #16324 from caesarxuchao/automated-cherry-pick-of-…
b37afa6 
…#15659-upstream-release-1.1

Auto commit by PR queue bot
shyamjvs referenced this pull request in shyamjvs/kubernetes Dec 1, 2016
Merge pull request kubernetes#16324 from caesarxuchao/automated-cherr…
6de31e2 
…y-pick-of-#15659-upstream-release-1.1

Auto commit by PR queue bot
shouhong referenced this pull request in shouhong/kubernetes Feb 14, 2017
Merge pull request kubernetes#16324 from caesarxuchao/automated-cherr…
96d5bd6 
…y-pick-of-#15659-upstream-release-1.1

Auto commit by PR queue bot
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@krousey krousey

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@caesarxuchao @k8s-bot @k8s-github-robot @derekwaynecarr @lavalamp @krousey @janetkuo @k8s-teamcity-mesosphere @googlebot