CONSOLE-5050: Make shared modules peerDeps in SDK#15934
CONSOLE-5050: Make shared modules peerDeps in SDK#15934openshift-merge-bot[bot] merged 1 commit intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@logonoff: This pull request references CONSOLE-5050 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.22.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. |
logonoff
force-pushed
the
CONSOLE-5050-peer-deps
branch
from
6576565 to
82bb698
Compare
📝 WalkthroughWalkthroughThis pull request restructures the dynamic plugin SDK's dependency model by converting Console-provided shared modules from direct dependencies to optional peer dependencies. The changes span package definition scripts, webpack module sharing configuration, and documentation. Key alterations include updating the core package manifest to declare peerDependencies with optional metadata, refactoring the webpack package to move webpack itself into peer dependencies, updating internal module sharing logic to reference peer dependencies instead of direct dependencies, and documenting the breaking change in the changelog. A minor import consolidation in the demo plugin is also included. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
|
@logonoff: This pull request references CONSOLE-5050 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.22.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. |
|
/label px-approved |
openshift-ci
bot
added
px-approved
|
/verified by @logonoff Built and ran plugin SDK, then built demo plugin and everything worked fine |
openshift-ci-robot
added
the
verified
|
@logonoff: This PR has been marked as verified by 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/packages/console-dynamic-plugin-sdk/CHANGELOG-core.md (1)
15-17: Complete the 4.22 section inupgrade-sdk.mdwith peer dependency migration guidance.The CHANGELOG directs users to
upgrade-sdk.mdfor breaking change details, but that file's 4.22 section is incomplete (currently just "TODO"). Ensure it documents how plugin authors should handle the new optional peer dependencies—specifically, when and how to add explicit dependencies to theirpackage.jsonif they rely on specific versions of Console-provided shared modules.
🤖 Fix all issues with AI agents
In
`@frontend/packages/console-dynamic-plugin-sdk/src/webpack/ConsoleRemotePlugin.ts`:
- Around line 50-51: The call to loadVendorPackageJSON in
getPluginSDKPackagePeerDependencies can return a package object without
peerDependencies, causing sdkPkgDeps[moduleName] to be undefined and
semver.validRange to return null; update getPluginSDKPackagePeerDependencies (or
its caller) to defensively handle a missing peerDependencies by treating it as
an empty object (e.g., use peerDependencies ?? {}) before indexing by
moduleName, and ensure any use of sdkPkgDeps in functions that call
semver.validRange (refer to moduleName and semver.validRange usages) checks for
undefined ranges and fails fast or logs a clear error; alternatively, document
that loadVendorPackageJSON must return a fully built package with
peerDependencies present.
🧹 Nitpick comments (2)
frontend/packages/console-dynamic-plugin-sdk/scripts/package-definitions.ts (1)
128-134: Consider cachingparseSharedModuleDepsresult to avoid duplicate computation.
parseSharedModuleDepsis invoked twice with identical arguments (lines 129 and 132). While the overhead is minimal, extracting to a variable improves clarity and ensures consistency.♻️ Suggested refactor
export const getCorePackage: GetPackageDefinition = ( sdkPackage, rootPackage, missingDepCallback, -) => ({ +) => { + const sharedModuleDeps = parseSharedModuleDeps(rootPackage, missingDepCallback); + return { outDir: 'dist/core', manifest: { name: '@openshift-console/dynamic-plugin-sdk', version: sdkPackage.version, description: 'Provides core APIs, types and utilities used by dynamic plugins at runtime.', main: 'lib/lib-core.js', ...commonManifestFields, dependencies: { ...parseDeps(sdkPackage, ['@openshift/dynamic-plugin-sdk'], missingDepCallback), ...parseDeps(rootPackage, ['immutable', 'reselect', 'typesafe-actions'], missingDepCallback), ...parseDepsAs(rootPackage, { 'lodash-es': 'lodash' }, missingDepCallback), }, peerDependencies: { - ...parseSharedModuleDeps(rootPackage, missingDepCallback), + ...sharedModuleDeps, }, - peerDependenciesMeta: _.mapValues( - parseSharedModuleDeps(rootPackage, missingDepCallback), - () => ({ optional: true }), - ), + peerDependenciesMeta: _.mapValues(sharedModuleDeps, () => ({ optional: true })), }, // ...rest unchanged + }; -});frontend/packages/console-dynamic-plugin-sdk/CHANGELOG-core.md (1)
28-28: Consider adding**Breaking**:prefix for consistency.Other breaking changes in this section use the
**Breaking**:or**Type breaking**:prefix. Since this change requires plugin authors to update theirpackage.json(adding peer dependencies explicitly or accepting warnings), it warrants the same treatment for discoverability.📝 Suggested change
-- Make all Console-provided shared modules optional peer dependencies ([CONSOLE-5050], [`#15934`]) +- **Breaking**: Make all Console-provided shared modules optional peer dependencies ([CONSOLE-5050], [`#15934`])
| const getPluginSDKPackagePeerDependencies = () => | ||
| loadVendorPackageJSON('@openshift-console/dynamic-plugin-sdk').peerDependencies; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find the file and examine the function and call sites
cat -n frontend/packages/console-dynamic-plugin-sdk/src/webpack/ConsoleRemotePlugin.ts | head -150Repository: openshift/console
Length of output: 6438
🏁 Script executed:
#!/bin/bash
# Find where loadVendorPackageJSON is defined
rg -n 'loadVendorPackageJSON' --type ts -B 2 -A 5Repository: openshift/console
Length of output: 2656
🏁 Script executed:
#!/bin/bash
# Check if the dynamic-plugin-sdk package.json exists and has peerDependencies
cat frontend/packages/console-dynamic-plugin-sdk/package.json | jq '.peerDependencies'Repository: openshift/console
Length of output: 65
🏁 Script executed:
#!/bin/bash
# Look for how peerDependencies is used at the call sites (lines 59, 125)
cat -n frontend/packages/console-dynamic-plugin-sdk/src/webpack/ConsoleRemotePlugin.ts | sed -n '50,70p;120,135p'Repository: openshift/console
Length of output: 1884
🏁 Script executed:
#!/bin/bash
# Find the SDK package.json directly in the repository
find . -path "*/node_modules" -prune -o -name "package.json" -type f -print | xargs grep -l '"name".*"@openshift-console/dynamic-plugin-sdk"' 2>/dev/null | head -5Repository: openshift/console
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Check if dynamic-plugin-sdk is in packages or if it's published separately
ls -la frontend/packages/*/package.json | grep -i sdkRepository: openshift/console
Length of output: 266
🏁 Script executed:
#!/bin/bash
# Inspect the shared-modules to understand what modules are being configured
rg -n 'sharedPluginModules' --type ts -B 2 -A 15 | head -80Repository: openshift/console
Length of output: 8939
🏁 Script executed:
#!/bin/bash
# Check if there's a package.json that SHOULD have peerDependencies
find frontend/packages -name "package.json" -type f | xargs grep -l "dynamic-plugin-sdk" | head -3Repository: openshift/console
Length of output: 164
🏁 Script executed:
#!/bin/bash
# Check the source SDK package.json
cat frontend/packages/console-dynamic-plugin-sdk/package.jsonRepository: openshift/console
Length of output: 1263
🏁 Script executed:
#!/bin/bash
# Read the getCorePackage function to see how the manifest is created
sed -n '111,200p' frontend/packages/console-dynamic-plugin-sdk/scripts/package-definitions.tsRepository: openshift/console
Length of output: 2886
🏁 Script executed:
#!/bin/bash
# Look for where peerDependencies might be set in the manifest
rg -n 'peerDependencies' frontend/packages/console-dynamic-plugin-sdk --type ts -B 2 -A 2Repository: openshift/console
Length of output: 2312
🏁 Script executed:
#!/bin/bash
# Check parseSharedModuleDeps to understand what it returns
sed -n '40,100p' frontend/packages/console-dynamic-plugin-sdk/scripts/package-definitions.tsRepository: openshift/console
Length of output: 1931
🏁 Script executed:
#!/bin/bash
# Check if there's any guard or try-catch around the function call
rg -n 'getPluginSDKPackagePeerDependencies' --type ts -B 3 -A 8Repository: openshift/console
Length of output: 4254
🏁 Script executed:
#!/bin/bash
# Check shared-modules-meta to understand what modules are being configured
cat frontend/packages/console-dynamic-plugin-sdk/src/shared-modules/shared-modules-meta.ts | head -60Repository: openshift/console
Length of output: 1735
Guard against undefined peerDependencies or document the assumption.
The function reads peerDependencies from the published SDK package, which is correctly populated at build time. However, there's no defensive check: if the field is missing (incomplete package, dev builds, corrupted install), accessing sdkPkgDeps[moduleName] silently returns undefined, causing semver.validRange() to return null. This bypasses all version constraint enforcement for shared modules—a critical issue that manifests silently with no error. Either add a guard (peerDependencies ?? {}) or explicitly document that the published package must be fully built and present.
🤖 Prompt for AI Agents
In
`@frontend/packages/console-dynamic-plugin-sdk/src/webpack/ConsoleRemotePlugin.ts`
around lines 50 - 51, The call to loadVendorPackageJSON in
getPluginSDKPackagePeerDependencies can return a package object without
peerDependencies, causing sdkPkgDeps[moduleName] to be undefined and
semver.validRange to return null; update getPluginSDKPackagePeerDependencies (or
its caller) to defensively handle a missing peerDependencies by treating it as
an empty object (e.g., use peerDependencies ?? {}) before indexing by
moduleName, and ensure any use of sdkPkgDeps in functions that call
semver.validRange (refer to moduleName and semver.validRange usages) checks for
undefined ranges and fails fast or logs a clear error; alternatively, document
that loadVendorPackageJSON must return a fully built package with
peerDependencies present.
82bb698 to
59ee45c
Compare
openshift-ci-robot
removed
the
verified
openshift-ci
bot
added
component/sdk
logonoff
force-pushed
the
CONSOLE-5050-peer-deps
branch
from
59ee45c to
12b99ac
Compare
logonoff
force-pushed
the
CONSOLE-5050-peer-deps
branch
from
12b99ac to
36345be
Compare
|
@logonoff: all tests passed! 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. |
|
/verified by @logonoff |
openshift-ci-robot
added
the
verified
|
@logonoff: This PR has been marked as verified by 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. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: logonoff, 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 |
1 similar comment
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: logonoff, 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 |
3 participants
Summary by CodeRabbit
Release Notes
Breaking Changes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.