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

Fix for multiple thirdparty resources with same group #24299

Closed
wants to merge 2 commits into from
Closed

Fix for multiple thirdparty resources with same group #24299

wants to merge 2 commits into from

Conversation

uruddarraju
Copy link

@uruddarraju uruddarraju commented Apr 14, 2016 •
edited by k8s-oncall

More details here #23831

@lavalamp

This change is Reviewable

@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Apr 14, 2016
@uruddarraju
Copy link
Author

cc @ashw7n

@lavalamp lavalamp assigned nikhiljindal and unassigned mikedanese Apr 15, 2016
@lavalamp
Copy link
Member

I think @nikhiljindal is a good person to review this.

@lavalamp lavalamp added ok-to-merge release-note-none Denotes a PR that doesn't merit a release note. and removed needs-ok-to-merge labels Apr 15, 2016
nikhiljindal
nikhiljindal reviewed Apr 19, 2016
View reviewed changes
pkg/apiserver/apiserver.go
@@ -137,23 +137,33 @@ func (g *APIGroupVersion) InstallREST(container *restful.Container) error {
// this method will return an error.
func (g *APIGroupVersion) UpdateREST(container *restful.Container) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this function was not being used anywhere before your change :)

@nikhiljindal
Copy link
Contributor

Thanks for sending the PR @uruddarraju
I have added a few comments.

Would also be great if you can add test cases to ensure that your use case is not broken again.
You can add unit tests or an e2e test or a kubectl test in hack/test-cmd.sh :)

@polvi
Copy link
Contributor

polvi commented Apr 28, 2016

I tested this manually and it fixed the issue I am seeing in #24394

@uruddarraju uruddarraju mentioned this pull request Apr 28, 2016
Closed
@k8s-github-robot
Copy link

@uruddarraju PR needs rebase

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 22, 2016
@k8s-bot
Copy link

k8s-bot commented Aug 2, 2016

GCE e2e build/test passed for commit b000309.

@sitepodmatt
Copy link
Contributor

Merging this would still put TPRs in a much better state than currently.

@MHBauer
Copy link
Contributor

MHBauer commented Aug 4, 2016

@sitepodmatt Would this make it into the next version of 1.3 if it was merged, or all the way to 1.4? If it's 1.4, I think we can wait for tests. The reason TPR in such a state in the first place is that there are no tests.

@lavalamp
Copy link
Member

lavalamp commented Aug 4, 2016

1.4 is closing in like 2.5 weeks so it would be super good if we could get this in asap.

@MHBauer
Copy link
Contributor

MHBauer commented Aug 18, 2016

Is this still needed or is the problem fixed by #28414 ?

@k8s-bot
Copy link

k8s-bot commented Aug 30, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message will repeat several times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

@duglin
Copy link

duglin commented Sep 17, 2016

@MHBauer you still seeing issues in this space?

@nikhiljindal nikhiljindal removed their assignment Oct 11, 2016
@jayunit100
Copy link
Member

Do we have adequete third-party test coverage in [Conformance] e2es cc @kubernetes/sig-testing / @ingvagabund (last worked on it i think) ... ? I know there is some stuff in test/e2e/third-party.go, but im wondering if it covers/should cover lifecycle/recreate/etc.

@spiffxp
Copy link
Member

spiffxp commented Nov 7, 2016

Do we have adequete third-party test coverage in [Conformance] e2es

define "adequate", are we trying to validate that the third-party interface works as advertised? does a "conformant" kubernetes have to implement/support third-party resources

@k8s-github-robot
Copy link

This PR hasn't been active in 60 days. It will be closed in 29 days (Feb 5, 2017).

cc @lavalamp @uruddarraju

You can add 'keep-open' label to prevent this from happening, or add a comment to keep it open another 90 days

@lavalamp
Copy link
Member

Adding the keep-open label because I still think we want this or something like it.

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] Needs approval from an approver in each of these OWNERS Files:

We suggest the following people:
cc @thockin
You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@bgrant0607
Copy link
Member

cc @adohe

This was referenced Feb 7, 2017
Closed
Closed
@adohe-zz
Copy link

adohe-zz commented Feb 8, 2017

@bgrant0607 get that, I will take a look.

@grodrigues3 grodrigues3 added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 5, 2017
@wojtek-t wojtek-t removed the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Jul 12, 2017
@enisoc enisoc added this to Assigned in CustomResourceDefinition Jul 13, 2017
@bgrant0607
Copy link
Member

TPRs have been superseded by Custom Resources.

cc @enisoc

@bgrant0607 bgrant0607 closed this Aug 10, 2017
@enisoc enisoc moved this from Assigned to Done in CustomResourceDefinition Aug 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@lavalamp lavalamp

@brendandburns brendandburns

Labels
needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
No open projects
CustomResourceDefinition
Done
Milestone
next-candidate
Development

Successfully merging this pull request may close these issues.

None yet