Skip to content
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

feature: Do not require APIConversion to exist for APIResourceSchema if no conversion required #2696

Open
1 task done
hasheddan opened this issue Jan 27, 2023 · 5 comments
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature.

Comments

@hasheddan
Copy link
Member

Feature Description

Currently, if an APIResourceSchema supports multiple versions and there is no corresponding APIConversion, creating an APIBinding for an APIExport that includes the APIResourceSchema fails. In short, an APIConversion is required for any APIResourceSchema with multiple versions, even if the APIConversion contains no conversion information (i.e. empty spec).

Proposed Solution

If no conversion information is required to move from one version to another, for example when the versions are identical or dropping fields is acceptable, it would be convenient to assume that the lack of an APIConversion is equivalent to the presence of an empty APIConversion.

Alternative Solutions

The alternative would be to keep the current system, or have the system generate the no-op APIConversion in the cluster and manage its lifecycle. However, in the latter case it would be unclear as to whether the user is planning to add an APIConversion but just created the APIResourceSchema first.

Want to contribute?

  • I would like to work on this issue.

Additional Context

No response

@hasheddan hasheddan added the kind/feature Categorizes issue or PR as related to a new feature. label Jan 27, 2023
@hasheddan
Copy link
Member Author

/assign

@ncdc
Copy link
Member

ncdc commented Jan 27, 2023

Thanks for filing! We do need to add a spec field on APIRS to indicate to kcp that the developer intends to supply an APIConversion. We need to know definitively if an APIConversion is expected, or not.

@hasheddan
Copy link
Member Author

@ncdc ack -- adding a spec.conversion field to match CRDs seems like it would make sense to me. However, since we have APIConversion type and it is associated via name, I imagine that our spec.conversion enum would be something like None or APIConversion? We would opt for just a bool instead, but if so I think we would need to commit to any conversion types being performed via the defined APIConversion. For example, today CEL transformations are supported, but if webhooks were also supported in the future, they would be added to APIConversion, rather than APIResourceSchema.

I would advocate for the latter strategy of using a bool and expanding the APIConversion type as needed (i.e. if we have an API dedicated to conversion, that is where all conversion strategies should be defined). Thoughts?

@ncdc
Copy link
Member

ncdc commented Jan 30, 2023

I think @sttts suggested in Slack to have an enum for None and External, with the latter meaning APIConversion. The enum allows more future flexibility (if we ever need it) compared to a bool. I think either approach would work, and it's ok if we get it "wrong" and need to change it later.

@hasheddan
Copy link
Member Author

@ncdc None and External feels reasonable to me. I do think we should be pretty stringent about separation of concerns between the two types, but that is somewhat orthogonal to preserving flexibility in case we need it.

Given the original set of steps that led to this issue being opened (and mirroring CRD behavior), I would think None should be our default here, but using External as default would be closer to current behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature.
Projects
Status: Backlog
Development

No branches or pull requests

2 participants