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 to specify categories for custom resources #59561

Merged
merged 2 commits into from Feb 23, 2018

Conversation

@nikhita
Member

nikhita commented Feb 8, 2018

Allow to specify categories for custom resources so that we can get it working with kubectl get all.

Adds a new field Categories in the CRD spec.

Release note:

Custom resources can be listed with a set of grouped resources (category) by specifying the categories in the CustomResourceDefinition spec. Example: They can be used with `kubectl get all`, where `all` is a category.

/cc sttts liggitt deads2k

// Categories implements the CategoriesProvider interface. Returns a list of categories a resource is part of.
func (r *REST) Categories() []string {
return []string{"all"}

This comment has been minimized.

@sttts

sttts Feb 8, 2018

Contributor

Not sure we really want that. The resources in all are manually curated because we don't want to see certain resources in there that are internal and not relevant for a user.

This comment has been minimized.

@deads2k

deads2k Feb 8, 2018

Contributor

Yeah, we don't really want this here. I could see allowing a user to specify their categories.

This comment has been minimized.

@nikhita

nikhita Feb 9, 2018

Member

I could see allowing a user to specify their categories.

Maybe as another field in the CRD spec (?)

This comment has been minimized.

@liggitt

liggitt Feb 9, 2018

Member

Exactly

// Categories implements the CategoriesProvider interface. Returns a list of categories a resource is part of.
func (r *REST) Categories() []string {
return []string{"all"}

This comment has been minimized.

@deads2k

deads2k Feb 8, 2018

Contributor

all is for user facing resources. CRDs are highly privileged. They don't belong in the all list.

This comment has been minimized.

This comment has been minimized.

@liggitt

liggitt Feb 8, 2018

Member

@deads2k: I could see allowing a user to specify their categories.

that is what I meant

@nikhita nikhita referenced this pull request Feb 9, 2018

Open

Umbrella issue for CRDs moving to GA #58682

17 of 53 tasks complete

@k8s-ci-robot k8s-ci-robot added size/M and removed size/S labels Feb 11, 2018

@nikhita nikhita changed the title from apiextensions: implement CategoriesProvider to [WIP] Allow to specify categories for custom resources Feb 12, 2018

@@ -30,6 +30,8 @@ type CustomResourceDefinitionSpec struct {
Scope ResourceScope
// Validation describes the validation methods for CustomResources
Validation *CustomResourceValidation
// Categories is a list of grouped resources custom resources belong to (e.g. 'all')
Categories []string

This comment has been minimized.

@liggitt

liggitt Feb 12, 2018

Member

I expected this next to ShortNames, is there a reason not to put it there?

This comment has been minimized.

@nikhita

nikhita Feb 12, 2018

Member

I expected this next to ShortNames, is there a reason not to put it there?

My first instinct was to include it next to ShortNames as well.

However, CustomResourceDefinitionNames is unique to the CRD and is used in the naming_controller too. Categories can be the same across CRDs and we don't really need to include it in the naming_controller. So I thought we could include it as a field in the CRD Spec...

Putting it in CustomResourceDefinitionNames should also be fine. @liggitt Wdyt?

This comment has been minimized.

@nikhita

nikhita Feb 20, 2018

Member

Done.

@@ -158,7 +158,6 @@ func ValidateCustomResourceDefinitionNames(names *apiextensions.CustomResourceDe
if errs := validationutil.IsDNS1035Label(shortName); len(errs) > 0 {
allErrs = append(allErrs, field.Invalid(fldPath.Child("shortNames").Index(i), shortName, strings.Join(errs, ",")))
}

This comment has been minimized.

@liggitt

liggitt Feb 12, 2018

Member

need to validate categories (probably the same validation we apply to shortnames)

This comment has been minimized.

@nikhita

nikhita Feb 12, 2018

Member

Added.

@nikhita

This comment has been minimized.

Member

nikhita commented Feb 12, 2018

To confirm - do we need a feature gate for this? Technically, it is an alpha field in an already beta API...

@nikhita

This comment has been minimized.

Member

nikhita commented Feb 13, 2018

/kind api-change

@sttts

This comment has been minimized.

Contributor

sttts commented Feb 19, 2018

Needs a release note.

@nikhita nikhita changed the title from [WIP] Allow to specify categories for custom resources to Allow to specify categories for custom resources Feb 19, 2018

@sttts

This comment has been minimized.

Contributor

sttts commented Feb 19, 2018

do we need a feature gate for this? Technically, it is an alpha field in an already beta API...

I tend to add it as a beta field. This is simple enough IMO.

@deads2k @liggitt opinions?

@nikhita

This comment has been minimized.

Member

nikhita commented Feb 19, 2018

Needs a release note.

Added. Ptal.

@deads2k

This comment has been minimized.

Contributor

deads2k commented Feb 20, 2018

I tend to add it as a beta field. This is simple enough IMO.

I'm fine with it as beta. I also expected it to be near names though.

@nikhita

This comment has been minimized.

Member

nikhita commented Feb 20, 2018

I also expected it to be near names though.

Done. Ptal, thanks.

@sttts sttts added this to the v1.10 milestone Feb 21, 2018

}
// WithCategories sets the categories for the resource and returns the RESTStorage object.
func (r *REST) WithCategories(categories []string) *REST {

This comment has been minimized.

@deads2k

deads2k Feb 22, 2018

Contributor

I don't think you need this

This comment has been minimized.

@nikhita

nikhita Feb 23, 2018

Member

Removed and rebased.

@deads2k

This comment has been minimized.

Contributor

deads2k commented Feb 22, 2018

@@ -111,6 +111,7 @@ func (c *NamingConditionController) getAcceptedNamesForGroup(group string) (allR
allKinds.Insert(item.Status.AcceptedNames.Kind)
allKinds.Insert(item.Status.AcceptedNames.ListKind)
allKinds.Insert(item.Status.AcceptedNames.Categories...)

This comment has been minimized.

@liggitt

liggitt Feb 23, 2018

Member

categories are not kinds... why are we adding them here? multiple CRDs can share the same categories... no duplicate checking is required

This comment has been minimized.

@nikhita

nikhita Feb 23, 2018

Member

Removed.

@liggitt

This comment has been minimized.

Member

liggitt commented Feb 23, 2018

code looks good. I expected a test exercising the function (ensuring a specified category makes it through to the discovery doc)

nikhita added some commits Feb 8, 2018

Add Categories to CRD spec
We can group custom resources into categories i.e.
use them with kubectl get all.
@nikhita

This comment has been minimized.

Member

nikhita commented Feb 23, 2018

I expected a test exercising the function (ensuring a specified category makes it through to the discovery doc)

Added.

Also added a unit test.

@liggitt

This comment has been minimized.

Member

liggitt commented Feb 23, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 23, 2018

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Feb 23, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, liggitt, nikhita

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

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 23, 2018

Automatic merge from submit-queue (batch tested with PRs 60302, 57921, 59042, 60126, 59561). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit 8d406bf into kubernetes:master Feb 23, 2018

13 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation nikhita authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@nikhita nikhita deleted the nikhita:customresources-kubectl-get-all branch Feb 23, 2018

@bgrant0607

This comment has been minimized.

Member

bgrant0607 commented Apr 3, 2018

Do we plan to add categories to built-in types?

@sttts

This comment has been minimized.

Contributor

sttts commented Apr 3, 2018

Do we plan to add categories to built-in types?

Technically we can (for quite some time already): https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/registry/rest/rest.go#L76

Are you asking for more categories by default?

@bgrant0607

This comment has been minimized.

Member

bgrant0607 commented Apr 3, 2018

@sttts No, I'm not asking for more default categories. I'm trying to figure out why we need categories.

"all" (which is misleading) is the only default category?

This is just intended for CLI shortcuts similar to resource-type short names?

@sttts

This comment has been minimized.

Contributor

sttts commented Apr 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment