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 a bug with pluralization of third party resources #25374

Merged
merged 1 commit into from
Jun 1, 2016
Merged

Fix a bug with pluralization of third party resources #25374

merged 1 commit into from
Jun 1, 2016
+61 −24

Conversation

brendandburns
Copy link
Contributor

Fixes #25129

@kubernetes/sig-api-machinery

Analytics

@brendandburns brendandburns added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label May 9, 2016
@k8s-github-robot k8s-github-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 9, 2016
lavalamp
lavalamp reviewed May 9, 2016
View reviewed changes
pkg/master/master.go
resourceStorage := thirdpartyresourcedataetcd.NewREST(
generic.RESTOptions{Storage: m.thirdPartyStorage, Decorator: generic.UndecoratedStorage, DeleteCollectionWorkers: m.deleteCollectionWorkers}, group, kind)

apiRoot := makeThirdPartyPath("")

storage := map[string]rest.Storage{
strings.ToLower(kind) + "s": resourceStorage,
pluralResource: resourceStorage,
Copy link
Member

Choose a reason for hiding this comment

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

So we're relying on meta.KindToResource's pluralization code working for arbitrary strings? I guess that's an improvement to just appending an 's'. Should we instead allow them to put a pluralized version in the specification? @smarterclayton here's another great reason not to use plurals in our paths...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

KindToResource is what the kubectl client code uses afaik, so at least its consistent?

@bgrant0607 bgrant0607 assigned lavalamp and unassigned bgrant0607 May 9, 2016
@brendandburns
Copy link
Contributor Author

comments responded to.

@lavalamp lavalamp added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 9, 2016
@lavalamp
Copy link
Member

lavalamp commented May 9, 2016

LGTM

@liggitt liggitt mentioned this pull request May 10, 2016
Closed
@brendandburns
Copy link
Contributor Author

@k8s-bot node test this please flake: #25439

@brendandburns
Copy link
Contributor Author

@k8s-bot node e2e test this please flake: #25439

@k8s-github-robot
Copy link

@brendandburns
You must link to the test flake issue which caused you to request this manual re-test.
Re-test requests should be in the form of: k8s-bot test this issue: #<number>
Here is the list of open test flakes.

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 16, 2016
@brendandburns brendandburns added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 16, 2016
@k8s-github-robot
Copy link

@brendandburns
You must link to the test flake issue which caused you to request this manual re-test.
Re-test requests should be in the form of: k8s-bot test this issue: #<number>
Here is the list of open test flakes.

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 16, 2016
@brendandburns
Copy link
Contributor Author

@k8s-bot e2e test this please issue: #24794

@brendandburns brendandburns added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 23, 2016
@brendandburns brendandburns mentioned this pull request May 23, 2016
Closed
@a-robinson
Copy link
Contributor

Is this bug worth fixing for 1.3? If so, please add the 1.3 milestone.

@brendandburns
Copy link
Contributor Author

@lavalamp for the 1.3 call. I believe it's worth fixing.

@a-robinson
Copy link
Contributor

@lavalamp might not see this until Thursday. In his stead, I'll say this should be fixed for 1.3, and we can sort it out later if he disagrees.

@a-robinson a-robinson added this to the v1.3 milestone Jun 1, 2016
@wojtek-t
Copy link
Member

wojtek-t commented Jun 1, 2016

I agree with @a-robinson

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Jun 1, 2016

GCE e2e build/test passed for commit b2bf960.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 99fab4a into kubernetes:master Jun 1, 2016
@lavalamp
Copy link
Member

lavalamp commented Jun 3, 2016

Yes, in 1.3 is good, thanks!

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

@lavalamp lavalamp

Labels
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
Milestone
v1.3
Development

Successfully merging this pull request may close these issues.
8 participants
@brendandburns @lavalamp @k8s-github-robot @a-robinson @wojtek-t @k8s-bot @googlebot @bgrant0607