-
Notifications
You must be signed in to change notification settings - Fork 983
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Decoupled Constraints API object from internal representation #1603
Conversation
✅ Deploy Preview for karpenter-docs-prod canceled.
|
17b953f
to
af39618
Compare
aws := &AWS{} | ||
_, gvk, err := Codec.UniversalDeserializer().Decode(constraints.Provider.Raw, nil, aws) | ||
p := &Provider{} | ||
_, gvk, err := Codec.UniversalDeserializer().Decode(provider.Raw, nil, p) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this wasn't changed, but it looked weird and from my reading of the docs we are safer to use the return value as opposed to assuming it populates p:
// If into is non-nil, it will be used as the target type
// and implementations may choose to use it rather than reallocating an object. However, the object is not
// guaranteed to be populated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To derisk, we can we cover this in another PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM
0583d99
to
a7acc52
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
1. Issue, if available:
#1350
2. Description of changes:
This is the first change in a series of changes to decouple the v1alpha5 API package from our internal node representation.
3. How was this change tested?
4. Does this change impact docs?
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.