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

initial types for TPRs #45115

Merged
merged 5 commits into from
May 3, 2017
Merged

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented Apr 28, 2017

This pull starts creating the types described by https://github.com/kubernetes/community/blob/master/contributors/design-proposals/thirdpartyresources.md . In the initial pull different names were suggested. I've started this pull with CustomResource.apiextensions.k8s.io.

The structure begins as a separate API server to facilitate rapid prototyping and experimentation, but the end result will be added to the end of the kube-apiserver chain as described in https://github.com/kubernetes/community/blob/master/sig-api-machinery/api-extensions-position-statement.md .

Because it is separate to start (not included in any default server), I don't think we need a perfect name, but I'd like to be close.

@kubernetes/sig-api-machinery-misc @enisoc @smarterclayton @erictune

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 28, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: deads2k
We suggest the following additional approver: smarterclayton

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

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

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels Apr 28, 2017
@deads2k deads2k force-pushed the tpr-07-types branch 2 times, most recently from dbb608a to 9d6b5a2 Compare May 1, 2017 19:53

// CustomResourceNames indicates the names to serve this CustomResource
type CustomResourceNames struct {
// Plural is the plural name of the resource to serve. It must match the name of the TPR-registration
Copy link
Contributor

Choose a reason for hiding this comment

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

Would also mention "lower-case" here.

@@ -0,0 +1,116 @@
/*
Copyright 2016 The Kubernetes Authors.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: 2017


import metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

// CustomResourceSpec describe how a user wants their resource to appear
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: describes

Conditions []CustomResourceCondition

// AcceptedNames are the names that are actually being used to serve discovery
// They may be different than the names in spec.
Copy link
Contributor

Choose a reason for hiding this comment

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

different = a subset?

Copy link
Contributor

Choose a reason for hiding this comment

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

Forseeing clients which do complicated comparison logic whether everything specified is actually accepted. Can we at least express this with a boolean Accepted: Partially/Complete ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

different = a subset?

No, different. Imagine someone changing their shortnames. The status may have values not present in the spec.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we at least express this with a boolean Accepted: Partially/Complete ?

It would set the nameconflict condition.

@deads2k
Copy link
Contributor Author

deads2k commented May 2, 2017

@k8s-bot unit test this: issue #45170

@sttts
Copy link
Contributor

sttts commented May 2, 2017

Looks good from my side as a first step. As nothing is exposed yet, we can freely change types and names in a follow-up if necessary.

@deads2k
Copy link
Contributor Author

deads2k commented May 2, 2017

@k8s-bot verify test this

@deads2k
Copy link
Contributor Author

deads2k commented May 2, 2017

Looks good from my side as a first step. As nothing is exposed yet, we can freely change types and names in a follow-up if necessary.

@ericchiang Anything to add at the moment? I'm flexible on names both now and later.

Copy link
Contributor

@ericchiang ericchiang left a comment

Choose a reason for hiding this comment

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

Types work for me. One question about the API group name.

limitations under the License.
*/

package v1alpha1
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, I had assumed this would be a beta group based on the proposal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, I had assumed this would be a beta group based on the proposal.

I really want to make it to beta, but I don't want to rush it and later regret it. If we decide this API is actually good enough to be beta, I'm fine with simply switching it later this release or adding a congruent file next release.

@deads2k
Copy link
Contributor Author

deads2k commented May 3, 2017

Looks like there's enough consensus to move forward with a non-exposed API like this. I am flexible about changing these names and I've opened #45277 to make sure that's well known.

I'm going to tag this so we can get a critical mass available for dealing with the issues around actually making it work and playing with migration cases.

@deads2k deads2k added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 3, 2017
@deads2k deads2k added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed release-note-label-needed labels May 3, 2017
@k8s-github-robot k8s-github-robot added release-note-label-needed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. labels May 3, 2017
@deads2k deads2k added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. release-note-label-needed labels May 3, 2017
@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 45272, 45115)

@k8s-github-robot k8s-github-robot merged commit df8551a into kubernetes:master May 3, 2017
k8s-github-robot pushed a commit that referenced this pull request May 6, 2017
Automatic merge from submit-queue (batch tested with PRs 45182, 45429)

CustomResources in separate API server

Builds on #45115.

This adds a basic handler for custom resources.  No status handling, no finalizers, no controllers, but basic CRUD runs to allow @enisoc and others to start considering migration.

@kubernetes/sig-api-machinery-misc
@deads2k deads2k deleted the tpr-07-types branch August 3, 2017 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants