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

Move Storage.New() -> Serializer.Defaulter().NewDefaultedObject() #36

Merged
merged 3 commits into from
Aug 21, 2020

Conversation

luxas
Copy link
Contributor

@luxas luxas commented Aug 21, 2020

In order to re-organize code where it fits better. Creating defaulted objects isn't really what the storage should be doing.

Notes: The old storage implementation relied heavily on objects always having their TypeMeta set.
Unfortunately, that is pretty brittle, and error prone, so in the moved code, we start progressively re-structuring code
so that it doesn't assume that, but instead use GVKForObject when needed.

cc @twelho @chanwit @stealthybox

@luxas luxas added this to the v0.0.3 milestone Aug 21, 2020
Copy link
Contributor

@twelho twelho left a comment

Choose a reason for hiding this comment

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

LGTM, great improvement 👍

@@ -13,6 +14,23 @@ type defaulter struct {
scheme *runtime.Scheme
}

// NewDefaultedObject returns a new, defaulted object. It is essentially scheme.New() and
// scheme.Default(obj), but with extra logic to cover also internal versions.
Copy link
Contributor

Choose a reason for hiding this comment

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

also cover internal versions

@@ -13,6 +14,23 @@ type defaulter struct {
scheme *runtime.Scheme
}

// NewDefaultedObject returns a new, defaulted object. It is essentially scheme.New() and
// scheme.Default(obj), but with extra logic to cover also internal versions.
// Important to note here is that the TypeMeta information is NOT applied automatically here.
Copy link
Contributor

Choose a reason for hiding this comment

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

Redundant here at the end of the sentence.

@luxas
Copy link
Contributor Author

luxas commented Aug 21, 2020

comments fixed. merging :)

@luxas luxas merged commit 4a0d190 into weaveworks:master Aug 21, 2020
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