CONSOLE-5065: Align Console useResolvedExtensions hook with upstream plugin SDK#16815
CONSOLE-5065: Align Console useResolvedExtensions hook with upstream plugin SDK#16815vojtechszocs wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
WalkthroughThe dynamic plugin SDK dependencies were upgraded, and extension resolution APIs now accept a single predicate and return loaded-and-resolved extension types. Related consumer typings, imports, tests, and release notes were updated. ChangesExtension Resolution API
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vojtechszocs The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
component/shared
vojtechszocs
changed the title
| > with unions of distinct extension objects. | ||
|
|
||
| - **Type breaking**: Replace `ExtensionTypeGuard` with `ExtensionPredicate` from `@openshift/dynamic-plugin-sdk` ([#16115], [CONSOLE-5065]) | ||
| - **Breaking**: `useResolvedExtensions` hook now accepts a single predicate parameter (TODO) |
There was a problem hiding this comment.
Still don't know how I feel about making a runtime breaking change in this release, even if most plugins won't be affected...
There was a problem hiding this comment.
Looking at git history, Console useExtensions initially supported multiple predicates and then it was aligned with upstream useExtensions to use a single predicate.
Console useResolvedExtensions should have been aligned accordingly, but we missed that and doing it now would yield a potentially breaking change. That said, most (if not all) consumers should not use >1 predicate since doing otherwise results in additional complexity (due to dealing with object type unions).
I'd rather do this change now than wait for the next release, but if others think we must avoid breaking changes (regardless of how big or small they are) then I can keep the glue code for evaluating multiple predicates.
openshift-ci-robot
added
the
jira/valid-reference
|
@vojtechszocs: This pull request references CONSOLE-5065 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 "5.0.0" version, but no target version was set. DetailsIn 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/package.json`:
- Around line 68-69: Pin the dependency versions for
`@openshift/dynamic-plugin-sdk` and `@openshift/dynamic-plugin-sdk-webpack` exactly
instead of using caret ranges, update the lockfile while preserving integrity
metadata, and run the repository’s OSV/SCA check against the resolved dependency
graph to identify any known CVEs.
In `@frontend/packages/console-dynamic-plugin-sdk/CHANGELOG-core.md`:
- Around line 15-20: Update the useResolvedExtensions breaking-change entry in
CHANGELOG-core.md to state that its public return type changed from
ResolvedExtension[] to LoadedAndResolvedExtension[]; remove “(TODO)” and change
“once per each” to “once for each,” while preserving the existing
predicate-arity and ExtensionTypeGuard details.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 38f06436-5add-4737-8850-43f71ca7733c
⛔ Files ignored due to path filters (1)
frontend/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
frontend/package.jsonfrontend/packages/console-dynamic-plugin-sdk/CHANGELOG-core.mdfrontend/packages/console-dynamic-plugin-sdk/src/api/useResolvedExtensions.tsfrontend/packages/console-dynamic-plugin-sdk/src/extensions/console-types.tsfrontend/packages/console-dynamic-plugin-sdk/src/types.tsfrontend/packages/console-shared/src/hooks/useDetailsItemExtensionsForResource.ts
💤 Files with no reviewable changes (1)
- frontend/packages/console-dynamic-plugin-sdk/src/types.ts
| "@openshift/dynamic-plugin-sdk": "^9.0.0", | ||
| "@openshift/dynamic-plugin-sdk-webpack": "^5.3.0", |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Pin the upgraded SDK packages exactly.
^9.0.0 and ^5.3.0 permit future resolutions to change the SDK and transitive dependency graph. Use exact versions and ensure the lockfile retains integrity metadata; run the repository’s OSV/SCA check for the resolved graph.
As per path instructions, package manifests must pin exact versions, verify hashes where supported, and flag known CVEs.
Proposed fix
- "`@openshift/dynamic-plugin-sdk`": "^9.0.0",
- "`@openshift/dynamic-plugin-sdk-webpack`": "^5.3.0",
+ "`@openshift/dynamic-plugin-sdk`": "9.0.0",
+ "`@openshift/dynamic-plugin-sdk-webpack`": "5.3.0",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "@openshift/dynamic-plugin-sdk": "^9.0.0", | |
| "@openshift/dynamic-plugin-sdk-webpack": "^5.3.0", | |
| "`@openshift/dynamic-plugin-sdk`": "9.0.0", | |
| "`@openshift/dynamic-plugin-sdk-webpack`": "5.3.0", |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/package.json` around lines 68 - 69, Pin the dependency versions for
`@openshift/dynamic-plugin-sdk` and `@openshift/dynamic-plugin-sdk-webpack` exactly
instead of using caret ranges, update the lockfile while preserving integrity
metadata, and run the repository’s OSV/SCA check against the resolved dependency
graph to identify any known CVEs.
Source: Path instructions
| > This release removes support for passing multiple predicates to `useResolvedExtensions` hook. | ||
| > Consumers should call this hook once per each distinct extension type to avoid complexity associated | ||
| > with unions of distinct extension objects. | ||
|
|
||
| - **Type breaking**: Replace `ExtensionTypeGuard` with `ExtensionPredicate` from `@openshift/dynamic-plugin-sdk` ([#16115], [CONSOLE-5065]) | ||
| - **Breaking**: `useResolvedExtensions` hook now accepts a single predicate parameter (TODO) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the complete breaking change before publishing.
The public return type also changed from ResolvedExtension[] to LoadedAndResolvedExtension[], but this note only covers predicate arity and the renamed type guard. Add that return-type change, remove (TODO), and change “once per each” to “once for each.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/packages/console-dynamic-plugin-sdk/CHANGELOG-core.md` around lines
15 - 20, Update the useResolvedExtensions breaking-change entry in
CHANGELOG-core.md to state that its public return type changed from
ResolvedExtension[] to LoadedAndResolvedExtension[]; remove “(TODO)” and change
“once per each” to “once for each,” while preserving the existing
predicate-arity and ExtensionTypeGuard details.
8becdb0 to
6d9f709
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
frontend/packages/console-dynamic-plugin-sdk/CHANGELOG-core.md (1)
15-20: 📐 Maintainability & Code Quality | 🟡 MinorComplete the breaking-change note before publishing.
This entry still leaves
(TODO), says “once per each,” and omits the return-type change fromResolvedExtension[]toLoadedAndResolvedExtension[].🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/packages/console-dynamic-plugin-sdk/CHANGELOG-core.md` around lines 15 - 20, The changelog entry for useResolvedExtensions must be publication-ready: remove “(TODO)”, correct the wording to “once for each distinct extension type,” and explicitly document the return-type change from ResolvedExtension[] to LoadedAndResolvedExtension[].
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@frontend/packages/console-dynamic-plugin-sdk/CHANGELOG-core.md`:
- Around line 15-20: The changelog entry for useResolvedExtensions must be
publication-ready: remove “(TODO)”, correct the wording to “once for each
distinct extension type,” and explicitly document the return-type change from
ResolvedExtension[] to LoadedAndResolvedExtension[].
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3e9068ef-4a0d-49be-9b3a-02a448e7d94e
⛔ Files ignored due to path filters (1)
frontend/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
frontend/package.jsonfrontend/packages/console-app/src/components/nodes/node-dashboard/InventoryCard.tsxfrontend/packages/console-app/src/components/nodes/node-dashboard/__tests__/InventoryCard.spec.tsxfrontend/packages/console-dynamic-plugin-sdk/CHANGELOG-core.mdfrontend/packages/console-dynamic-plugin-sdk/src/api/useResolvedExtensions.tsfrontend/packages/console-dynamic-plugin-sdk/src/extensions/console-types.tsfrontend/packages/console-dynamic-plugin-sdk/src/types.tsfrontend/packages/console-shared/src/hooks/useDetailsItemExtensionsForResource.ts
💤 Files with no reviewable changes (1)
- frontend/packages/console-dynamic-plugin-sdk/src/types.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/package.json
- frontend/packages/console-dynamic-plugin-sdk/src/extensions/console-types.ts
|
@vojtechszocs: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
5 participants
Analysis / Root cause
Align Console
useResolvedExtensionshook with changes in openshift/dynamic-plugin-sdk#313useExtensionshook (not exposed via plugin API) accepts a single predicateuseResolvedExtensionshook updated to accept a single predicate, which follows the upstream hook signatureImportant
Consumers should call this hook once per each distinct extension type to avoid complexity associated with unions of distinct extension objects.
Solution description
Modify Console
useResolvedExtensionshook and its types according to the above analysis.Additional changes
ExtensionTypeGuardwithExtensionPredicatefrom@openshift/dynamic-plugin-sdkconsole-app/src/components/nodes/node-dashboard/InventoryCard.tsxto use Console hook@openshift/dynamic-plugin-sdkversion to9.0.0@openshift/dynamic-plugin-sdk-webpackversion to5.3.0Summary by CodeRabbit
Breaking Changes
Chores
Tests