Fix unstructured metadata accessors to respect omitempty semantics by nikhita · Pull Request #67635 · kubernetes/kubernetes

nikhita · 2018-08-21T06:43:56Z

Fixes #67541
Fixes #48211
Fixes #49075
Follow up of #67562

ObjectMeta has fields with omitempty json tags. This means that when the fields have zero values, they should not be persisted in the object.

Before this PR, some of the metadata accessors for unstructured objects did not respect these semantics i.e they would persist a field even if it had a zero value.

This PR updates the accessors so that the field is removed from the unstructured object map if it contains a zero value.

/sig api-machinery
/kind bug
/area custom-resources
/cc sttts liggitt yue9944882 roycaihw
/assign sttts liggitt

Release note:

NONE

nikhita · 2018-08-21T06:52:17Z

Does this need a "action required" release note?

nikhita · 2018-08-21T07:45:03Z

Some packages in hack/.golint_failures are passing golint. Please remove them.

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

How is this even passing golint? There are so many linting errors in there. 🤷‍♀️

/lint

k8s-ci-robot

@nikhita: 2 warnings.

Details

In response to this:

Some packages in hack/.golint_failures are passing golint. Please remove them.

 staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured
How is this even passing golint? There are so many linting errors in there. 🤷‍♀️

/lint

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.

k8s-ci-robot · 2018-08-21T07:45:40Z

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

Golint naming: receiver name u should be consistent with previous receiver name obj for Unstructured. More info.

k8s-ci-robot · 2018-08-21T07:45:40Z

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

Golint naming: receiver name u should be consistent with previous receiver name obj for Unstructured. More info.

well that's annoying

nikhita · 2018-08-21T07:46:09Z

@cblecker @fejta re: golint and /lint above ^^

ObjectMeta has fields with omitempty json tags. This means that when the fields have zero values, they should not be persisted in the object. Before this commit, some of the metadata accessors for unstructured objects did not respect these semantics i.e they would persist a field even if it had a zero value. This commit updates the accessors so that the field is removed from the unstructured object map if it contains a zero value.

sttts · 2018-08-21T09:08:20Z

Looks good to me. @liggitt any further comments?

#67637 will add tests for the empty patch behaviour.

nikhita · 2018-08-21T13:22:26Z

/lint

k8s-ci-robot

@nikhita: 2 unresolved warnings and 6 new warnings.

Details

In response to this:

/lint

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.

k8s-ci-robot · 2018-08-21T13:23:01Z

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

Golint comments: exported method Unstructured.EachListItem should have comment or be unexported. More info.

None of the methods in that file have comments and it doesn't really make sense to add to each one either. :/

k8s-ci-robot · 2018-08-21T13:23:01Z

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

Golint comments: exported method Unstructured.UnstructuredContent should have comment or be unexported. More info.

k8s-ci-robot · 2018-08-21T13:23:01Z

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

Golint comments: exported method Unstructured.SetUnstructuredContent should have comment or be unexported. More info.

k8s-ci-robot · 2018-08-21T13:23:01Z

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

Golint comments: exported method Unstructured.GetObjectKind should have comment or be unexported. More info.

k8s-ci-robot · 2018-08-21T13:23:01Z

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

Golint comments: exported method Unstructured.IsList should have comment or be unexported. More info.

k8s-ci-robot · 2018-08-21T13:23:01Z

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

Golint comments: exported method Unstructured.ToList should have comment or be unexported. More info.

liggitt · 2018-08-21T13:25:34Z

don't feed the linter... it'll just want another cookie :)

nikhita · 2018-08-21T13:28:18Z

don't feed the linter... it'll just want another cookie :)

:)

I removed the last commit which tried to fix the golint errors. It's back to how it was before in the previous PR.

nikhita · 2018-08-21T14:58:04Z

Regarding the linting:

kubernetes/hack/verify-golint.sh

Lines 75 to 76 in 816f2a4

    
           # Packages with a corresponding foo_test package will make golint fail 
        
           # with a useless error. Just ignore that, see golang/lint#68.

Looks like this completely ignores all errors if it sees a foo_test package.
Related issue in golint: golang/lint#68

nikhita · 2018-08-21T15:54:35Z

If golint sees a foo_test package in foo, it gives an error like:
staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured/unstructured_test.go is in package unstructured_test, not unstructured and does not do any linting. We swallow this error and skip the linting.

kubernetes/hack/verify-golint.sh

Line 77 in 816f2a4

