Extensions: Add hooks to support virtual clusters #11064
Conversation
nwmac
changed the title
nwmac
force-pushed
the
add-virtual-cluster-hooks
branch
3 times, most recently
from
3225ac2 to
82fc100
Compare
|
@mantis-toboggan-md finally got this to pass gates - refactored as we discussed. |
nwmac
force-pushed
the
add-virtual-cluster-hooks
branch
from
82fc100 to
37c7477
Compare
nwmac
force-pushed
the
add-virtual-cluster-hooks
branch
from
71c0394 to
29ec9b5
Compare
nwmac
force-pushed
the
add-virtual-cluster-hooks
branch
from
29ec9b5 to
06756d1
Compare
There was a problem hiding this comment.
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
| * @param cluster The cluster model (`provisioning.cattle.io.cluster`) | ||
| * @returns Whether to use this provisioner for the given cluster. | ||
| */ | ||
| useForModel(cluster: any): boolean; |
There was a problem hiding this comment.
this isn't wired in anywhere
| * @param type Model type | ||
| * @param clz Class for the model extension (constructor) | ||
| */ | ||
| addModelExtension(type: string, clz: Function): void; |
There was a problem hiding this comment.
given clz is a known function .. here (and shell/core/plugin.ts) should be typed
| @@ -88,6 +88,11 @@ export default { | |||
| }, | |||
|
|
|||
| computed: { | |||
| groupable() { | |||
There was a problem hiding this comment.
this is going to break with Cody's SSP PR. We won't have access to all provisioning clusters (filteredRows) just a single pages worth.
| */ | ||
| get customProvisionerHelper() { | ||
| // Find the first model extension that says it can be used for this model | ||
| return this.modelExtensions.find((modelExt) => modelExt.useFor ? modelExt.useFor(this) : false); |
There was a problem hiding this comment.
should there be a type / interface for modelExtension instances? missing this fn will trip up developers
| return this.customProvisionerHelper?.parentCluster?.(this); | ||
| } | ||
|
|
||
| get groupByLabel() { |
There was a problem hiding this comment.
Think this is fine now, so just a comment, but in the future the plan is to group clusters by cluster class as well
| } | ||
|
|
||
| get groupByParent() { | ||
| // Customer helper can report if the cluster has a parent cluster |
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