-
Notifications
You must be signed in to change notification settings - Fork 665
CONSOLE-4806: Remove static extensions from knative-plugin #15641
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
base: main
Are you sure you want to change the base?
CONSOLE-4806: Remove static extensions from knative-plugin #15641
Conversation
…ate console-extensions.json to include new topology data model factories for serving, eventing, and kamelets, and remove deprecated topology-plugin.ts file
|
@Leo6Leo: This pull request references CONSOLE-4806 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
| "resources": { | ||
| "$codeRef": "getKnativeResources.getKnativeServingResources" | ||
| }, |
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.
resources expects a WatchK8sResourcesGeneric and not a codeRef
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.
also since getKnativeServingResources always returns the same object we can embed the contents of that directly into the json file--no codeRef needed
| "resources": { | ||
| "$codeRef": "getKnativeResources.getKnativeEventingResources" | ||
| }, |
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.
resources expects a WatchK8sResourcesGeneric and not a codeRef.
The challenge of this ticket is that getKnativeEventingResources is dynamic, so we need to modify console.topology/data/factory to also allow codeRefs.
We discussed in the dynamic plugin sync that this coderef should be typed with
resources: WatchK8sResourcesGeneric | CodeRef<() => Promise<WatchK8sResourcesGeneric>>.
The promise is needed as plugins may want to do some fetching in order to determine which k8s resources should be wtached.
This will require changes to any code which consumes the TopologyDataModelFactory extension. I suggest possibly writing a useTopologyDataModelFactory hook in the topology package to allow resolve the codeRefs
This API change is already approved by @spadgett so you will get plugin-api-approved with this change
| "resources": { | ||
| "$codeRef": "getKnativeResources.getKnativeEventingKameletsResources" | ||
| }, |
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.
resources expects a WatchK8sResourcesGeneric and not a codeRef.
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.
also since getKnativeEventingKameletsResources always returns the same object we can embed the contents of that directly into the json file--no codeRef needed
jhadvig
left a comment
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.
Nice job @Leo6Leo 👍
/lgtm
/approve
|
/hold for addressing the PR review from @logonoff |
…static-extensions-from-knative-plugin/main
Enable dynamic plugin extensions to use CodeRef for the resources property in topology data model factories. This allows plugins to fetch resources dynamically at runtime (e.g., discovering CRDs). Changes: - Update TopologyDataModelFactory type to accept CodeRef<() => Promise<WatchK8sResourcesGeneric>> - Add Promise resolution in DataModelExtension for CodeRef resources - Update get-knative-resources functions to return Promises with resource definitions - Maintain backward compatibility with static resource definitions The implementation follows the same pattern as other CodeRef properties (getDataModel, isResourceDepicted) by resolving the Promise eagerly during extension initialization and caching the result. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
logonoff
left a comment
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.
nice!
frontend/packages/knative-plugin/src/utils/get-knative-resources.ts
Outdated
Show resolved
Hide resolved
frontend/packages/knative-plugin/src/utils/get-knative-resources.ts
Outdated
Show resolved
Hide resolved
| * Resources type that can be: | ||
| * 1. Static WatchK8sResourcesGeneric (from dynamic plugin SDK) | ||
| * 2. CodeRef<() => Promise<WatchK8sResourcesGeneric>> (from dynamic plugin SDK, for dynamic resources) | ||
| * 3. Function (namespace: string) => WatchK8sResources (from old internal plugin system or converted dynamic plugins) |
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 should only be supporting 1 and 2, this really should be TopologyDataModelFactory.properties.resources.
you can build on my PR, #15617, which removes the final usage of TopologyDataModelFacotry. then we no longer care about its support
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.
Once #15617 merges, I will clean up the DataModelProvider to remove the old modelFactories usage.
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.
FYI this PR has merged
| return { | ||
| ...extension, | ||
| properties: { | ||
| ...extension.properties, | ||
| // Keep the original CodeRef - it will be resolved by useResolvedResources hook | ||
| resources, | ||
| }, | ||
| }; |
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.
doesn't this just return extension?
Co-authored-by: logonoff <[email protected]>
…ic definitions Replaces dynamic CodeRef-based resource definitions with inline static definitions for knative-serving and kamelets topology model factories, removing the need for runtime resolution. Adds compatibility layer for converting dynamic model factories to internal plugin format. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…static-extensions-from-knative-plugin/main
|
/retest |
logonoff
left a comment
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 guess our prettier/eslint setup is too old for satisfies... 🤷
|
/unhold |
logonoff
left a comment
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
|
/assign @yapei |
|
/label px-approved |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhadvig, Leo6Leo, logonoff The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
|
/retest-required |
| prop: key, | ||
| })); | ||
| }, [namespacedDynamicFactories, modelFactories, namespace]); | ||
| }, [namespace]); |
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.
Sometimes, the Application dropdown will got reset back to All applications
reset.mov
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.
I think this is related to https://issues.redhat.com/browse/OCPBUGS-62713 ?
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.
you are right!
| prop: key, | ||
| })); | ||
| }, [namespacedDynamicFactories, modelFactories, namespace]); | ||
| }, [namespace]); |
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.
this issue is also reproducible on 420 nightly, not sure whether the fix should be part of PR scope?
|
performed other regression testings about
|
|
no new issues during PR testing |
|
@yapei: This PR has been marked as verified by In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/retest |
1 similar comment
|
/retest |
|
@Leo6Leo: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/retest |
Description
As a part of the alignment of OpenShift console's dynamic plugin sdk and the OpenShift dynamic plugin sdk, we want to get rid of the long-standing legacy "static plugin" API which is still used in console.
To do this, we can gradually migrate each "static plugin extension" to the equivalent "dynamic extension". This ensures better alignment of web console code with its own SDK.
AC: All static extension in the plugin mentioned in the ticket name is removed and replaced with the dynamic counterpart