Kubectl support for validating nested objects with different ApiGroups #25172

pwittrock · 2016-05-04T23:08:49Z

Pull Request Guidelines

Please read our contributor guidelines.
See our developer guide.
Follow the instructions for labeling and writing a release note for this PR in the block below.

kubectl now supports validation of nested objects with different ApiGroups (e.g. objects in a List)

#24089

pwittrock · 2016-05-04T23:09:56Z

cc @nikhiljindal

@lavalamp Is this something you should also take a look at?

pwittrock · 2016-05-04T23:10:31Z

pkg/api/testing/fuzzer.go

+			types := []runtime.Object{&api.Pod{}, &extensions.Deployment{}, &api.Service{}}
+			t := types[c.Rand.Intn(len(types))]
+			c.Fuzz(t)
+			by, _ := json.Marshal(t)


Should I panic if I get an error here?

Yes, and you shouldn't use json, you should use runtime.Encode. Raw JSON encoding won't set the api version & kind on the wire, which will make the object uninterpretable.

SGTM. Done.

pwittrock · 2016-05-04T23:12:00Z

Still need to add tests for this but, wanted to make sure the approach is correct.

lavalamp · 2016-05-05T00:01:06Z

general idea lgtm, thanks!

pwittrock · 2016-05-05T02:33:05Z

Added the tests. Janet I think this is ready for review.

pwittrock · 2016-05-05T22:36:54Z

cc @kubernetes/kubectl

pwittrock · 2016-05-05T23:01:48Z

All tests passing

janetkuo · 2016-05-06T00:37:49Z

pkg/api/validation/schema.go

@@ -62,27 +62,36 @@ type NullSchema struct{}
 func (NullSchema) ValidateBytes(data []byte) error { return nil }

 type SwaggerSchema struct {
-	api swagger.ApiDeclaration
+	api     swagger.ApiDeclaration
+	factory Schema // For delegating to  other api groups


I'm curious about why name it factory instead of something like delegate?

There's a redundant whitespace in the comment

I can change it to delegate. It is named factory because that is the name of the Schema type that it actually delegates to. WDYT?

janetkuo · 2016-05-06T00:39:27Z

Suggest adding a test in hack/test-cmd.sh for this case also

janetkuo · 2016-05-06T16:39:33Z

pkg/api/validation/schema.go

 	return schema, nil
 }

 // validateList unpacks a list and validate every item in the list.
 // It return nil if every item is ok.
 // Otherwise it return an error list contain errors of every item.
+// validateList unpacks a list and validate every item in the list.
+// It return nil if every item is ok.
+// Otherwise it return an error list contain errors of every item.


redundant comments here

smarterclayton · 2016-05-06T17:17:44Z

pkg/api/validation/schema.go

