Support mixed case Kind #1133

mariantalla · 2019-10-28T10:45:20Z

This PR should address issues #1019 and #1091.

The implementation uses the validation logic that runs against Kind in
apimachinery; i.e. allows any Kind value that, when converted to
lowercase, is a valid DNS-1035 label.

Reusing that logic instead of reimplementing it helps with ensuring that
the two (i.e. Kind validation in core Kubernetes and Kind validation
in kubebuilder) are as similar as possible.

Would love feedback on:

The choice to use k8s.io/apimachinery logic vs continuing to extend the validation logic in kubebuilder.
If the above does sound like a good idea, is it worth refactoring Validate() more broadly, to use available logic in k8s.io/apimachinery?
There were more changes than I expected in go.mod 😅 I'll take an extra look at that.

Remaining work in this PR:
(once the approach seems reasonable)

Refactor tests to describe successful & failed validation more succinctly (at the moment there's duplication and missing test cases while the PR is in progress)
Ensure any logging and error messages are descriptive of the validation that takes place.

go.mod

mariantalla · 2019-10-28T17:07:18Z

/hold
(while there's open TODO items)

mariantalla · 2019-10-30T11:47:20Z

Any thoughts @mengqiy @DirectXMan12 @Liujingfang1 ?

/assign @mengqiy @DirectXMan12 @Liujingfang1

pkg/scaffold/v1/resource/resource.go

pkg/scaffold/v1/resource/resource_test.go

camilamacedo86 · 2019-11-04T23:46:29Z

go.mod

@@ -3,19 +3,17 @@ module sigs.k8s.io/kubebuilder
 go 1.12

 require (


Update the go.mo and go.sum shows not be part of the scope of this PR.
I'd recommend you revert it and ensure that your branch is rebased with the master.

Hi @mariantalla,

I still not getting why the modules need to be updated with these changes. So, I'd suggest to you:

Ensure that the branch is rebased with master

Revert the changes of go.mod and go.sum

Also, has nothing here to update the testdata. But, in order to test/check the change and be ensure I'd run make generate as well.

pkg/scaffold/v1/resource/resource.go

mariantalla · 2019-11-05T18:38:50Z

Thanks everyone for the comments. There's indeed a couple of follow-ups before this is on its way to merge (I've updated the PR description so it's clearer what work is left).

At the moment I was curious to hear how folks felt towards using k8s.io/apimachinery code for validation (as opposed to continuing to extend the validation that kubebuilder is doing). Once there's consensus in that, error messages and test cases are pretty straightforward to change.

pkg/scaffold/v1/resource/resource.go

mariantalla · 2019-11-11T11:17:17Z

Following the advice in #1048 (comment) (thanks @camilamacedo86 for the pointer), we've removed dependencies on apimachinery.

Adirio · 2019-11-11T12:36:00Z

The Travis error seems not to be related to this PR but to #1169

mariantalla · 2019-11-13T16:43:56Z

oops, forgot to
/hold cancel

pkg/scaffold/resource/resource.go

camilamacedo86 · 2019-11-13T16:48:05Z

pkg/scaffold/resource/resource_test.go

-		It("should fail if the Kind is not pascal cased", func() {
-			// Base case
-			instance := &Resource{Group: "crew", Kind: "FirstMate", Version: "v1"}
+		It("should succeed if the Kind is valid, according to core Kubernetes", func() {


IHMO the existent KInds check should pass.
Here for example : #1019 is for we allow AWSCluster

Agreed; this PR relaxes Kind validation (to bring it in line with the way it happens in core k8s). Previous passing tests continue to pass, and some previously failing should no longer fail (e.g. this one). Or am I missing something? 🤔

The validation obtained is not the correct one for we check the kinds. If you use the correct one the only change here will be adding more one test to ensure that AWSCluster will pass. Also, note that is required to remove the pascalze as well since it is not something check-in k8s API too.

pkg/scaffold/resource/resource.go

camilamacedo86

Following my suggestion to solve it.

Remove the Pascalize
Use the func https://github.com/kubernetes/apimachinery/blob/ced427e1ea5fdd2d307fa083f960961ceff71b65/pkg/util/validation/validation.go#L123 from k8s.io/apimachinery to check the Kind.
Then, add a unit test with the same Kind used in the issue that this PR has the purpose to solve AWSxpto

mariantalla · 2019-11-14T11:57:04Z

Thanks for the comments @camilamacedo86 ! I've responded inline; but just to make sure I got your last suggestion right:

Remove the Pascalize

I think this is happening here; where there other spots it needs to change?

Use the func ... from apimachinery

Kind validation now uses IsDns1350Label, same as in core kubernetes

Then, add a unit test

I think this test covers that case, is there something missing?

mariantalla · 2020-01-20T11:27:32Z

👋 Is there anything else missing here?

Adirio · 2020-01-20T11:31:27Z

I think you may want to rebase to solve the Travis issue and benefit from all the added lint checks that were added in the meantime.

DirectXMan12 · 2020-01-31T22:59:14Z

whoops, lost track of this. Can merge after rebase.

/assign @mengqiy
please do the rebase and we'll get this merge.

mengqiy · 2020-02-01T01:03:21Z

@mariantalla Sorry for the delay on this PR.
Can you please rebase, so we can get this merged.
Some files have been refactored recently by @Adirio, it may require a little more work to rebase.

Adirio · 2020-02-01T10:45:19Z

With the recent scaffolding it may be easier to branch from master and copy-paste your checks. pkg/model/resource/options.go should be where the check for Kind is placed, in the Validate method. pkg/model/resource/options_test.go should be where the additional tests go. Want some help?

mariantalla · 2020-02-03T11:08:25Z

Thanks! I'll make a start at rebasing, will poke you if it all goes wrong @Adirio :)

mariantalla · 2020-02-03T14:42:19Z

Did a go mod tidy too, which brought in the changes to go.mod and go.sum; let me know if we don't want these in.

Adirio · 2020-02-03T14:57:06Z

Did a go mod tidy too, which brought in the changes to go.mod and go.sum; let me know if we don't want these in.

We should probably add a test to Travis that checks that go mod tidy was executed so that we do not forget about it.

Adirio

I ended up wondering why 2 different DNS RFCs are used. Is Kubernetes using both or did it change to one of them and we still have the outdated version?

go.mod

pkg/model/resource/options_test.go

Adirio · 2020-02-03T17:11:27Z

Otherwise, LGTM.

The implementation * uses the same validation logic that runs against `Kind` in apimachinery; i.e. allows any `Kind` value that, when converted to lowercase, is a valid DNS-1035 label. * additionally requires that `Kind`s start with an uppercase character, to ensure they map to exported types. Co-Authored-By: Hannes Hörl <[email protected]> Co-Authored-By: Camila Macedo <[email protected]>

k8s-ci-robot · 2020-02-03T17:51:32Z

@mariantalla: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-kubebuilder-e2e	`5742ddd`	link	`/test pull-kubebuilder-e2e`

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.

mariantalla · 2020-02-03T17:52:49Z

/retest

camilamacedo86

/lgtm
for me wdyt @DirectXMan12 and @mengqiy ?

mengqiy

/lgtm
/approve

k8s-ci-robot · 2020-02-03T20:06:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mariantalla, mengqiy

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~~ [mengqiy]

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

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 28, 2019

k8s-ci-robot requested review from DirectXMan12 and Liujingfang1 October 28, 2019 10:45

mariantalla commented Oct 28, 2019

View reviewed changes

go.mod Outdated Show resolved Hide resolved

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 28, 2019

k8s-ci-robot assigned DirectXMan12, Liujingfang1 and mengqiy Oct 30, 2019

jdef reviewed Oct 31, 2019

View reviewed changes

pkg/scaffold/v1/resource/resource.go Outdated Show resolved Hide resolved

pkg/scaffold/v1/resource/resource_test.go Outdated Show resolved Hide resolved

camilamacedo86 reviewed Nov 4, 2019

View reviewed changes

Adirio reviewed Nov 4, 2019

View reviewed changes

pkg/scaffold/v1/resource/resource.go Outdated Show resolved Hide resolved

camilamacedo86 reviewed Nov 5, 2019

View reviewed changes

pkg/scaffold/v1/resource/resource.go Outdated Show resolved Hide resolved

DirectXMan12 mentioned this pull request Nov 6, 2019

Extract Resource from the v1 Scaffolding package #1144

Closed

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 11, 2019

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 11, 2019

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 13, 2019

camilamacedo86 reviewed Nov 13, 2019

View reviewed changes

pkg/scaffold/resource/resource.go Outdated Show resolved Hide resolved

camilamacedo86 reviewed Nov 13, 2019

View reviewed changes

pkg/scaffold/resource/resource.go Outdated Show resolved Hide resolved

camilamacedo86 requested changes Nov 13, 2019

View reviewed changes

Adirio mentioned this pull request Jan 9, 2020

Hotfix for TRAVIS error introduced by #1016 #1288

Closed

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 31, 2020

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 3, 2020

Adirio suggested changes Feb 3, 2020

View reviewed changes

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 3, 2020

Adirio reviewed Feb 3, 2020

View reviewed changes

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

Adirio approved these changes Feb 3, 2020

View reviewed changes

camilamacedo86 approved these changes Feb 3, 2020

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 3, 2020

mengqiy approved these changes Feb 3, 2020

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 3, 2020

k8s-ci-robot merged commit 6c6ab03 into kubernetes-sigs:master Feb 3, 2020

Adirio mentioned this pull request Feb 4, 2020

mixed up between camelCase and PascalCase in kind validation #1091

Closed

mariantalla mentioned this pull request Feb 4, 2020

Kind validation is a bit too strict #1019

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support mixed case Kind #1133

Support mixed case Kind #1133

mariantalla commented Oct 28, 2019 •

edited

Loading

mariantalla commented Oct 28, 2019

mariantalla commented Oct 30, 2019

camilamacedo86 Nov 4, 2019

camilamacedo86 Feb 3, 2020 •

edited

Loading

mariantalla commented Nov 5, 2019 •

edited

Loading

mariantalla commented Nov 11, 2019

Adirio commented Nov 11, 2019

mariantalla commented Nov 13, 2019

camilamacedo86 Nov 13, 2019

mariantalla Nov 14, 2019

camilamacedo86 Nov 14, 2019

camilamacedo86 left a comment •

edited

Loading

mariantalla commented Nov 14, 2019

mariantalla commented Jan 20, 2020

Adirio commented Jan 20, 2020

DirectXMan12 commented Jan 31, 2020

mengqiy commented Feb 1, 2020

Adirio commented Feb 1, 2020

mariantalla commented Feb 3, 2020

mariantalla commented Feb 3, 2020

Adirio commented Feb 3, 2020 •

edited

Loading

Adirio left a comment

Adirio commented Feb 3, 2020

k8s-ci-robot commented Feb 3, 2020 •

edited

Loading

mariantalla commented Feb 3, 2020

camilamacedo86 left a comment

mengqiy left a comment

k8s-ci-robot commented Feb 3, 2020

		@@ -3,19 +3,17 @@ module sigs.k8s.io/kubebuilder
		go 1.12

		require (

Support mixed case Kind #1133

Support mixed case Kind #1133

Conversation

mariantalla commented Oct 28, 2019 • edited Loading

mariantalla commented Oct 28, 2019

mariantalla commented Oct 30, 2019

camilamacedo86 Nov 4, 2019

Choose a reason for hiding this comment

camilamacedo86 Feb 3, 2020 • edited Loading

Choose a reason for hiding this comment

mariantalla commented Nov 5, 2019 • edited Loading

mariantalla commented Nov 11, 2019

Adirio commented Nov 11, 2019

mariantalla commented Nov 13, 2019

camilamacedo86 Nov 13, 2019

Choose a reason for hiding this comment

mariantalla Nov 14, 2019

Choose a reason for hiding this comment

camilamacedo86 Nov 14, 2019

Choose a reason for hiding this comment

camilamacedo86 left a comment • edited Loading

Choose a reason for hiding this comment

mariantalla commented Nov 14, 2019

mariantalla commented Jan 20, 2020

Adirio commented Jan 20, 2020

DirectXMan12 commented Jan 31, 2020

mengqiy commented Feb 1, 2020

Adirio commented Feb 1, 2020

mariantalla commented Feb 3, 2020

mariantalla commented Feb 3, 2020

Adirio commented Feb 3, 2020 • edited Loading

Adirio left a comment

Choose a reason for hiding this comment

Adirio commented Feb 3, 2020

k8s-ci-robot commented Feb 3, 2020 • edited Loading

mariantalla commented Feb 3, 2020

camilamacedo86 left a comment

Choose a reason for hiding this comment

mengqiy left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 3, 2020

mariantalla commented Oct 28, 2019 •

edited

Loading

camilamacedo86 Feb 3, 2020 •

edited

Loading

mariantalla commented Nov 5, 2019 •

edited

Loading

camilamacedo86 left a comment •

edited

Loading

Adirio commented Feb 3, 2020 •

edited

Loading

k8s-ci-robot commented Feb 3, 2020 •

edited

Loading