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

Allow multiple APIs to register for the same API Group #28414

Merged
merged 1 commit into from
Jul 22, 2016

Conversation

brendandburns
Copy link
Contributor

@brendandburns brendandburns commented Jul 2, 2016

Fixes #23831

@kubernetes/sig-api-machinery

Analytics


This change is Reviewable

@brendandburns brendandburns added area/apiserver release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jul 2, 2016
@brendandburns brendandburns added this to the v1.4 milestone Jul 2, 2016
@k8s-github-robot k8s-github-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 2, 2016
@roberthbailey
Copy link
Contributor

@brendandburns do you want to cherry pick this into the 1.3 branch?

@duglin
Copy link

duglin commented Jul 15, 2016

are you able to build this? I'm seeing errors such as:

pkg/api/unversioned/zz_generated.deepcopy.go:28: DeepCopy_unversioned_APIGroup redeclared in this block

but to be honest it might be my env.

@caesarxuchao
Copy link
Member

@duglin the error you see is caused by #25978, try make clean, then make all, which will generate the missing files.

@duglin
Copy link

duglin commented Jul 15, 2016

@caesarxuchao thanks!

@brendandburns
Copy link
Contributor Author

@lavalamp @caesarxuchao can I get a quick review on this?
@roberthbailey yes, I'll cherry-pick once it's in.

Thanks
--brendan

@caesarxuchao caesarxuchao self-assigned this Jul 22, 2016
@caesarxuchao
Copy link
Member

Thanks @brendandburns. LGTM.

@caesarxuchao caesarxuchao added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 22, 2016
@k8s-bot
Copy link

k8s-bot commented Jul 22, 2016

GCE e2e build/test passed for commit ca9a61b.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 63e23a2 into kubernetes:master Jul 22, 2016
@fabioy fabioy added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Jul 24, 2016
@polvi
Copy link
Contributor

polvi commented Jul 26, 2016

This does not seem to fix the issue. Tested at ed3a29b

$ cat example-tprs.yml 
apiVersion: extensions/v1beta1
description: A foo
kind: ThirdPartyResource
metadata:
  name: foo.example.com
versions:
- name: v1
---
apiVersion: extensions/v1beta1
description: A bar
kind: ThirdPartyResource
metadata:
  name: bar.example.com
versions:
- name: v1
$ kubectl create -f example-tprs.yml 
thirdpartyresource "foo.example.com" created
thirdpartyresource "bar.example.com" created
$ curl localhost:8080/apis/example.com/v1/
{
  "kind": "APIResourceList",
  "apiVersion": "v1",
  "groupVersion": "example.com/v1",
  "resources": [
    {
      "name": "bars",
      "namespaced": true,
      "kind": "Bar"
    }
  ]
}

},
},
}
err := master.InstallThirdPartyResource(api)
Copy link
Contributor

@hongchaodeng hongchaodeng Jul 27, 2016

Choose a reason for hiding this comment

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

This unit test doesn't equal to real cases.
See here:

    hasResource, err := t.master.HasThirdPartyResource(rsrc)
    if !hasResource {
        return t.master.InstallThirdPartyResource(rsrc)
    }

The bug was caused in HasThirdPartyResource returning true because the API Group has been registered before despite of different resource kind.

I tried to fix it by comparing both API group and resource kind. But I got error:

[restful] WebService with duplicate root path detected:

Seems like rest path duplicate reg. Any suggestion on how to handle the install rest thing? @brendandburns

/cc @polvi @xiang90

@brendandburns
Copy link
Contributor Author

@polvi let me try to repro this again. I did exactly the above with my branch...

@nebril
Copy link
Contributor

nebril commented Aug 2, 2016

The same thing is happening on c7ec11b

@brendandburns
Copy link
Contributor Author

@polvi @nebril I believe this is fixed in #29724

Can you give it a test?

k8s-github-robot pushed a commit that referenced this pull request Aug 14, 2016
Automatic merge from submit-queue

Fix third party APIResource reporting

@polvi @caesarxuchao @deads2k 

This "fixes" some additional bugs in third party `APIResourceList` reporting.

This code needs a bunch of cleanup, and more tests, but sending it out for a quick smell check review in case I'm doing something stupid.

Fixes the bug referenced here:  #28414 (comment) and in #23831

Fixes #25570
@goltermann
Copy link
Contributor

@brendandburns @pwittrock This is in v1.4 milestone, but with cherrypick tags. Is the intent still to get into 1.3.x at some point?

@foxish
Copy link
Contributor

foxish commented Sep 8, 2016

This already went into 1.4. Removing cherrypick tags.

cc @brendandburns

@foxish foxish removed cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. cherrypick-candidate labels Sep 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/apiserver lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.