-
Notifications
You must be signed in to change notification settings - Fork 9
Add MCAD API groups migration plan #7
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
Add MCAD API groups migration plan #7
Conversation
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.
Overall LGTM. Left some questions
If we are going to force every downstream project/user to take action, it may be worth considering making other/all desirable changes to the CRD structure at the same time, so that we don't have to repeat the disruption again and again. Maybe we need first separate discussion/ADR about these changes and then decide whether to combine these efforts. |
@tardieu that's a good point. It's worth noting that those "structural" changes, within the CRDs, could be handled with the usual CRD versioning mechanism, possibly using conversion webhooks (or CEL based conversion in the future) to automate it, as opposed to changes to identifying fields, like the API group. That being said, I agree it'd be worth assessing if any major breaking changes could be piggy-backed with that effort. |
Thanks, @astefanutti and @tardieu - "structural changes" as mentioned are bound to happen in the future. For the features that are required for the immediate release, I don't think there is anything major that needs to be added to the effort. |
I think is OK to try to piggy back everything if all the items are known at this point in time. From my perspective (KubeRay API server integration) the sooner the final API is available the better. The only items that I can think that would fall into
|
For the TorchX+MCAD piece, I plan to try to support both the old version and the new version to help with the transition for users on different systems. Is there a clear way to tell which CRDs are in use that does not require administrative type permissions? |
One possible solution would be to query the discovery API, and check if the new API group is present. Generally most users are granted permission to query the discovery API, which is what's used when running the |
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, but I would wait for the others to chime in before considering the ADR approved. @tardieu needs to sign off on this ADR.
+1 for one-shot. |
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.
Approved from my perspective.
Approved. |
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, makes logical sense.
I think we have quorum for the approvals. The work has already somewhat begun -- I'm going to go ahead and merge this. If in future we require additional revisions, we can always propose a new ADR / add an addendum to this one. |
Closes project-codeflare/multi-cluster-app-dispatcher#352.