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 singular resource names to discovery #43312

Merged
merged 1 commit into from
Mar 26, 2017
Merged

add singular resource names to discovery #43312

merged 1 commit into from
Mar 26, 2017

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented Mar 17, 2017 •
edited
Loading

Adds the singular resource name to our resource for discovery. This is something we've discussed to remove our pseudo-pluralization library which is unreliable even for english and really has no hope of properly handling other languages or variations we can expect from TPRs and aggregated API servers.

This pull simply adds the information to discovery, it doesn't not re-wire any RESTMappers.

@kubernetes/sig-cli-misc @kubernetes/sig-apimachinery-misc @kubernetes/api-review

API resource discovery now includes the `singularName` used to refer to the resource.

@deads2k deads2k added this to the v1.7 milestone Mar 17, 2017
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 17, 2017
@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: @lavalamp

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/XL Denotes a PR that changes 500-999 lines, ignoring generated files. release-note-label-needed labels Mar 17, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@deads2k deads2k force-pushed the cli-08-discovery branch 3 times, most recently from cde3468 to 6d906dd Compare March 20, 2017 13:45
lavalamp
lavalamp reviewed Mar 20, 2017
View reviewed changes
staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go Outdated
Name string `json:"name" protobuf:"bytes,1,opt,name=name"`
// singular is the singular name of the resource. This allows clients to handle plural and singular opaquely.
Singular string `json:"singular" protobuf:"bytes,6,opt,name=singular"`
Copy link
Member

Choose a reason for hiding this comment

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

I prefer "SingularName"

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

lavalamp
lavalamp reviewed Mar 20, 2017
View reviewed changes
staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go Outdated
Name string `json:"name" protobuf:"bytes,1,opt,name=name"`
// singular is the singular name of the resource. This allows clients to handle plural and singular opaquely.
Copy link
Member

Choose a reason for hiding this comment

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

Add to documentation where the plural name is used vs where the singular name is used.

@lavalamp
Copy link
Member

API change lgtm if you fix my comments.

Are you planning on doing a second PR or adding the implementation to this one?

@deads2k
Copy link
Contributor Author

deads2k commented Mar 20, 2017

Are you planning on doing a second PR or adding the implementation to this one?

Second PR. I may even see if I can con @p0lyn0mial into trying it :)

@lavalamp
Copy link
Member

@mbohlool is there a convenient place in OpenAPI to list this sort of metadata?

@sttts
Copy link
Contributor

sttts commented Mar 21, 2017

lgtm with SingularName as field name.

Let's also keep this in mind for TPRs. Now the kind and resource are linked, i.e. either singular or plural.

@k8s-github-robot k8s-github-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Mar 21, 2017
@k8s-github-robot k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 21, 2017
@deads2k
Copy link
Contributor Author

deads2k commented Mar 21, 2017

@k8s-bot cvm gce e2e test this

@deads2k deads2k added approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Mar 21, 2017
@deads2k
Copy link
Contributor Author

deads2k commented Mar 21, 2017

Name updated.

@mbohlool
Copy link
Contributor

Generated and non-generated code are both in one commit that make it hard to review.

deads2k
deads2k commented Mar 21, 2017
View reviewed changes
staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go
// singularName is the singular name of the resource. This allows clients to handle plural and singular opaquely.
// The singularName is more correct for reporting status on a single item and both singular and plural are allowed
// from the kubectl CLI interface.
SingularName string `json:"singularName" protobuf:"bytes,6,opt,name=singularName"`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mbohlool only line of meat here.

@mbohlool
Copy link
Contributor

Yep, I saw 54 files changes and was looking for some kind of implementation for this :). In future if we split the commits, it make it really easy to review.

Regarding openapi, the implementation should also add a tag to openapi spec of that model. e.g., adding a "x-kubernetes-plural-name": "pods" to pod spec. To add a tag to a type, you can follow the example here (GetDefinitionName function): https://github.com/kubernetes/apiserver/blob/aa75ef0b36d8ca79160d9f8f3e8a9b926ee30c1b/pkg/endpoints/openapi/openapi.go#L163

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 25, 2017
@fejta fejta removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 25, 2017
@deads2k
Copy link
Contributor Author

deads2k commented Mar 25, 2017

@k8s-bot kops aws e2e test this
@k8s-bot bazel test this

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 43429, 43416, 43312, 43141, 43421)

@k8s-github-robot k8s-github-robot merged commit bc0171c into kubernetes:master Mar 26, 2017
@deads2k deads2k deleted the cli-08-discovery branch August 3, 2017 20:07
@cben cben mentioned this pull request Oct 30, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sttts sttts sttts left review comments

@lavalamp lavalamp lavalamp left review comments

Assignees

@lavalamp lavalamp

@smarterclayton smarterclayton

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 Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v1.7
Development

Successfully merging this pull request may close these issues.
9 participants
@deads2k @k8s-github-robot @k8s-reviewable @lavalamp @sttts @mbohlool @fejta @smarterclayton @k8s-ci-robot