-
Notifications
You must be signed in to change notification settings - Fork 268
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
Extensions: Add hooks to support virtual clusters #11064
Conversation
3225ac2
to
82fc100
Compare
@mantis-toboggan-md finally got this to pass gates - refactored as we discussed. |
82fc100
to
37c7477
Compare
71c0394
to
29ec9b5
Compare
29ec9b5
to
06756d1
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.
Reviewing this, changes seem pretty self self-contained as it's targeting the provisioning.cattle.io.cluster
. I assume that since this is experimental, we don't want to document it right now, but in the future, if we open it, we need to add it to the documentation.
One last remark would be around using modelExtensions
instead of extending/reusing uiConfig
🤔
nevertheless, I would say that it LGTM
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.
Haven't given this a functional test, just looked at code.
Only minor comments. We are limiting model extensions to be singular though. Not sure we'll ever be in a scenario where different extensions might want to update things like deployment actions?
8432344
to
1f0919e
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 - I tested this by running code against the virtual clusters ui poc
* Add hooks to support virtual clusters * Fix lint issues * Refinements * Update for Vue 3 changes * Fix import * Minor tweaks * Fix bug causing e2e tests to fail * Fix lint issue * Rename internal properties and ensure they don't break clone/save * Ensure we generate types for the plugins package to give us access to mapDriver * Simpler approach * Fix lint issues and add type * Remove unused code * Revery unnecessary changes * Bug fix for finding model extension * Factor out string constant and add provider display method * Add experimental to API * Update typegen.sh to use SHELL_DIR var * Move type def * Fix lint issue * Address PR feedback * Update steve-class.js so we always get an array * Fix type definition
Summary
Fixes #11065
This PR adds several hooks for virtual cluster support.
The main change is a new extension API
addModelExtension
- this is marked experimental for now - once we establish if we need any changes, we can remove that label.You can register model extensions and they are stored against a model type, e.g.
provisioning.cattle.io.cluster
. There is a method on the Steve base class to retrieve all of the model extensions for a given model type. You can register multiple model extensions for a given model.Beyond that, it is up to a given model to decide how to use these model extensions - i.e. a model needs to define an API for them and explicitly wire that into itself. This ensures we have a defined API for extensibility, rather than allowing a model extension to do anything it wants.
In this PR, we write a model extension into the provisioning cluster model to support the needs of virtual clusters.
Risk wise, this PR is designed to minimise impact to existing code, so the risk is small.
Areas or cases that should be tested
These hooks will be tested by extensions.
Current e2e test should validate that the hooks do not break existing functionality.
Checklist