-
Notifications
You must be signed in to change notification settings - Fork 897
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
feat: support conversions in XRDs #3940
Conversation
Signed-off-by: Philippe Scorsolini <p.scorsolini@gmail.com>
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.
Two non-blocking comments, but otherwise this LGTM.
Do we want to back port it to v1.11.x though? I would argue no - this feels like a new feature rather than a bug fix.
|
||
// Conversion defines all conversion settings for the defined Composite resource. | ||
// +optional | ||
Conversion *extv1.CustomResourceConversion `json:"conversion,omitempty"` |
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.
Should we use the extv1
type here, or define our own?
I'm thinking we may want to support strategies that upstream doesn't support in future, for example some kind of declarative conversion. I suppose we could always switch from upstream's type to our own at that point.
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.
Yes, I also had the same doubt, in the end I chose extv1
because I thought it was better to effortlessly support everything they might introduce upstream, and as you said, we can always switch to our own later on if needed. But I'm ok switching to our own if you feel we want to have more control.
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.
we can always switch to our own later on if needed.
@phisco can you validate that this is possible without changing (i.e. breaking) the API? If possible, we can go with extv1
for now and consider this later.
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.
On the CRD side there won't be changes, except the desired ones, nor aliasing the type to add some methods, nor embedding with json:",inline"
to add additional fields, nor completely duplicating the struct.
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.
Do we want to back port it to v1.11.x though? I would argue no - this feels like a new feature rather than a bug fix.
Agreed, I would also like to let this bake in main branch for a bit and then be initially released in v1.12 as opposed to being backported for a patch release.
Co-authored-by: Nic Cope <nicc@rk0n.org> Signed-off-by: Philippe Scorsolini <p.scorsolini@gmail.com>
Description of your changes
Fixes #2608.
Adds the ability to configure the Conversion strategy of the resulting CRD, allowing users to define multiple versions with different schemas.
I have:
make reviewable
to ensure this PR is ready for review.backport release-x.y
labels to auto-backport this PR if necessary.How has this code been tested
Results in the following CRD: