-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
🐛 Fix the kubebuilder api creation when resource creation is set to false #1770
Conversation
Hi @prafull01. 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. |
/cc @camilamacedo86 |
/ok-to-test |
pkg/plugin/v2/scaffolds/internal/templates/controller/controller.go
Outdated
Show resolved
Hide resolved
pkg/plugin/v2/scaffolds/internal/templates/controller/controller_suitetest.go
Outdated
Show resolved
Hide resolved
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 seems correct, but I haven't checked if the scaffolded files work properly.
I remember talking about a related topic with @DirectXMan12 some months ago. Having the WireResource
field set to false is not enough to know if the CRD is an external one. For example, imagine the following sequence:
kubebuilder init
kubebuilder create api --group ship --version v1beta1 --kind Frigate --resource=true --controller=false
kubebuilder create api --group ship --version v1beta1 --kind Frigate --resource=false --controller=true
This case should be supported, maybe at the start we don't think we need a controller or we just want to do it in tweo steps.
@camilamacedo86 IMO, we should add the new attribute to the @Adirio The case you have mentioned is supported, the only thing which will be missing is: WDYT? |
You meak using this as a hot-fix for In that case, wherever a input is required from the user, at least a comment should be placed. For example in the |
Hi @prafull01 and @Adirio, I removed my latest commend and I will try to do centralize all in just one. I tested locally the scenario added by @Adirio
And it worked with changes added here. Also, see that it was added in the tests as well. IMO we could have the values of the flags info Regards this solution I am OK with it. Really thank you for the collab @prafull01, it is /approve for me 👍 |
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.
/approve
P.S.: the scenario I mentioned and Camila tested does get scaffolded, but some lines are not written to the file as if we didn't have the CRD inside the project when we do. |
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.
The four suggested changes need to be applied also to the v3 equivalent file.
pkg/plugin/v2/scaffolds/internal/templates/controller/controller.go
Outdated
Show resolved
Hide resolved
pkg/plugin/v2/scaffolds/internal/templates/controller/controller.go
Outdated
Show resolved
Hide resolved
pkg/plugin/v2/scaffolds/internal/templates/controller/controller.go
Outdated
Show resolved
Hide resolved
pkg/plugin/v2/scaffolds/internal/templates/controller/controller.go
Outdated
Show resolved
Hide resolved
/test pull-kubebuilder-e2e-k8s-1-17-0 |
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.
Still some changes needed
pkg/plugin/v2/scaffolds/internal/templates/controller/controller.go
Outdated
Show resolved
Hide resolved
pkg/plugin/v2/scaffolds/internal/templates/controller/controller.go
Outdated
Show resolved
Hide resolved
pkg/plugin/v3/scaffolds/internal/templates/config/controller/controller.go
Outdated
Show resolved
Hide resolved
pkg/plugin/v3/scaffolds/internal/templates/config/controller/controller.go
Outdated
Show resolved
Hide resolved
pkg/plugin/v3/scaffolds/internal/templates/config/controller/controller.go
Show resolved
Hide resolved
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.
Still some changes needed
My bad, apparently doing changes fastly so missed those. Fixed it. Thanks 👍
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Adirio, camilamacedo86, prafull01 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 |
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.
/lgtm
Description
Fix the
kb create api
command when the create resource is set to false.Added tests in the generate_testdata.sh so that such issue doesn't occur again.
Motivation*
Fixes: #1767