failedLint=$(golint "$p"/*.go 2>/dev/null)

liggitt · 2018-08-21T18:35:29Z

this LGTM. @sttts, was there a way we were collecting dev-related release notes for client-go/apimachinery usage? I'm not sure this change makes sense as a user-facing release note, but seems worthwhile to capture for users of the go libraries

liggitt · 2018-08-21T18:36:19Z

/lgtm

k8s-ci-robot · 2018-08-21T18:36:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, nikhita

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~hack/OWNERS~~ [liggitt]
~~pkg/kubectl/OWNERS~~ [liggitt]
~~staging/src/k8s.io/apimachinery/pkg/OWNERS~~ [liggitt]
~~staging/src/k8s.io/apiserver/OWNERS~~ [liggitt]

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

nikhita · 2018-08-21T18:39:53Z

was there a way we were collecting dev-related release notes for client-go/apimachinery usage?

manually. :/
ref:

I was doing the manual release note process work for client-go anyway, so I'll take care of the release note for this one.

nikhita · 2018-08-21T20:45:09Z

Quick update here: there is a golint fix now - #67675 🎉

roycaihw · 2018-08-21T21:30:56Z

This LGTM. One question: do we need to check zero values for TypeMeta (SetKind and SetAPIVersion) as well?

liggitt · 2018-08-21T21:43:41Z

One question: do we need to check zero values for TypeMeta (SetKind and SetAPIVersion) as well?

No valid serialized object has those set to empty values, so I don't think so

k8s-github-robot · 2018-08-22T01:33:06Z

Automatic merge from submit-queue (batch tested with PRs 67298, 67518, 67635, 67673). If you want to cherry-pick this change to another branch, please follow the instructions here.

sttts · 2018-08-22T06:53:13Z

was there a way we were collecting dev-related release notes for client-go/apimachinery usage

@liggitt we don't have prow support. But we usually use dev-release-notes

k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Aug 21, 2018

k8s-ci-robot assigned liggitt and sttts Aug 21, 2018

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. kind/bug Categorizes issue or PR as related to a bug. labels Aug 21, 2018

k8s-ci-robot requested review from liggitt, roycaihw, sttts and yue9944882 August 21, 2018 06:43

k8s-ci-robot added area/custom-resources cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. area/apiserver labels Aug 21, 2018

nikhita mentioned this pull request Aug 21, 2018

Prevent resourceVersion updates for custom resources on no-op writes #67562

Merged

k8s-ci-robot reviewed Aug 21, 2018

View reviewed changes

nikhita added 2 commits August 21, 2018 13:17

Fix tests to support ObjectMeta omitempty semantics

dabd56f

nikhita force-pushed the customresource-subresource-patch-04 branch from 039005a to dabd56f Compare August 21, 2018 07:47

sttts mentioned this pull request Aug 21, 2018

apiextensions: improve patch integration test #67637

Closed

k8s-ci-robot reviewed Aug 21, 2018

View reviewed changes

nikhita force-pushed the customresource-subresource-patch-04 branch from 0eb1f27 to dabd56f Compare August 21, 2018 13:27

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 21, 2018

nikhita mentioned this pull request Aug 21, 2018

Pass files to golint one at a time #67675

Merged

k8s-github-robot merged commit ce8a628 into kubernetes:master Aug 22, 2018

nikhita deleted the customresource-subresource-patch-04 branch August 22, 2018 08:10

nikhita mentioned this pull request Aug 22, 2018

Automated cherry pick of #67562: Prevent resourceVersion updates for custom resources on no-op writes #67616

Merged

nikhita mentioned this pull request Oct 23, 2018

Update metadata.generation behaviour for custom resources kubernetes/website#10705

Merged

Conversation

nikhita commented Aug 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikhita commented Aug 21, 2018

Uh oh!

nikhita commented Aug 21, 2018

Uh oh!

k8s-ci-robot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikhita commented Aug 21, 2018

Uh oh!

sttts commented Aug 21, 2018

Uh oh!

nikhita commented Aug 21, 2018

Uh oh!

k8s-ci-robot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liggitt commented Aug 21, 2018

Uh oh!

nikhita commented Aug 21, 2018

Uh oh!

nikhita commented Aug 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikhita commented Aug 21, 2018

Uh oh!

liggitt commented Aug 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liggitt commented Aug 21, 2018

Uh oh!

k8s-ci-robot commented Aug 21, 2018

Uh oh!

nikhita commented Aug 21, 2018

Uh oh!

nikhita commented Aug 21, 2018

Uh oh!

roycaihw commented Aug 21, 2018

Uh oh!

liggitt commented Aug 21, 2018

Uh oh!

k8s-github-robot commented Aug 22, 2018

Uh oh!

sttts commented Aug 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

nikhita commented Aug 21, 2018 •

edited

Loading

nikhita commented Aug 21, 2018 •

edited

Loading

liggitt commented Aug 21, 2018 •

edited

Loading