Fix pointer receivers handling in unstructured converter by ash2k · Pull Request #51505 · kubernetes/kubernetes

ash2k · 2017-08-29T07:53:07Z

What this PR does / why we need it:
Fixes unstructured converter to properly handle types that have MarshalJSON() implemented with a pointer receiver. In particular the converter now can roundtrip *Unstructured type.

Which issue this PR fixes:
Fixes #47889. Similar to #43346.

Special notes for your reviewer:
Without the fix the tests are failing:

make test WHAT=./vendor/k8s.io/apimachinery/pkg/conversion/unstructured

Running tests for APIVersion: v1,admissionregistration.k8s.io/v1alpha1,admission.k8s.io/v1alpha1,apps/v1beta1,apps/v1beta2,authentication.k8s.io/v1,authentication.k8s.io/v1beta1,authorization.k8s.io/v1,authorization.k8s.io/v1beta1,autoscaling/v1,autoscaling/v2alpha1,batch/v1,batch/v1beta1,batch/v2alpha1,certificates.k8s.io/v1beta1,extensions/v1beta1,imagepolicy.k8s.io/v1alpha1,networking.k8s.io/v1,policy/v1beta1,rbac.authorization.k8s.io/v1,rbac.authorization.k8s.io/v1beta1,rbac.authorization.k8s.io/v1alpha1,scheduling.k8s.io/v1alpha1,settings.k8s.io/v1alpha1,storage.k8s.io/v1beta1,storage.k8s.io/v1,,federation/v1beta1
+++ [0829 16:39:36] Running tests without code coverage
--- FAIL: TestCustomToUnstructured (0.00s)
    --- FAIL: TestCustomToUnstructured/false (0.00s)
        Error Trace:    converter_test.go:500
        Error:          Not equal:
                        expected: bool(false)
                        actual: map[string]interface {}(map[string]interface {}{"data":"ZmFsc2U="})
        Messages:       customPointer1
    --- FAIL: TestCustomToUnstructured/[1] (0.00s)
        Error Trace:    converter_test.go:500
        Error:          Not equal:
                        expected: []interface {}([]interface {}{1})
                        actual: map[string]interface {}(map[string]interface {}{"data":"WzFd"})
        Messages:       customPointer1
    --- FAIL: TestCustomToUnstructured/true (0.00s)
        Error Trace:    converter_test.go:500
        Error:          Not equal:
                        expected: bool(true)
                        actual: map[string]interface {}(map[string]interface {}{"data":"dHJ1ZQ=="})
        Messages:       customPointer1
    --- FAIL: TestCustomToUnstructured/0 (0.00s)
        Error Trace:    converter_test.go:500
        Error:          Not equal:
                        expected: int64(0)
                        actual: map[string]interface {}(map[string]interface {}{"data":"MA=="})
        Messages:       customPointer1
    --- FAIL: TestCustomToUnstructured/null (0.00s)
        Error Trace:    converter_test.go:500
        Error:          Not equal:
                        expected: <nil>(<nil>)
                        actual: map[string]interface {}(map[string]interface {}{"data":"bnVsbA=="})
        Messages:       customPointer1
    --- FAIL: TestCustomToUnstructured/[] (0.00s)
        Error Trace:    converter_test.go:500
        Error:          Not equal:
                        expected: []interface {}([]interface {}{})
                        actual: map[string]interface {}(map[string]interface {}{"data":"W10="})
        Messages:       customPointer1
    --- FAIL: TestCustomToUnstructured/{"a":1} (0.00s)
        Error Trace:    converter_test.go:500
        Error:          Not equal:
                        expected: map[string]interface {}{"a":1}
                        actual: map[string]interface {}{"data":"eyJhIjoxfQ=="}

                        Diff:
                        --- Expected
                        +++ Actual
                        @@ -1,3 +1,3 @@
                         (map[string]interface {}) (len=1) {
                        - (string) (len=1) "a": (int64) 1
                        + (string) (len=4) "data": (string) (len=12) "eyJhIjoxfQ=="
                         }
        Messages:       customPointer1
    --- FAIL: TestCustomToUnstructured/0.0 (0.00s)
        Error Trace:    converter_test.go:500
        Error:          Not equal:
                        expected: float64(0)
                        actual: map[string]interface {}(map[string]interface {}{"data":"MC4w"})
        Messages:       customPointer1
    --- FAIL: TestCustomToUnstructured/{} (0.00s)
        Error Trace:    converter_test.go:500
        Error:          Not equal:
                        expected: map[string]interface {}{}
                        actual: map[string]interface {}{"data":"e30="}

                        Diff:
                        --- Expected
                        +++ Actual
                        @@ -1,2 +1,3 @@
                        -(map[string]interface {}) {
                        +(map[string]interface {}) (len=1) {
                        + (string) (len=4) "data": (string) (len=4) "e30="
                         }
        Messages:       customPointer1
--- FAIL: TestCustomToUnstructuredTopLevel (0.00s)
    --- FAIL: TestCustomToUnstructuredTopLevel/1 (0.00s)
        Error Trace:    converter_test.go:519
        Error:          Not equal:
                        expected: map[string]interface {}{"a":1}
                        actual: map[string]interface {}{"data":"eyJhIjoxfQ=="}

                        Diff:
                        --- Expected
                        +++ Actual
                        @@ -1,3 +1,3 @@
                         (map[string]interface {}) (len=1) {
                        - (string) (len=1) "a": (int64) 1
                        + (string) (len=4) "data": (string) (len=12) "eyJhIjoxfQ=="
                         }
FAIL
FAIL    k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/conversion/unstructured    0.047s
make: *** [test] Error 1

make test WHAT=./vendor/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured

Running tests for APIVersion: v1,admissionregistration.k8s.io/v1alpha1,admission.k8s.io/v1alpha1,apps/v1beta1,apps/v1beta2,authentication.k8s.io/v1,authentication.k8s.io/v1beta1,authorization.k8s.io/v1,authorization.k8s.io/v1beta1,autoscaling/v1,autoscaling/v2alpha1,batch/v1,batch/v1beta1,batch/v2alpha1,certificates.k8s.io/v1beta1,extensions/v1beta1,imagepolicy.k8s.io/v1alpha1,networking.k8s.io/v1,policy/v1beta1,rbac.authorization.k8s.io/v1,rbac.authorization.k8s.io/v1beta1,rbac.authorization.k8s.io/v1alpha1,scheduling.k8s.io/v1alpha1,settings.k8s.io/v1alpha1,storage.k8s.io/v1beta1,storage.k8s.io/v1,,federation/v1beta1
+++ [0829 16:40:38] Running tests without code coverage
--- FAIL: TestConversionRoundtrip (0.00s)
    unstructured_test.go:111: FromUnstructured failed: Object 'Kind' is missing in '{"object":{"apiVersion":"v1","kind":"Foo","metadata":{"name":"foo1"}}}'
FAIL
FAIL    k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured  0.046s
make: *** [test] Error 1

Release note:

NONE

/kind bug
/sig api-machinery

k8s-ci-robot · 2017-08-29T07:53:14Z

Hi @ash2k. Thanks for your PR.

I'm waiting for a kubernetes 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.

I understand the commands that are listed here.

Details

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.

xiangpengzhao · 2017-08-29T08:18:48Z

/ok-to-test

ash2k · 2017-08-29T13:22:24Z

/retest

ash2k · 2017-08-31T07:04:37Z

/retest

ash2k · 2017-09-02T12:38:39Z

/retest

ash2k · 2017-09-05T08:44:13Z

/retest

sttts · 2017-09-08T07:07:37Z

staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured_test.go

can we move and unify both into staging/src/k8s.io/apimachinery/pkg/conversion/unstructured/testing/roundtrip.go?

Done. I moved the whole test file into a subdirectory - tests share type definitions (A, B, etc) so I couldn't move just the roundtrip test.

ash2k · 2017-09-08T08:50:52Z

staging/src/k8s.io/apimachinery/pkg/conversion/unstructured/BUILD

I ran hack/update-bazel.sh but it did not remove this go_test section even though there are no test files in this directory anymore. I removed it myself. Probably a bug in whatever is ran in update-bazel.sh script.

It seems to be broken.

/cc @mikedanese @frobware

sttts · 2017-09-08T09:00:38Z

staging/src/k8s.io/apimachinery/pkg/conversion/unstructured/test/converter_test.go

can you call the package testing instead? test makes trouble with go get.

sttts · 2017-09-08T09:00:54Z

staging/src/k8s.io/apimachinery/pkg/conversion/unstructured/test/converter_test.go

no underscore please. golint freaks out.

ash2k · 2017-09-08T09:49:36Z

Comments addressed.

sttts · 2017-09-08T11:56:34Z

/lgtm
/approve

k8s-github-robot · 2017-09-08T11:57:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ash2k, sttts

Associated issue: 47889

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these OWNERS Files:

~~staging/src/k8s.io/apimachinery/pkg/OWNERS~~ [sttts]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

ash2k · 2017-09-08T12:20:02Z

/retest

k8s-github-robot · 2017-09-08T13:13:49Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-09-08T14:09:12Z

Automatic merge from submit-queue

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 29, 2017

k8s-github-robot assigned timothysc and erictune Aug 29, 2017

k8s-github-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Aug 29, 2017

ash2k force-pushed the fix-unstructured-converter branch from 9f9249a to 624e4be Compare August 29, 2017 08:13

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 29, 2017

ash2k force-pushed the fix-unstructured-converter branch from 624e4be to e60be9a Compare August 30, 2017 01:48

lavalamp assigned mbohlool and caesarxuchao and unassigned timothysc and erictune Aug 31, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 3, 2017

ash2k force-pushed the fix-unstructured-converter branch from e60be9a to b8d3144 Compare September 3, 2017 09:36

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 3, 2017

ash2k mentioned this pull request Sep 6, 2017

Do deep copy instead of to and from JSON encoding #51526

Merged

ash2k force-pushed the fix-unstructured-converter branch from b8d3144 to 43334a8 Compare September 7, 2017 12:28

sttts reviewed Sep 8, 2017

View reviewed changes

ash2k force-pushed the fix-unstructured-converter branch 2 times, most recently from f1128ed to 3ec0b35 Compare September 8, 2017 08:47

ash2k commented Sep 8, 2017

View reviewed changes

sttts reviewed Sep 8, 2017

View reviewed changes

ash2k force-pushed the fix-unstructured-converter branch from 3ec0b35 to 5d2cde6 Compare September 8, 2017 09:48

Fix pointer receivers handling in unstructured converter

a672b98

ash2k force-pushed the fix-unstructured-converter branch from 5d2cde6 to a672b98 Compare September 8, 2017 11:29

k8s-ci-robot assigned sttts Sep 8, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 8, 2017

sttts added this to the v1.8 milestone Sep 8, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 8, 2017

k8s-github-robot merged commit a416534 into kubernetes:master Sep 8, 2017

ash2k deleted the fix-unstructured-converter branch September 8, 2017 21:42

Conversation

ash2k commented Aug 29, 2017

Uh oh!

k8s-ci-robot commented Aug 29, 2017

Uh oh!

xiangpengzhao commented Aug 29, 2017

Uh oh!

ash2k commented Aug 29, 2017

Uh oh!

ash2k commented Aug 31, 2017

Uh oh!

ash2k commented Sep 2, 2017

Uh oh!

ash2k commented Sep 5, 2017

Uh oh!

sttts Sep 8, 2017

Choose a reason for hiding this comment

Uh oh!

ash2k Sep 8, 2017

Choose a reason for hiding this comment

Uh oh!

ash2k Sep 8, 2017

Choose a reason for hiding this comment

Uh oh!

sttts Sep 8, 2017

Choose a reason for hiding this comment

Uh oh!

sttts Sep 8, 2017

Choose a reason for hiding this comment

Uh oh!

sttts Sep 8, 2017

Choose a reason for hiding this comment

Uh oh!

ash2k commented Sep 8, 2017

Uh oh!

sttts commented Sep 8, 2017

Uh oh!

k8s-github-robot commented Sep 8, 2017

Uh oh!

ash2k commented Sep 8, 2017

Uh oh!

k8s-github-robot commented Sep 8, 2017

Uh oh!

k8s-github-robot commented Sep 8, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants