Add validate support for list kind

[linzichang]

fix #13812
@bgrant0607

[k8s-bot]

Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist")

If this message is too spammy, please complain to ixdy.

[k8s-github-robot]

Labelling this PR as size/M

[ncdc]

@kubernetes/rh-cluster-infra @liggitt @deads2k

[deads2k]

Isn't obj.(map[]) exactly what fields is?

[lavalamp]

Do not do this bare type assertion without checking the type of obj.

[bgrant0607]

cc @kubernetes/kubectl

[smarterclayton]

Are you doing this because of runtime.Unstructured? It would be better to just use runtime.Unstructured directly here, to be consistent with the rest of the list handling code.

[lavalamp]

Another bare type assertion-- I'll stop mentioning them. All of these should look like:

if got, ok := item.(map[string]interface{}); ok {
  doThing()
}

Or variations thereof.

[bgrant0607]

Filed #14775 re. tests for the feature.

We'll need to cherrypick this into the 1.1 branch after it merges.

[linzichang]

@lavalamp @bgrant0607 @liggitt @deads2k @smarterclayton Thanks for review. I have adjusted this code and add some type checking. And I can't found an api which can decode the []byte into an runtime.Object and fit runtime.IsListType. I have try runtime.UnstructuredJSONScheme.Decode and scheme.Decode. So I currently can only test the string "List". If you know other ways to do this better. Please tell me.
If this's ok to merge. Then I think there are two things should do in the future.

1. Add some APIs like runtime.IsList to support runtime.Unstructured and []data to be checked. The runtime.IsListType don't do full check.
2. Add tests

[smarterclayton]

runtime.IsList supporting runtime.Unstructured was the original intent of
the Unstructured type.

On Sep 30, 2015, at 8:53 AM, Zichang Lin notifications@github.com wrote:

@lavalamp https://github.com/lavalamp @bgrant0607
https://github.com/bgrant0607 @liggitt https://github.com/liggitt
@deads2k https://github.com/deads2k @smarterclayton
https://github.com/smarterclayton Thanks for review. I have adjusted this
code and add some type checking. And I can't found an api which can decode
the []byte into an runtime.Object and fit runtime.IsListType. I have try
runtime.UnstructuredJSONScheme.Decode and scheme.Decode. So I currently can
only test the string "List". If you know other way to do this better.
Please tell me.
If this's ok to merge. Then I think there are several thing should do in
the future.

1. Add some APIs like runtime.IsList to support runtime.Object and []data
   to be checked. The runtime.IsListType don't do full check.
2. Add tests

—
Reply to this email directly or view it on GitHub
#14726 (comment)
.

[deads2k]

Give the index of the list that is failing. for i, item := range items....

[deads2k]

Minor comments on the code. Please add a test that exercises this: positive and negative. I suspect that test-cmd.sh is the best location.

I'll create a followup issue to do validation with Unstructured. That was the intent, but I don't want to hold this pull on that.

[deads2k]

In thinking about this, I'd really like to have this loop check every item even if an early one fails. You can return a NewAggregate after the for loop. That way I can get all my failures at once instead of stumbling through one at a time.

[linzichang]

Yes, I'm thinking about this too. It's better to check every item in one time.

[linzichang]

Next I will add test and do validation with Unstructured. I'll take a vacation next week. So I can't code for about one week. But discussing is ok.

[k8s-github-robot]

@k8s-bot ok to test

pr builder appears to be missing, activating due to 'lgtm' label.

[k8s-bot]

GCE e2e test build/test passed for commit 755d740.

• Build Log
• Test Artifacts
• Internal Jenkins Results

[k8s-github-robot]

The author of this PR is not in the whitelist for merge, can one of the admins add the 'ok-to-merge' label?

[deads2k]

@liggitt can you add the tag?

[linzichang]

Please merge. Then I can go on #15085 .

[caesarxuchao]

The merge robot seems not working. @k8s-oncall, can we merge this manually?

[roberthbailey]

The merge robot was working earlier today. Sometime in the last few hours the UI has change significantly and the PR search box doesn't show any results for this PR.

/cc @brendandburns

[roberthbailey]

/cc @lavalamp

[lavalamp]

I started up a new version about 30 mins ago. It seems to not be aware of this PR at all.

[brendandburns]

Probably worth some manual debugging before we merge. If its still not in the queue tomorrow afternoon, we'll manually merge it.

[lavalamp]

oh ok this is in the queue dashboard now. It's in the (much shorter) queue of things that will be retested & merged.

[k8s-github-robot]

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

[k8s-bot]

GCE e2e test build/test passed for commit 755d740.

• Build Log
• Test Artifacts
• Internal Jenkins Results

[k8s-github-robot]

Automatic merge from submit-queue