OCPBUGS-85043: Remove @console imports from SDK dist#16399
OCPBUGS-85043: Remove @console imports from SDK dist#16399logonoff wants to merge 2 commits intoopenshift:mainfrom
@console imports from SDK dist#16399Conversation
|
@logonoff: This pull request references Jira Issue OCPBUGS-85043, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
/verified by test |
|
@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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: logonoff 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 |
|
@logonoff: This pull request references Jira Issue OCPBUGS-85043, which is valid. 3 validation(s) were run on this bug
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. |
📝 WalkthroughWalkthroughThis pull request consolidates type definitions and enforces distribution integrity across the OpenShift Console SDK. The 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/__tests__/sdk-dist-imports.spec.ts`:
- Around line 25-58: The test currently splits files into lines and uses
importFromConsoleRe which requires a "from" clause, so it misses side-effect
imports and multiline imports; update the test to run regex matching against the
entire file content (not line-by-line) and change importFromConsoleRe to also
match side-effect imports by making the from-clause optional (capture the
`@console` specifier whether it's in a from '...' form or a bare import '...');
when collecting violations use the full-content regex matches (extract the
specifier from the appropriate capture group) and keep pushing { file:
relative(DIST_DIR, filePath), line: (derive line number from
content.indexOf(match) or compute line by counting newlines up to match.index),
specifier } so multiline and side-effect imports are detected (update references
to importFromConsoleRe, the loop that uses lines[], and the violations push
logic).
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: f2ced5a9-cd3e-4c15-b556-0c7626ce43e9
📒 Files selected for processing (8)
frontend/__tests__/sdk-dist-imports.spec.tsfrontend/packages/console-dynamic-plugin-sdk/src/app/components/factory/ListPage/ListPageBody.tsxfrontend/packages/console-dynamic-plugin-sdk/src/app/core/actions/core.tsfrontend/packages/console-dynamic-plugin-sdk/src/app/core/reducers/coreSelectors.tsfrontend/packages/console-dynamic-plugin-sdk/src/app/redux-types.tsfrontend/packages/console-dynamic-plugin-sdk/src/extensions/console-types.tsfrontend/packages/console-dynamic-plugin-sdk/src/utils/k8s/k8s-resource.tsfrontend/public/module/k8s/types.ts
💤 Files with no reviewable changes (1)
- frontend/public/module/k8s/types.ts
📜 Review details
🧰 Additional context used
🪛 OpenGrep (1.20.0)
frontend/__tests__/sdk-dist-imports.spec.ts
[ERROR] 45-45: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 47-47: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🔇 Additional comments (7)
frontend/packages/console-dynamic-plugin-sdk/src/app/components/factory/ListPage/ListPageBody.tsx (2)
2-2: Good dependency boundary change for SDK dist.Switching to
PageSectionfrom@patternfly/react-coreremoves reliance on internal console component imports and aligns with the dist-packaging objective.
8-12:ListPageBodyrefactor preserves behavior cleanly.Using
PageSectionwithclassName="co-m-pane__body"andhasBodyWrapper={false}keeps the wrapper semantics/layout intent while avoiding extra nested body markup.frontend/packages/console-dynamic-plugin-sdk/src/extensions/console-types.ts (1)
1187-1191: Good consolidation ofUserKindinto SDK console types.This keeps the user resource type in the public SDK surface and supports the dist-import cleanup goal.
frontend/packages/console-dynamic-plugin-sdk/src/app/redux-types.ts (1)
4-4: Import path update looks correct.Using the SDK-local type source here is consistent with the monorepo import removal effort.
frontend/packages/console-dynamic-plugin-sdk/src/app/core/actions/core.ts (1)
3-3: Type import consolidation is solid.Line 3 correctly points action payload typing at the SDK extensions export surface.
frontend/packages/console-dynamic-plugin-sdk/src/app/core/reducers/coreSelectors.ts (1)
2-2: Selector type import switch is correct.This keeps selector contracts stable while decoupling from monorepo-internal type paths.
frontend/packages/console-dynamic-plugin-sdk/src/utils/k8s/k8s-resource.ts (1)
5-5:coFetchJSONsource change is aligned and safe.Line 5 removes the monorepo-shared import path while preserving existing call patterns in this module.
8371c06 to
707686e
Compare
| extra?: object; | ||
| }; | ||
|
|
||
| export type UserKind = { |
There was a problem hiding this comment.
Should we re-export UserKind from console-types.ts ?
There was a problem hiding this comment.
Define and export UserKind in extensions/console-types, remove the duplicate definition from public k8s types, and update affected files to import UserKind/UserInfo from the extensions path. Replace PaneBody with PatternFly PageSection in ListPageBody. Change k8s resource fetch import to use consoleFetchJSON (aliased as coFetchJSON) from lib-core. Add a test (sdk-dist-imports.spec.ts) that scans the SDK dist output to ensure no ES import/export statements reference monorepo packages, preventing accidental bundling of internal module specifiers.
Move consoleFetch, consoleFetchJSON, and consoleFetchText from core-api.ts into a new console-fetch.ts module. This breaks the circular dependency (k8s-resource → lib-core → core-api → utils/k8s → k8s-resource) by allowing k8s-resource.ts to import consoleFetchJSON from console-fetch.ts, which has no back-edge into the k8s utils. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
852e13f to
49dcfee
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. |
Analysis / Root cause:
@consolemonorepo imports do not work in the dist package and so should not be includedSolution description:
Test cases:
Summary by CodeRabbit
Tests
Refactor
Updates