-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
馃悰 Synchonized plural generation with api-machinery #3433
Conversation
Hi @lauchokyip. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/ok-to-test
Could you please squash the commits ? @varshaprasad96 @everettraven wdyt about this one? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for trying to keep apimachinery and kubebuilder synced.
Quick question: this will probably break existing projects that already benefited from gobuffalo/flect
pluralize mechanism. Maybe we should allow users to pass a set of "override" plural names (via flag, env/var...?), so at least they can work around this change?
/hold Definitely feel like a breaking change |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @lauchokyip,
Could you please squash the commits?
Also, see that it is not passing in the lint you can run locally make lint locally to check it out.
By last, we need to provide a guidance for those that:
- Scaffold the project with an previous version, create an api and then get a new kubebuilder version to create another API
Could you please add the steps in the description of this PR? What guidance/steps should we provide in this case?
It seems for me that would not work at all and this one is a breaking change. For other hand it seems a valid / reasonable request. I am just trying to think if it could be addressed for v4 or if we should only perform this change for an future version like go/v5.
c/c @everettraven @varshaprasad96 @Kavinjsir wdyt about this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx for your contribution @lauchokyip ! Just give a first-round review and left a nit.
@camilamacedo86
I'm Okay to make it consistent with the upstream kb plural system, though I personally find it hacky. Basically, the upstream plural system looks far from 'perfect':
- The k8s plural implementation itself is kind of broken, like
helmswomans
andfishs
. Also, the implementation looks tricky, where it maintains a hardcodedunpluralizedSuffixes
. - Like as you mentioned, if users are migrating their operator projects to the new kubebuilder with such an updated plural system. It may probably bring breaking change. (Though, it might not be a common scenario.)
Also in the future, we may probably change it once again, as such plural system might not be the 'ultimate' solution. (And then users might meet the similar scenario one more time.)
pkg/model/resource/utils.go
Outdated
// unpluralizedSuffixes is a list of resource suffixes that are the same plural and singular | ||
// This is only is only necessary because some bits of code are lazy and don't actually use the RESTMapper like they should. | ||
// TODO eliminate this so that different callers can correctly map to resources. This probably means updating all | ||
// callers to use the RESTMapper they mean. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution @lauchokyip ! I have a couple suggested changes:
pkg/model/resource/utils.go
Outdated
// unpluralizedSuffixes is a list of resource suffixes that are the same plural and singular | ||
// This is only is only necessary because some bits of code are lazy and don't actually use the RESTMapper like they should. | ||
// TODO eliminate this so that different callers can correctly map to resources. This probably means updating all | ||
// callers to use the RESTMapper they mean. | ||
var unpluralizedSuffixes = []string{ | ||
"endpoints", | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we actually need this? Endpoints
is a core Kubernetes resource and we likely aren't going to have to differentiate this in kubebuilder.
pkg/model/resource/utils.go
Outdated
for _, skip := range unpluralizedSuffixes { | ||
if strings.HasSuffix(singularName, skip) { | ||
return singular | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can probably remove this as per my previous comment
pkg/model/resource/utils.go
Outdated
singularName := strings.ToLower(singular) | ||
for _, skip := range unpluralizedSuffixes { | ||
if strings.HasSuffix(singularName, skip) { | ||
return singular | ||
} | ||
} | ||
|
||
switch string(singularName[len(singularName)-1]) { | ||
case "s": | ||
return singularName + "es" | ||
case "y": | ||
return strings.TrimSuffix(singularName, "y") + "ies" | ||
} | ||
return singularName + "s" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something I just realized, the restmapper.UnsafeGuessKindToResource
that this logic is pulled from is an exported function and something that we could pull in as a dependency. Instead of fully copying this logic I would personally prefer using the exported function so we automatically benefit from any future improvements made to this function.
The main caveat here is that we would be responsible for converting the input string --> a schema.GroupVersionKind
and the schema.GroupVersionResource
returned by the restmapper.UnsafeGuessKindToResource
function --> string
pkg/model/resource/utils.go
Outdated
switch string(singularName[len(singularName)-1]) { | ||
case "s": | ||
return singularName + "es" | ||
case "y": | ||
return strings.TrimSuffix(singularName, "y") + "ies" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a piece of code in k8s/code-generator, that deals with this and some of the edge cases explicitly: https://github.com/kubernetes/gengo/blob/ab3349d207d4/namer/plural_namer.go#L36. May not be an immediate action item, but it would be preferable to have a TODO added here, to be able to resuse that piece of code. (It currently is very opinionated to spit out plurals for core k8s resources).
Hi @camilamacedo86 , do you mean providing the migration documents? Any guidance on how I could do that? I don't know the code well enough to answer this question - if the user generate a CRD named I will be voting towards putting this into |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good to me, thanks @lauchokyip !
Hi @everettraven @varshaprasad96 @Kavinjsir @lauchokyip, We have encountered several deprecations, and it's essential that we deprecate the declarative plugin as soon as possible. You can find the details in this pull request: #3395 To ensure a cleaner and more maintainable codebase as we continue to grow, we plan to remove all deprecations at the beginning of next year, leading to a major bump for Kubebuilder 4.0.0. However, since this change will have a significant impact on projects that are already scaffolded, I suggest we consider the following:
Please share your thoughts on these suggestions. |
@lauchokyip We are interested in resolving this issue. Could you please rebase this so that we revisit it again. Thanks for taking the time for working on this. |
Let me get back to this in a bit |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: everettraven, lauchokyip The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
HI @varshaprasad96 ,
Does |
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@lauchokyip Would you rebase the branch based off the latest master branch? There might be an issue for |
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
@lauchokyip: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Closed based on:
|
Fixes #3402