+// The SwaggerSchema will not be able to process objects in different ApiVersions unless they are vendored.
+func (s *SwaggerSchema) isDifferentApiVersion(obj interface{}) bool {
+	groupVersion := obj.(map[string]interface{})["apiVersion"]
+	return groupVersion.(string) != "" && s.api.ApiVersion != groupVersion.(string)


style: use len(string) == 0 for empty checks as per our style guide

pwittrock · 2016-05-06T20:57:38Z

Add hack/test-cmd.sh test case

pwittrock · 2016-05-06T22:09:22Z

PTAL

janetkuo · 2016-05-09T18:36:54Z

pkg/api/validation/schema.go

@@ -62,27 +63,33 @@ type NullSchema struct{}
 func (NullSchema) ValidateBytes(data []byte) error { return nil }

 type SwaggerSchema struct {
-	api swagger.ApiDeclaration
+	api      swagger.ApiDeclaration
+	delegate Schema // For delegating to  other api groups


nit: the comment has a redundant space here: to other

janetkuo · 2016-05-09T18:39:31Z

Only nits

pwittrock · 2016-05-10T00:13:44Z

PTAL

janetkuo · 2016-05-10T01:16:07Z

LGTM. Please squash and feel free to apply the tag.

…s (e.g. Lists containing objects in different api groups). Closes kubernetes#24089

pwittrock · 2016-05-10T03:14:39Z

SGTM. Squashed. Will apply lgtm when all tests pass.

pwittrock · 2016-05-10T17:37:24Z

@k8s-bot test this issue: #IGNORE

pwittrock · 2016-05-10T20:00:23Z

Applied the 'lgtm' label per-Janet's comment.

pwittrock · 2016-05-10T20:02:38Z

Applying 'p1' because this is blocking downstream PRs

pwittrock · 2016-05-11T15:08:25Z

@k8s-bot test this issue: #25479

k8s-github-robot · 2016-05-11T16:29:13Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2016-05-11T17:06:25Z

GCE e2e build/test passed for commit 680b2b9.

k8s-github-robot · 2016-05-11T17:11:54Z

Automatic merge from submit-queue

pwittrock assigned janetkuo May 4, 2016

pwittrock added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label May 4, 2016

pwittrock added this to the v1.3 milestone May 4, 2016

googlebot added the cla: yes label May 4, 2016

pwittrock reviewed May 4, 2016
View reviewed changes

k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 4, 2016

pwittrock force-pushed the kubectl-apiversion branch 4 times, most recently from c66c978 to 9ec208f Compare May 5, 2016 01:50

pwittrock force-pushed the kubectl-apiversion branch 3 times, most recently from 8f6c73e to 77fe815 Compare May 5, 2016 20:30

pwittrock added the release-note-label-needed label May 5, 2016

k8s-github-robot removed the release-note-label-needed label May 5, 2016

pwittrock added the release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. label May 5, 2016

janetkuo changed the title ~~Kubectl support for validating nested objects with different ApiGroup…~~ Kubectl support for validating nested objects with different ApiGroups May 5, 2016

janetkuo reviewed May 6, 2016
View reviewed changes

smarterclayton reviewed May 6, 2016
View reviewed changes

pwittrock force-pushed the kubectl-apiversion branch from 77fe815 to 09780c9 Compare May 6, 2016 20:35

janetkuo reviewed May 9, 2016
View reviewed changes

pwittrock force-pushed the kubectl-apiversion branch 2 times, most recently from 7263f75 to f4c814b Compare May 9, 2016 21:03

pwittrock removed the release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. label May 10, 2016

Kubectl support for validating nested objects with different ApiGroup…

680b2b9

…s (e.g. Lists containing objects in different api groups). Closes kubernetes#24089

pwittrock force-pushed the kubectl-apiversion branch from 42a12ef to 680b2b9 Compare May 10, 2016 02:44

pwittrock added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 10, 2016

pwittrock added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label May 10, 2016

k8s-github-robot merged commit 7ae1dab into kubernetes:master May 11, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kubectl support for validating nested objects with different ApiGroups #25172

Kubectl support for validating nested objects with different ApiGroups #25172

pwittrock commented May 4, 2016 •

edited

Loading

pwittrock commented May 4, 2016

pwittrock May 4, 2016

lavalamp May 4, 2016

pwittrock May 5, 2016

pwittrock commented May 4, 2016

lavalamp commented May 5, 2016

pwittrock commented May 5, 2016

pwittrock commented May 5, 2016

pwittrock commented May 5, 2016

janetkuo May 6, 2016 •

edited

Loading

janetkuo May 6, 2016

pwittrock May 6, 2016

janetkuo commented May 6, 2016 •

edited

Loading

janetkuo May 6, 2016

pwittrock May 6, 2016

smarterclayton May 6, 2016

pwittrock May 6, 2016

pwittrock commented May 6, 2016

pwittrock commented May 6, 2016

janetkuo May 9, 2016

pwittrock May 9, 2016

janetkuo commented May 9, 2016

pwittrock commented May 10, 2016

janetkuo commented May 10, 2016

pwittrock commented May 10, 2016

pwittrock commented May 10, 2016

pwittrock commented May 10, 2016

pwittrock commented May 10, 2016

pwittrock commented May 11, 2016

k8s-github-robot commented May 11, 2016

k8s-bot commented May 11, 2016

k8s-github-robot commented May 11, 2016

Kubectl support for validating nested objects with different ApiGroups #25172

Kubectl support for validating nested objects with different ApiGroups #25172

Conversation

pwittrock commented May 4, 2016 • edited Loading

Pull Request Guidelines

pwittrock commented May 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwittrock commented May 4, 2016

lavalamp commented May 5, 2016

pwittrock commented May 5, 2016

pwittrock commented May 5, 2016

pwittrock commented May 5, 2016

janetkuo May 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janetkuo commented May 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwittrock commented May 6, 2016

pwittrock commented May 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janetkuo commented May 9, 2016

pwittrock commented May 10, 2016

janetkuo commented May 10, 2016

pwittrock commented May 10, 2016

pwittrock commented May 10, 2016

pwittrock commented May 10, 2016

pwittrock commented May 10, 2016

pwittrock commented May 11, 2016

k8s-github-robot commented May 11, 2016

k8s-bot commented May 11, 2016

k8s-github-robot commented May 11, 2016

pwittrock commented May 4, 2016 •

edited

Loading

janetkuo May 6, 2016 •

edited

Loading

janetkuo commented May 6, 2016 •

edited

Loading