Skip to content
This repository has been archived by the owner on Dec 7, 2023. It is now read-only.

Fix meta.ObjectMeta.Created to always be present and update itself automatically #174

Merged
merged 4 commits into from
Jul 15, 2019

Conversation

twelho
Copy link
Contributor

@twelho twelho commented Jul 15, 2019

This changes meta.ObjectMeta.Created from being a pointer with omitempty to a required Time struct instance. Time now embeds a pointer to metav1.Time to tell if it's set and will set itself automatically upon marshaling if unset.

If we don't need any metav1.Time specific functionality directly, we should probably move from embedding to have a private field, so any metav1.Time methods cannot be accidentally called with a nil receiver. What do you think @luxas ?

@twelho twelho changed the title FIx meta.ObjectMeta.Created to always be present and update itself automatically Fix meta.ObjectMeta.Created to always be present and update itself automatically Jul 15, 2019
Copy link
Contributor

@luxas luxas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

I think we can reuse most stuff from k8s apimachinery here and avoid the embedded pointer.
I'm fine with embedding still, but without the pointer

@@ -9,21 +10,67 @@ import (
)

type Time struct {
metav1.Time
*metav1.Time
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we actually need this?
There is a metav1.Time.IsZero() method we could use instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know about metav1.Time.IsZero(), will definitely use that instead of a pointer 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 5b9bb3f.

ti = Timestamp()
}

b, err := ti.MarshalText()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this marshals the value without " on the sides. I don't think that's a good idea with YAML.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead, re-use metav1.Time's MarshalJSON func here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely, fixed in 5b9bb3f.

return json.Marshal(string(b))
}

func (t *Time) UnmarshalJSON(b []byte) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this function, only call the underlying metav1.Time Unmarshal func. If the input is no content, give metav1.Time.Unmarshal a byte array of null

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the input is no content, the byte array is already empty (nil). We don't even need the custom unmarshaler to overload the underlying metav1.Time.UnmarshalJSON. Fixed in 5b9bb3f.

Copy link
Contributor

@luxas luxas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks 👍 !

@luxas luxas merged commit 4e1b65d into master Jul 15, 2019
@luxas luxas deleted the created-fix branch July 15, 2019 16:30
@twelho twelho added this to the v0.4.2 milestone Jul 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants