Bump dependencies for OCP 4.23/5.0 and add test infrastructure - #118
Draft
razo7 wants to merge 3 commits into
Draft
Bump dependencies for OCP 4.23/5.0 and add test infrastructure#118razo7 wants to merge 3 commits into
razo7 wants to merge 3 commits into
Conversation
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: razo7 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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughVitest is added with runtime configuration, dependency compatibility settings, data-layer test suites, utility fixes, local test commands, and pre-submit test execution. ChangesVitest testing
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PreSubmit
participant Makefile
participant Npm
participant Vitest
PreSubmit->>Makefile: run make test
Makefile->>Npm: run npm run test
Npm->>Vitest: run vitest
Vitest-->>PreSubmit: report test results
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/data/__tests__/model.test.ts`:
- Around line 49-51: Update the parseApiVersion test for the bare "v1" input to
expect the core-group representation used by the existing empty-group cases,
rather than undefined. Ensure parseApiVersion and its downstream converters
continue accepting core Kubernetes resources while preserving rejection of
genuinely invalid version strings.
In `@src/data/__tests__/nodeRoles.test.ts`:
- Around line 64-88: Update getNodeRolesText to use the "-" fallback when its
computed role string is empty by replacing the nullish fallback with a
falsy-value fallback. Add a regression test in the getNodeRolesText suite for a
node with no role labels, asserting it returns "-".
In `@src/data/__tests__/parseErrors.test.ts`:
- Around line 28-30: Update isParseError to safely handle null input and return
false instead of dereferencing it or throwing; revise the null test expectation
in parseErrors.test.ts accordingly while preserving true results for valid parse
errors.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 57cfde5c-11ba-4d4b-b177-cafc6a9ad824
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
.github/workflows/pre-submit.yaml.npmrcMakefilepackage.jsonsrc/data/__tests__/dependency-imports.test.tssrc/data/__tests__/model.test.tssrc/data/__tests__/nodeRoles.test.tssrc/data/__tests__/parseErrors.test.tssrc/data/__tests__/remediator.test.tssrc/data/__tests__/validationSchema.test.tsvitest.config.tsvitest.setup.ts
Comment on lines
+64
to
+88
| describe("getNodeRolesText", () => { | ||
| it("joins sorted unique roles", () => { | ||
| const node = { | ||
| metadata: { | ||
| labels: { | ||
| "node-role.kubernetes.io/worker": "", | ||
| "node-role.kubernetes.io/infra": "", | ||
| }, | ||
| }, | ||
| }; | ||
| expect(getNodeRolesText(node)).toBe("infra, worker"); | ||
| }); | ||
|
|
||
| it("deduplicates roles", () => { | ||
| const node = { | ||
| metadata: { | ||
| labels: { | ||
| "node-role.kubernetes.io/control-plane": "", | ||
| "node-role.kubernetes.io/master": "", | ||
| }, | ||
| }, | ||
| }; | ||
| expect(getNodeRolesText(node)).toBe("control-plane"); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Cover and fix the empty-role fallback.
getNodeRolesText returns "" for a node without roles because "" ?? "-" does not use the fallback. Add the missing regression case and use || in the helper.
Proposed fix
- return uniq(getNodeRoles(node)).sort().join(", ") ?? "-";
+ return uniq(getNodeRoles(node)).sort().join(", ") || "-";+ it("returns a fallback when no roles exist", () => {
+ expect(getNodeRolesText({ metadata: { labels: {} } })).toBe("-");
+ });🤖 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 `@src/data/__tests__/nodeRoles.test.ts` around lines 64 - 88, Update
getNodeRolesText to use the "-" fallback when its computed role string is empty
by replacing the nullish fallback with a falsy-value fallback. Add a regression
test in the getNodeRolesText suite for a node with no role labels, asserting it
returns "-".
There was a problem hiding this comment.
🧹 Nitpick comments (1)
webpack.config.ts (1)
79-82: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPrevent this workaround from permanently disabling shared-module validation.
Lines [79-82] disable validation for every Console-provided shared module, not just
react-router. A future incompatible shared dependency could therefore pass the build and fail at runtime. Tie this bypass to the known SDK prerelease, and restore validation once the SDK peer range is corrected; add an upgrade/CI follow-up so this setting is not forgotten.🤖 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 `@webpack.config.ts` around lines 79 - 82, Update the ConsoleRemotePlugin configuration to bypass shared-module validation only for the known SDK 4.23 prerelease/react-router peer-range mismatch, rather than disabling validation globally. Restore normal validation when that SDK peer range is corrected, and add an upgrade or CI follow-up tied to the workaround so it cannot remain enabled unnoticed.
🤖 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.
Nitpick comments:
In `@webpack.config.ts`:
- Around line 79-82: Update the ConsoleRemotePlugin configuration to bypass
shared-module validation only for the known SDK 4.23 prerelease/react-router
peer-range mismatch, rather than disabling validation globally. Restore normal
validation when that SDK peer range is corrected, and add an upgrade or CI
follow-up tied to the workaround so it cannot remain enabled unnoticed.
razo7
force-pushed
the
deps/ocp-4.23-combined-update
branch
from
July 26, 2026 13:27
c7706de to
bacf710
Compare
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 `@src/data/__tests__/remediator.test.ts`:
- Around line 92-101: Extend the positive test coverage for
isRemediationTemplateSelected with a named SNRTemplate whose namespace is an
empty string. Keep the non-empty name and other existing template fields, and
assert that the function returns true to lock in namespace-independent
selection.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 087b8bb1-bd33-403d-8de3-749425316720
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
.github/workflows/pre-submit.yaml.npmrcMakefilepackage.jsonsrc/data/__tests__/dependency-imports.test.tssrc/data/__tests__/model.test.tssrc/data/__tests__/nodeRoles.test.tssrc/data/__tests__/parseErrors.test.tssrc/data/__tests__/remediator.test.tssrc/data/__tests__/validationSchema.test.tssrc/data/parseErrors.tssrc/data/remediator.tsvitest.config.tsvitest.setup.tswebpack.config.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- .github/workflows/pre-submit.yaml
- vitest.setup.ts
- vitest.config.ts
- src/data/tests/model.test.ts
- Makefile
- .npmrc
- src/data/tests/validationSchema.test.ts
- src/data/tests/parseErrors.test.ts
- webpack.config.ts
- src/data/parseErrors.ts
- package.json
Comment on lines
+92
to
+101
| it("returns true when template has name", () => { | ||
| expect( | ||
| isRemediationTemplateSelected({ | ||
| apiVersion: "v1", | ||
| kind: "SNRTemplate", | ||
| name: "self-node-remediation-automatic-strategy-template", | ||
| namespace: "openshift-operators", | ||
| }) | ||
| ).toBe(true); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test a named template without a namespace.
The positive case includes a namespace, so it would also pass under the old contract. Add a case with a non-empty name and namespace: "" to lock in the documented behavior.
Proposed test
+ it("returns true when template has a name but no namespace", () => {
+ expect(
+ isRemediationTemplateSelected({
+ apiVersion: "v1",
+ kind: "SNRTemplate",
+ name: "self-node-remediation-template",
+ namespace: "",
+ })
+ ).toBe(true);
+ });📝 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.
Suggested change
| it("returns true when template has name", () => { | |
| expect( | |
| isRemediationTemplateSelected({ | |
| apiVersion: "v1", | |
| kind: "SNRTemplate", | |
| name: "self-node-remediation-automatic-strategy-template", | |
| namespace: "openshift-operators", | |
| }) | |
| ).toBe(true); | |
| }); | |
| it("returns true when template has name", () => { | |
| expect( | |
| isRemediationTemplateSelected({ | |
| apiVersion: "v1", | |
| kind: "SNRTemplate", | |
| name: "self-node-remediation-automatic-strategy-template", | |
| namespace: "openshift-operators", | |
| }) | |
| ).toBe(true); | |
| }); | |
| it("returns true when template has a name but no namespace", () => { | |
| expect( | |
| isRemediationTemplateSelected({ | |
| apiVersion: "v1", | |
| kind: "SNRTemplate", | |
| name: "self-node-remediation-template", | |
| namespace: "", | |
| }) | |
| ).toBe(true); | |
| }); |
🤖 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 `@src/data/__tests__/remediator.test.ts` around lines 92 - 101, Extend the
positive test coverage for isRemediationTemplateSelected with a named
SNRTemplate whose namespace is an empty string. Keep the non-empty name and
other existing template fields, and assert that the function returns true to
lock in namespace-independent selection.
razo7
force-pushed
the
deps/ocp-4.23-combined-update
branch
from
July 26, 2026 14:41
bacf710 to
5278794
Compare
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 `@Makefile`:
- Around line 28-31: Update the Makefile test target so it runs npm ci before
npm run test, matching the dependency-installation behavior of the lint target
and allowing make test to work from a clean checkout.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 41ba1cc2-6907-401f-8060-8890cbeaa465
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (17)
.github/workflows/pre-submit.yaml.npmrcMakefilepackage.jsonsrc/data/__tests__/dependency-imports.test.tssrc/data/__tests__/model.test.tssrc/data/__tests__/nodeRoles.test.tssrc/data/__tests__/parseErrors.test.tssrc/data/__tests__/remediator.test.tssrc/data/__tests__/validationSchema.test.tssrc/data/model.tssrc/data/nodeRoles.tssrc/data/parseErrors.tssrc/data/remediator.tsvitest.config.tsvitest.setup.tswebpack.config.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- .github/workflows/pre-submit.yaml
- .npmrc
- vitest.setup.ts
- src/data/tests/nodeRoles.test.ts
- webpack.config.ts
- src/data/tests/validationSchema.test.ts
- src/data/remediator.ts
- src/data/tests/dependency-imports.test.ts
- src/data/parseErrors.ts
- vitest.config.ts
- src/data/tests/remediator.test.ts
- src/data/tests/parseErrors.test.ts
- package.json
Comment on lines
+28
to
+31
| # Run tests | ||
| .PHONY: test | ||
| test: | ||
| npm run test |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the standalone test target install its dependencies.
make test fails in a clean checkout because it skips npm ci, unlike make lint.
Proposed fix
test:
- npm run test
+ npm ci && npm run test📝 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.
Suggested change
| # Run tests | |
| .PHONY: test | |
| test: | |
| npm run test | |
| # Run tests | |
| .PHONY: test | |
| test: | |
| npm ci && npm run test |
🤖 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 `@Makefile` around lines 28 - 31, Update the Makefile test target so it runs
npm ci before npm run test, matching the dependency-installation behavior of the
lint target and allowing make test to work from a clean checkout.
Add vitest as the project's first test framework with 98 tests covering data layer utilities: model helpers, validation regexes, parse error handling, node role functions, remediator logic, and PatternFly/react-router import smoke tests. Fix isParseError type guard crash on null input, support core-group apiVersions in parseApiVersion, fix getNodeRolesText empty-role fallback, and correct isRemediationTemplateSelected docstring. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Update PatternFly 6.4→6.6, react-router 7.13→7.18, and SDK to 4.23.0-prerelease.4, matching the OCP console release-4.23 stack. Add .npmrc with legacy-peer-deps for SDK peer dep mismatch and disable validateSharedModules until SDK publishes a corrected prerelease. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
razo7
force-pushed
the
deps/ocp-4.23-combined-update
branch
from
July 27, 2026 03:53
5278794 to
2a38070
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Combined dependency update for OCP 4.23/5.0 compatibility with test infrastructure
to prevent regressions.
Why
react-router 7.18.x
Changes
websocket-driver, shell-quote security fixes)
import smoke tests (84 tests)
Supersedes: #104, #112, #113, #114, #116, #117
Test plan
npm run lintpasses (0 errors)npm run testpasses (84 tests, 6 files)make docker-buildsucceeds