output empty creationTimestamp as null

[juanvallejo]

Release note

NONE

Updates the value of the creationTimestamp field to be null
when empty, to keep parity between it and deletionTimestamp.

Adds a round-trip test to ensure that unstructured objects containing
empty metadata fields are able to be re-converted back into internal
or external objects. Prior to the proposed patch in this PR, an
unstructured object whose .metadata.creationTimestamp value had
been set through the metadata accessor to an empty value
(metav1.Time{} in this case), was unable to be re-converted to an
internal or external type using the runtime decoder. Conversion would
fail with the error:

unstructured_test.go:177: FromUnstructured failed: parsing time "" as "2006-01-02T15:04:05Z07:00": cannot parse "" as "2006"

cc @liggitt @fabianofranz

[k8s-ci-robot]

Hi @juanvallejo. 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.

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.

[juanvallejo]

/assign fabianofranz
/assign shyiwang

[k8s-ci-robot]

@juanvallejo: GitHub didn't allow me to assign the following users: shyiwang.

Note that only kubernetes members can be assigned.

In response to this:

/assign fabianofranz
/assign shyiwang

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.

[cblecker]

/ok-to-test

[juanvallejo]

/test pull-kubernetes-e2e-gce-bazel

[liggitt]

Lgtm, would like to see a test

[juanvallejo]

@liggitt thanks, test added

[liggitt]

hmm... actually, should this be storing nil or removing from the map? @ironcladlou, any thoughts here?

[ironcladlou]

I suggested removing nil keys from the map generally in #48211, but I wouldn't say there's consensus on that point yet.

@deads2k suggested we rely on omitempty in the external types to accommodate a nil here (mirroring his prior counterpoint to #48211 (comment)), which I'm fine with.

Maybe some more godocs would help here in the meantime?

[deads2k]

This field is omitempty.

@juanvallejo I prefer to delete the entry, then this lgtm.

[juanvallejo]

Thanks, will update

[juanvallejo]

@ironcladlou thanks for your feedback on this; per our conversation I added a round trip test
that ensures empty metadata field values in an object do not inhibit that object's conversion.
Updated PR's description to explain the new test, and did a local test proving the fix from
this patch.

From the PR's description:

Adds a round-trip test to ensure that unstructured objects containing
empty metadata fields are able to be re-converted back into internal
or external objects. Prior to the proposed patch in this PR, an
unstructured object whose `.metadata.creationTimestamp` value had
been set through the metadata accessor to an empty value 
(`metav1.Time{}` in this case), was unable to be re-converted to an
internal or external type using the runtime decoder. Conversion would
fail with the error:

unstructured_test.go:177: FromUnstructured failed: parsing time "" as "2006-01-02T15:04:05Z07:00": cannot parse "" as "2006"

[juanvallejo]

/retest

[deads2k]

@sttts creationtime is an omitEmpty field, so when it is zero value it doesn't get serialized. While I may not detect it this way (@juanvallejo can you check if the passed timestamp is zero value), removing the field when this happens is reasonable.

Also, the current serialization is incorrect. We should never produce one with an empty string.

[liggitt]

creationtime is an omitEmpty field, so when it is zero value it doesn't get serialized

actually, since it's a struct in ObjectMeta, it still does (yay, go json), but logically, should not

[juanvallejo]

@deads2k

@juanvallejo can you check if the passed timestamp is zero value

Done

[deads2k]

lgtm, please squash for merge.

[juanvallejo]

@deads2k thanks, commits squashed

[juanvallejo]

/test pull-kubernetes-e2e-kops-aws

[liggitt]

/lgtm

[fabianofranz]

/approve no-issue

[k8s-github-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: fabianofranz, juanvallejo, liggitt

No associated issue. Update pull-request body to add a reference to an issue, or get approval with /approve no-issue

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

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

• pkg/api/OWNERS [liggitt]
• staging/src/k8s.io/apimachinery/pkg/OWNERS [liggitt]

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

[fabianofranz]

/sig cli
/kind bug

[k8s-github-robot]

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

[k8s-github-robot]

Automatic merge from submit-queue (batch tested with PRs 55050, 53464, 54936, 55028, 54928). If you want to cherry-pick this change to another branch, please follow the instructions here.

[k8s-ci-robot]

@juanvallejo: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-unit	2008750	link	/test pull-kubernetes-unit

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.