-
Notifications
You must be signed in to change notification settings - Fork 420
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
⚠ Generate Embedded ObjectMeta in the CRDs #539
Conversation
Welcome @dvaldivia! |
/assign @DirectXMan12 |
I updated the PR description |
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.
really minor comment as per the meeting today, otherwise looks good
if f, ok := KnownPackages["k8s.io/apimachinery/pkg/apis/meta/v1"]; ok { | ||
f(p, pkg) | ||
} | ||
// This is a allow-listed set of properties of ObjectMeta, other runtime properties are not part of this list |
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.
as per the meeting today, please link to sttts's comment here
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 clarify, I think @DirectXMan12 is referring to this comment #395 (comment). Can you confirm?
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.
yeah, that's what I was talking about.
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.
@DirectXMan12 @coderanger I'll update the 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.
done, rebased, squashed and comment added.
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dvaldivia The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
1228456
to
3beb2ea
Compare
@dvaldivia: PR needs rebase. 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. |
1 similar comment
@dvaldivia: PR needs rebase. 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. |
subsumed by #557 (with attribution :-) ) |
This PR makes it so embedded
ObjectMeta
in the CRD get's properly generated if the generator optiongenerateEmbeddedObjectMeta=true
is passed, this is needed because if a CRD has embeddedObjectMeta
in any field andpreserveUnknowFields
is set to false, all the metadata will be lost when doing conversion between versions.An example on how to have the embedded ObjectMeta generated in the resulting CRD
This PR makes it so by default any embedded
ObjectMeta
is not generated in the resulting CRD, however the top levelObjectMeta
belonging to the CRD itself is never generated as the kubernetes API disallows changes to the CRD metadata between conversions.The generated
ObjectMeta
is also only a subset of the original set of fields insideObjectMeta
this is due to the fact that other runtime fields are problematic if they are being traded with the kubernetes API, such ascreationTimeStamp
(rancher/rancher#23857) so this only generatesname, namespace, labels, annotations and finalizers
which from a design perspective should be enough. This follows the recommendation by @sttts who recommended aEmbeddedObjectMeta
but instead of using a different type, we have a reduced set of fields that are "just enough" for a CRD design.An example of why we need this is for example if a CRD had a
volumeClaimTemplate
(for an underlying statefulset) which includeObjectMeta
such as Labels, Annotations and/org name which are meant to be passed to the PVC where getting lost between conversions.This PR is based on the work by @arjunrn and @champak following the discussion on #448 and the PR #498 and it's also similar to #395 as it addressed the same problem, but adds the type casting for
FieldsV1
and the requested argument to control this feature (per @DirectXMan12 on #395)