✨ Added support for empty group in kubebuilder api creation with v3+ #1404 #1679

prafull01 · 2020-09-13T21:25:36Z

Description of the change:

Added support for the kubebuilder api creation without providing any group. If no group is provided then instead of "group"."domain", apiVersion will have only "domain"
Added validation that you can't provide group and domain both as empty
Added multi group support for the empty group by not creating a directory for the empty group in the apis and controller directory.

Motivation for the change:

It will improve user experience in api creation using kubebuilder.
It will solve the workarounds used by user to just have their domain name as the apiVersion.

Fixes #1404

k8s-ci-robot · 2020-09-13T21:25:44Z

Welcome @prafull01!

It looks like this is your first PR to kubernetes-sigs/kubebuilder 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kubebuilder has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2020-09-13T21:25:44Z

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

gabbifish · 2020-09-14T08:20:33Z

/ok-to-test

prafull01 · 2020-09-14T08:50:04Z

/retest

gabbifish

Thank you for seeing #1404 through! I had some minor nits, but this looks good to me :)

pkg/model/resource/options.go

pkg/model/resource/resource.go

pkg/plugin/v2/scaffolds/internal/templates/main.go

pkg/plugin/v3/scaffolds/internal/templates/main.go

camilamacedo86

HI @prafull01.,

Really thank you for the contribution 👍 It is great. IMO we need to apply this only to v3+, we need make the tool be able to work with empty groups instead of replacing the empty string by core and it is missing this scenario to the generate_testadata.sh script.

However, it is great that you are looking for to address this need.

pkg/model/resource/options.go

pkg/model/resource/options_test.go

camilamacedo86 · 2020-09-14T11:40:05Z

Also, could you please update the title with the mojo: https://github.com/kubernetes-sigs/kubebuilder/blob/master/CONTRIBUTING.md#pr-process and add (v3+ only)? Also, please ensure that your commits are squashed.

camilamacedo86 · 2020-09-14T15:19:34Z

testdata/project-v3-multigroup/PROJECT

@@ -25,4 +25,6 @@ resources:
 - group: foo.policy
  kind: HealthCheckPolicy
  version: v1
+- kind: Lakers
+  version: v1


pkg/plugin/v2/api.go

pkg/model/resource/resource_test.go

camilamacedo86

/approve

Great work 🥇

pkg/model/resource/options.go

k8s-ci-robot · 2020-09-17T20:48:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

~~OWNERS~~ [camilamacedo86]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

camilamacedo86 · 2020-09-17T20:52:12Z

testdata/project-v3-multigroup/controllers/suite_test.go

+
+	By("bootstrapping test environment")
+	testEnv = &envtest.Environment{
+		CRDDirectoryPaths: []string{filepath.Join("..", "config", "crd", "bases")},


camilamacedo86 · 2020-09-17T20:52:54Z

/test pull-kubebuilder-e2e-k8s-1-17-0

camilamacedo86 · 2020-09-17T20:55:31Z

Hi @prafull01,

The coveralls check is not passing because it is missing cover the ValidateV2 with all same checks that is done for Validate + the group one. See: https://coveralls.io/builds/33545877/source?filename=pkg/model/resource/options.go#L88

prafull01 · 2020-09-17T21:13:44Z

Hi @prafull01,

The coveralls check is not passing because it is missing cover the ValidateV2 with all same checks that is done for Validate + the group one. See: https://coveralls.io/builds/33545877/source?filename=pkg/model/resource/options.go#L88

There are two ways I can handle it:

Use the same test data to validate against Validate() and ValidateV2() and in the same test cases.
Write the similar test case to test the ValidateV2(), however it will duplicate the lots of test cases.

How should we handle this?

camilamacedo86 · 2020-09-17T21:16:26Z

Write the similar test case to test the ValidateV2(), however it will duplicate the lots of test cases.

I think would be fine just duplicate as well. We can remove the tests when the V2 be removed.
If you face lint issues because of the duplication use //nolint:dupl to ignore the check and please just add a comment explaining that we are duplicating the cases for V2 but that would also be removed when v2 will no longer be supported.

Please, feel free to choose the way that you think that it would be better.

prafull01 · 2020-09-17T21:48:17Z

/test pull-kubebuilder-e2e-k8s-1-14-1

prafull01 · 2020-09-18T09:38:02Z

/assign @camilamacedo86

prafull01 · 2020-09-21T06:45:40Z

/assign @DirectXMan12

DirectXMan12 · 2020-09-21T22:57:21Z

minor nit: the commit message mentions the "core" directory, but that bit seems gone. Can you fix up the commit message. AIUI, with multigroup we'll end up with

pkgs/apis/v1/xyz_types.go # test.kubebuilder.io/v1
pkgs/apis/foo/v1/xyz_types.go # foo.test.kubebuilder.io/v1

Is that correct?

DirectXMan12 · 2020-09-21T22:57:55Z

(otherwise, I delegate to @camilamacedo86 for the lgtm ;-) )

prafull01 · 2020-09-22T01:51:09Z

minor nit: the commit message mentions the "core" directory, but that bit seems gone. Can you fix up the commit message. AIUI, with multigroup we'll end up with
pkgs/apis/v1/xyz_types.go # test.kubebuilder.io/v1
pkgs/apis/foo/v1/xyz_types.go # foo.test.kubebuilder.io/v1
Is that correct?

Fixed the commit message. Yes, the path you mentioned is correct for empty group and non empty group.

gabbifish

I had one nit about what looked like an unnecessary check, but otherwise this looks good to me!

gabbifish · 2020-09-22T08:04:51Z

pkg/plugin/v3/api.go

@@ -137,6 +137,10 @@ func (p *createAPIPlugin) Validate() error {
 		return err
 	}

+	if p.resource.Group == "" && p.config.Domain == "" {


I think this check is redundant, given that p.resource.Validate() (called above) never allows p.Config.Domain to be an empty string in the first place?

@gabbifish IMO this is a necessary check, while initializing a project (kb init) you can provide a domain which can be empty.

Validate() function doesn't check the value of domain, it only checks the group, version, kind and related values which are provided in create api command. (kb create api). Domain is provided at the init level. Hence this check is necessary to know a user do not provide domain and group both as empty.

prafull01 · 2020-09-25T13:14:49Z

@camilamacedo86 Rebase done. make generate done, to have the finalizers markers in the multi-group with empty group api(lakers).

prafull01 · 2020-09-25T13:36:33Z

/test pull-kubebuilder-e2e-k8s-1-15-3

camilamacedo86 · 2020-09-25T14:12:37Z

It shows great for me. I could not find any issue with that.
Also, received the @DirectXMan12 and @gabbifish oks.

So, it shows fine for we move forward
/lgtm

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 13, 2020

k8s-ci-robot requested review from DirectXMan12 and joelanford September 13, 2020 21:25

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 13, 2020

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 14, 2020

gabbifish reviewed Sep 14, 2020

View reviewed changes

pkg/model/resource/options.go Outdated Show resolved Hide resolved

pkg/model/resource/options.go Outdated Show resolved Hide resolved

prafull01 requested a review from gabbifish September 14, 2020 09:15