-
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
✨ Add --plural flag #1802
✨ Add --plural flag #1802
Conversation
@@ -118,13 +118,15 @@ type GVK struct { | |||
Group string `json:"group,omitempty"` | |||
Version string `json:"version,omitempty"` | |||
Kind string `json:"kind,omitempty"` | |||
Plural string `json:"plural,omitempty"` |
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 need a test to ensure these changes. WDYT about add/update the testdata samples with this scenario? See that it means that 1 resource for v3-projects should use it at least. Also, will it work fine for the addon scenario?
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.
No idea about the addon scenario, but I guess it shouldn't affect as it was being passed a Resource
which already ahs this field (it was always set with flect.Pluralize(strings.ToLower(resource.Kind))
instead of providing the user the ability to define it).
Will add the test later.
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.
IMHO, this actually doesn't solve the complete problem. It doesn't add the controller-gen markers which will generate the special plurals in the CRD. I copied this change locally and tried following scenario:
prafull@EMPID18004:~/go/src/github.com/prafull01/kubebuilder/proj$ ../bin/kubebuilder create api --group=praf --kind=Fernandez --version=v1 --plural=Fernandezes
Create Resource [y/n]
y
Create Controller [y/n]
y
Writing scaffold for you to edit...
api/v1/fernandez_types.go
controllers/fernandez_controller.go
Running make:
$ make
/home/prafull/go/bin/controller-gen object:headerFile="hack/boilerplate.go.txt" paths="./..."
go fmt ./...
go vet ./...
go build -o bin/manager main.go
PROJECT file:
prafull@EMPID18004:~/go/src/github.com/prafull01/kubebuilder/proj$ cat PROJECT
domain: my.domain
layout: go.kubebuilder.io/v3-alpha
projectName: proj
repo: sigs.k8s.io/kubebuilder/v2
resources:
- group: praf
kind: Fernandez
plural: Fernandezes
version: v1
version: 3-alpha
CRD file:
group: praf.my.domain
names:
kind: Fernandez
listKind: FernandezList
plural: fernandezs
singular: fernandez
scope: Namespaced
It should add marker (here) in the api/_types.go file which will actually add the plurals in the CRD as well.
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.
I am not sure if we should add a new flag. Could not it be solved by fixing the plural? Also, see: https://github.com/kubernetes-sigs/controller-tools/pull/522/files
WDYT about we raise an issue with the problematic scenarios and the use cases where the flag should be used for we discuss if we should add the flag or not?
I think they are 2 different topics.
IMO #1805 needs to be adressed to solve the bug and then we could discuss if this PR is needed. IMHO, we should add it at least to P.S.: once #1805 is solved, I will update this PR. |
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.
after we chat in the bug triage meeting over it all shows that we all agreed that it is fine and we can address that for v3+plugin. So, for we are able to get it merged in POV we need to:
- add a valid scenario in the testdata (generate samples)
- to do the changes only for v3+
- the mojo needs to be breaking change. The flag was changed for webhooks
- make clear in the title the changes made ( e.g add same option flag for webhooks and create api to allow customize pluralizations.
- rebase with master
Those are already done
That only is true for v3 which is in alpha, do we need to consider it breaking? (previously it was changing it for v2 too, but not any more) |
HI @Adirio,
The mojo only helps us to know that something was removed and it is braking change. Over what is missing here to get it merged, I think that we need to:
|
The thing is that this PR is no longer removing the About the tests, I do agree they need to be added. I'm already coordinating that with @gabbifish who is assigned to this issue. |
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.
Would be nice add scenario to check it in the testdata before its get merged. Otherwise,
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Adirio, camilamacedo86 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 |
@gabbifish was assigned this topic IIRC, and there were additional changes that needed to be done to fully supporting irregular plurals. That is the reason why this is holded. The flag itself is incremental so could be added after 3.0.0 without it being breaking, but the other changes may not be non-breaking. |
…r kubebuilder create webhook from --resource to --plural Signed-off-by: Adrian Orive <adrian.orive.oneca@gmail.com>
Hi @Adirio, By checking your comment #1802 (comment) I understand that it is in WIP. Could you please update its title and add in the first comment what is missing to get done? Also, could you please consider in this todo list add a scenario to cover this change in the testdata? |
It has changes that needs to be in place
@Adirio: 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/test-infra repository. I understand the commands that are listed here. |
@Adirio: 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. |
Description
Add
--plural
flag tokubebuilder create api
to allow irregular plural forms. Also, replace the--resource
flag for--plural
inkubebuilder create webhook
forv3-alpha
projects. This change was not done tov2
as it would be a breaking change.Motivation
https://kubernetes.slack.com/archives/CAR30FCJZ/p1604861145449700
/kind feature