output empty creationTimestamp as null #53464

juanvallejo · 2017-10-04T22:09:38Z

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 · 2017-10-04T22:09:46Z

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 · 2017-10-05T14:12:46Z

/assign fabianofranz
/assign shyiwang

k8s-ci-robot · 2017-10-05T14:12:47Z

@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 · 2017-10-05T18:51:32Z

/ok-to-test

juanvallejo · 2017-10-05T19:49:55Z

/test pull-kubernetes-e2e-gce-bazel

liggitt · 2017-10-07T05:36:26Z

Lgtm, would like to see a test

juanvallejo · 2017-10-09T15:23:44Z

@liggitt thanks, test added

liggitt · 2017-10-09T19:24:47Z

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

@@ -464,6 +464,10 @@ func (u *Unstructured) GetCreationTimestamp() metav1.Time {

 func (u *Unstructured) SetCreationTimestamp(timestamp metav1.Time) {
 	ts, _ := timestamp.MarshalQueryParameter()
+	if len(ts) == 0 {
+		u.setNestedField(nil, "metadata", "creationTimestamp")


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

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?

This field is omitempty.

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

Thanks, will update

juanvallejo · 2017-10-11T18:21:13Z

@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 · 2017-10-11T20:48:54Z

/retest

deads2k · 2017-11-02T15:03:52Z

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

@@ -258,6 +258,10 @@ func (u *Unstructured) GetCreationTimestamp() metav1.Time {

 func (u *Unstructured) SetCreationTimestamp(timestamp metav1.Time) {
 	ts, _ := timestamp.MarshalQueryParameter()
+	if len(ts) == 0 {
+		RemoveNestedField(u.Object, "metadata", "creationTimestamp")


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

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

@deads2k

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

Done

deads2k · 2017-11-02T17:16:38Z

lgtm, please squash for merge.

juanvallejo · 2017-11-02T18:08:41Z

@deads2k thanks, commits squashed

juanvallejo · 2017-11-03T13:54:41Z

/test pull-kubernetes-e2e-kops-aws

liggitt · 2017-11-03T16:49:09Z

/lgtm

fabianofranz · 2017-11-03T16:59:20Z

/approve no-issue

k8s-github-robot · 2017-11-03T16:59:34Z

[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 · 2017-11-03T17:10:34Z

/sig cli
/kind bug

k8s-github-robot · 2017-11-03T19:56:18Z

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

k8s-github-robot · 2017-11-03T19:59:12Z

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 · 2017-11-03T20:20:38Z

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

k8s-github-robot assigned erictune and wojtek-t Oct 4, 2017

k8s-ci-robot assigned fabianofranz Oct 5, 2017

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 Oct 5, 2017

fabianofranz requested a review from deads2k October 6, 2017 19:49

juanvallejo force-pushed the jvallejo/output-empty-creation-ts-as-null branch from d78cd09 to 465edb2 Compare October 9, 2017 15:23

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 9, 2017

liggitt reviewed Oct 9, 2017

View reviewed changes

juanvallejo mentioned this pull request Oct 11, 2017

UPSTREAM: 53464: use unstructured builder - oc export openshift/origin#16665

Merged

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 11, 2017

juanvallejo force-pushed the jvallejo/output-empty-creation-ts-as-null branch 2 times, most recently from d06e84c to 327353c Compare October 11, 2017 19:15

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

juanvallejo force-pushed the jvallejo/output-empty-creation-ts-as-null branch from 327353c to a358092 Compare October 12, 2017 13:55

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

juanvallejo force-pushed the jvallejo/output-empty-creation-ts-as-null branch from 2d4692e to 62a8e47 Compare November 1, 2017 17:20

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

juanvallejo force-pushed the jvallejo/output-empty-creation-ts-as-null branch from 62a8e47 to 288e21f Compare November 2, 2017 14:41

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 2, 2017

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

deads2k reviewed Nov 2, 2017

View reviewed changes

juanvallejo force-pushed the jvallejo/output-empty-creation-ts-as-null branch 2 times, most recently from 7d34131 to e7ca207 Compare November 2, 2017 15:53

remove empty creationTimestamp field

2008750

juanvallejo force-pushed the jvallejo/output-empty-creation-ts-as-null branch from e7ca207 to 2008750 Compare November 2, 2017 18:08

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

k8s-ci-robot added sig/cli Categorizes an issue or PR as relevant to SIG CLI. kind/bug Categorizes issue or PR as related to a bug. labels Nov 3, 2017

fabianofranz added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 3, 2017

k8s-github-robot merged commit 1ad792f into kubernetes:master Nov 3, 2017

juanvallejo deleted the jvallejo/output-empty-creation-ts-as-null branch November 3, 2017 21:07

liuhuiping2013 mentioned this pull request Sep 14, 2018

configMapGenerator please do not generate creationTimestamp kubernetes-sigs/kustomize#344

Closed

karlkfi mentioned this pull request Apr 6, 2021

Marshal writes CreationTimestamp: null kubernetes/apimachinery#119

Closed

jpbetz mentioned this pull request Dec 2, 2022

creationTimestamp: null is invalid since it isn't a string #109427

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

output empty creationTimestamp as null #53464

output empty creationTimestamp as null #53464

juanvallejo commented Oct 4, 2017 •

edited

Loading

k8s-ci-robot commented Oct 4, 2017

juanvallejo commented Oct 5, 2017

k8s-ci-robot commented Oct 5, 2017

cblecker commented Oct 5, 2017

juanvallejo commented Oct 5, 2017

liggitt commented Oct 7, 2017

juanvallejo commented Oct 9, 2017

liggitt Oct 9, 2017

ironcladlou Oct 11, 2017

deads2k Nov 1, 2017

juanvallejo Nov 1, 2017

juanvallejo commented Oct 11, 2017

juanvallejo commented Oct 11, 2017

deads2k Nov 2, 2017 •

edited

Loading

liggitt Nov 2, 2017 •

edited

Loading

juanvallejo Nov 2, 2017

deads2k commented Nov 2, 2017

juanvallejo commented Nov 2, 2017

juanvallejo commented Nov 3, 2017

liggitt commented Nov 3, 2017

fabianofranz commented Nov 3, 2017

k8s-github-robot commented Nov 3, 2017

fabianofranz commented Nov 3, 2017

k8s-github-robot commented Nov 3, 2017

k8s-github-robot commented Nov 3, 2017

k8s-ci-robot commented Nov 3, 2017 •

edited

Loading

output empty creationTimestamp as null #53464

output empty creationTimestamp as null #53464

Conversation

juanvallejo commented Oct 4, 2017 • edited Loading

k8s-ci-robot commented Oct 4, 2017

juanvallejo commented Oct 5, 2017

k8s-ci-robot commented Oct 5, 2017

cblecker commented Oct 5, 2017

juanvallejo commented Oct 5, 2017

liggitt commented Oct 7, 2017

juanvallejo commented Oct 9, 2017

liggitt Oct 9, 2017

Choose a reason for hiding this comment

ironcladlou Oct 11, 2017

Choose a reason for hiding this comment

deads2k Nov 1, 2017

Choose a reason for hiding this comment

juanvallejo Nov 1, 2017

Choose a reason for hiding this comment

juanvallejo commented Oct 11, 2017

juanvallejo commented Oct 11, 2017

deads2k Nov 2, 2017 • edited Loading

Choose a reason for hiding this comment

liggitt Nov 2, 2017 • edited Loading

Choose a reason for hiding this comment

juanvallejo Nov 2, 2017

Choose a reason for hiding this comment

deads2k commented Nov 2, 2017

juanvallejo commented Nov 2, 2017

juanvallejo commented Nov 3, 2017

liggitt commented Nov 3, 2017

fabianofranz commented Nov 3, 2017

k8s-github-robot commented Nov 3, 2017

fabianofranz commented Nov 3, 2017

k8s-github-robot commented Nov 3, 2017

k8s-github-robot commented Nov 3, 2017

k8s-ci-robot commented Nov 3, 2017 • edited Loading

juanvallejo commented Oct 4, 2017 •

edited

Loading

deads2k Nov 2, 2017 •

edited

Loading

liggitt Nov 2, 2017 •

edited

Loading

k8s-ci-robot commented Nov 3, 2017 •

edited

Loading