CONSOLE-5227: Upgrade ESLint from v6 to v8 and @typescript-eslint to v8#16353
CONSOLE-5227: Upgrade ESLint from v6 to v8 and @typescript-eslint to v8#16353openshift-merge-bot[bot] merged 1 commit intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@logonoff: This pull request references CONSOLE-5227 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. |
|
/label px-approved |
openshift-ci
Bot
added
component/core
📝 WalkthroughWalkthroughThis pull request updates the frontend's ESLint toolchain and configurations across multiple packages. The changes migrate from 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/packages/dev-console/src/components/add/layout/Masonry.tsx (1)
39-44:⚠️ Potential issue | 🟡 MinorBug risk: treating measured height
0as “unknown” due to truthiness checks.In
MeasuredItem, this condition treatscurrentHeight === 0as falsy and will re-emitonHeightMeasuredeven when there’s no meaningful change:
if (!currentHeight || Math.abs(currentHeight - newHeight) > 1)Same issue exists later with
if (heights[itemKey])(Line 103) which will also treat a legitimate stored height of0as “not measured yet”.🛠️ Proposed fix
- if (!currentHeight || Math.abs(currentHeight - newHeight) > 1) { + if (currentHeight === undefined || Math.abs(currentHeight - newHeight) > 1) { onHeightMeasured(itemKey, newHeight); }- if (heights[itemKey]) { + if (heights[itemKey] !== undefined) { column.height += heights[itemKey]; column.items.push(measuredItem); return; }This keeps the “unknown vs known” distinction correct even when the DOM reports a height of exactly
0.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/dev-console/src/components/add/layout/Masonry.tsx` around lines 39 - 44, The code in MeasuredItem treats a stored height of 0 as "unknown" due to falsy checks; change the checks to explicitly test for undefined/null instead of relying on truthiness. Replace `if (!currentHeight || Math.abs(currentHeight - newHeight) > 1)` with an explicit existence check such as `if (currentHeight === undefined || currentHeight === null || Math.abs(currentHeight - newHeight) > 1)` (or `currentHeight === undefined` if null isn't used), and replace `if (heights[itemKey])` with an explicit check like `if (heights[itemKey] !== undefined && heights[itemKey] !== null)` (or `heights.hasOwnProperty(itemKey)` / `heights[itemKey] !== undefined`) so a legitimate height of 0 is treated as known.frontend/packages/webterminal-plugin/src/components/cloud-shell/CloudShellTerminal.tsx (1)
148-157:⚠️ Potential issue | 🟡 MinorRe-check
unmountedinside the nestedtext()handlers.Lines 149-156 can still call
setInitError(...)after the effect has cleaned up, because the inner.then/.catchdon't re-checkunmounted. That can race when the workspace changes quickly and reintroduces the post-unmount update the outer guard is trying to avoid.Suggested fix
e.response .text() .then((text) => { - setInitError(text); + if (!unmounted) { + setInitError(text); + } }) .catch(() => { - setInitError(defaultError); + if (!unmounted) { + setInitError(defaultError); + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/webterminal-plugin/src/components/cloud-shell/CloudShellTerminal.tsx` around lines 148 - 157, The nested promise handlers for e.response.text() can call setInitError after the component has unmounted; update the .then and .catch handlers used in CloudShellTerminal so they re-check the same unmounted flag (or an isMounted ref) before calling setInitError (and before any state updates) inside the handlers for e.response.text(), ensuring setInitError is only invoked when still mounted; locate the block around e.response.text().then(...).catch(...) and add the guard check in both the then and catch branches (or use AbortController to cancel the read) to prevent post-unmount state updates.
🧹 Nitpick comments (6)
frontend/packages/webterminal-plugin/src/redux/actions/cloud-shell-actions.ts (1)
19-26: LGTM — eslint suppression is justified; consideras constfor stronger typing intent.The
// eslint-disable-next-line@typescript-eslint/no-unused-vars`` is appropriately scoped:actionsis intentionally only referenced via `typeof actions` to derive `CloudShellActions`, matching the stated “used in typeof for type export” rationale.Optional: you can make the typing intent more explicit by marking the
actionsobject asas const(helps TS keep exact property/value types in some generic scenarios with mapped/conditional types):Suggested tweak (optional)
-// eslint-disable-next-line `@typescript-eslint/no-unused-vars` -- used in typeof for type export -const actions = { +// eslint-disable-next-line `@typescript-eslint/no-unused-vars` -- used in typeof for type export +const actions = { setCloudShellExpanded, setCloudShellActive, setCloudShellCommand, -}; +} as const;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/webterminal-plugin/src/redux/actions/cloud-shell-actions.ts` around lines 19 - 26, The actions object is intentionally only used via typeof to derive CloudShellActions; to make the typing intent stronger change the declaration of actions to be read-only by appending as const so TypeScript preserves exact property/value types — update the const actions = { setCloudShellExpanded, setCloudShellActive, setCloudShellCommand } to be declared with as const (affecting the inferred type used by ActionType<typeof actions> and referencing the symbols actions, setCloudShellExpanded, setCloudShellActive, setCloudShellCommand, and CloudShellActions).frontend/packages/console-app/src/hooks/useNamespacedNotificationAlertsPoller.ts (1)
18-63: Unsubscribe logic clears the timeout, but doesn’t cancel in-flight requests.The effect cleanup (lines 58-62) clears the scheduled
setTimeout, but the in-flightcoFetchJSON(...).then(...)/catch(...)can still resolve/reject after unmount and callsetAlerts/setLoaded/setLoadError. If the promise can’t be canceled, you may want alet isMounted = trueguard (or AbortController ifcoFetchJSONsupports it).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/console-app/src/hooks/useNamespacedNotificationAlertsPoller.ts` around lines 18 - 63, The cleanup must prevent state updates from in-flight coFetchJSON calls: in the useEffect containing poll/pollTimeout, add a mounted guard (e.g., let isMounted = true) or use an AbortController if coFetchJSON supports it, then check isMounted before calling setAlerts/setLoaded/setLoadError inside the poll promise handlers (and call controller.abort() or set isMounted = false in the returned cleanup). Update references in this effect to use the guard (or controller) so no state setters in poll run after unmount.frontend/packages/dev-console/src/components/add/layout/Masonry.tsx (1)
23-56: Recommended: avoid recreating the ResizeObserver on every height change.
useEffectdepends oncurrentHeight(Line 55). That means whenever height updates, the effect re-runs, re-creates the observer, and re-attaches it—unnecessary churn and potential jank.A common pattern is to keep
currentHeightin a ref and let the observer effect depend only on stable inputs (itemKey,onHeightMeasured), while reading the latest value from the ref inside the callback.♻️ Suggested refactor (outline)
const MeasuredItem = memo<MeasuredItemProps>( ({ item, itemKey, onHeightMeasured, currentHeight }) => { const measureRef = useRef<HTMLDivElement>(null); + const currentHeightRef = useRef<number | undefined>(currentHeight); + useEffect(() => { + currentHeightRef.current = currentHeight; + }, [currentHeight]); useEffect(() => { if (!measureRef.current) return undefined; let rafId: number; const resizeObserver = new ResizeObserver((entries) => { const entry = entries[0]; if (!entry) return; if (rafId) { cancelAnimationFrame(rafId); } rafId = requestAnimationFrame(() => { const newHeight = entry.contentRect.height; - if (currentHeight === undefined || Math.abs(currentHeight - newHeight) > 1) { + const prevHeight = currentHeightRef.current; + if (prevHeight === undefined || Math.abs(prevHeight - newHeight) > 1) { onHeightMeasured(itemKey, newHeight); } }); }); resizeObserver.observe(measureRef.current); return () => { if (rafId) { cancelAnimationFrame(rafId); } resizeObserver.disconnect(); }; - }, [itemKey, onHeightMeasured, currentHeight]); + }, [itemKey, onHeightMeasured]);This should preserve behavior while reducing observer lifecycle churn.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/dev-console/src/components/add/layout/Masonry.tsx` around lines 23 - 56, The effect creating the ResizeObserver is re-run whenever currentHeight changes (because useEffect depends on currentHeight), causing unnecessary observer recreation; to fix, keep the latest height in a ref (e.g., currentHeightRef) and update that ref in a separate useEffect that depends on currentHeight, then change the ResizeObserver effect (the one using measureRef, requestAnimationFrame/rafId, and calling onHeightMeasured(itemKey, newHeight)) to depend only on stable inputs (itemKey and onHeightMeasured) and read the latest value from currentHeightRef inside the observer callback; preserve the existing cancelAnimationFrame and resizeObserver.disconnect() cleanup logic in the observer effect.frontend/public/components/app.tsx (1)
502-520: Add a.catch()to the service-worker load-test promise chain (minor reliability).The dynamic import → register → controller-ready → postMessage chain has no terminal error handling. If any step rejects, you’ll rely on the global
window.onunhandledrejectionhandler, which is good for visibility but not for controlled UX/test behavior.🔧 Suggested diff
import('file-loader?name=load-test.sw.js!../load-test.sw.js') .then(() => navigator.serviceWorker.register('/load-test.sw.js')) .then( () => new Promise<void>((r) => navigator.serviceWorker.controller ? r() : navigator.serviceWorker.addEventListener('controllerchange', () => r()), ), ) .then(() => navigator.serviceWorker.controller.postMessage({ topic: 'setFactor', value: window.SERVER_FLAGS.loadTestFactor, }), - ); + ) + .catch((e) => { + // eslint-disable-next-line no-console + console.warn('Failed to initialize load-test service worker', e); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/public/components/app.tsx` around lines 502 - 520, The load-test service-worker promise chain (starting from the dynamic import of 'file-loader?name=load-test.sw.js!../load-test.sw.js' through navigator.serviceWorker.register, the controller-ready Promise, and the postMessage call) lacks terminal error handling; append a .catch handler to that chain to catch any rejection and handle it (e.g., console.error or a dedicated logger and optionally set a fallback flag/UI), so failures to import, register, wait for controller, or postMessage are logged and do not rely solely on global unhandled rejection handling.frontend/packages/dev-console/integration-tests/testData/git-import/repos.ts (2)
1-1: Narrow the eslint-disable scope (avoid blanket file-level suppression).File-level disabling on Line 1 hides linting for the entire module. Prefer
eslint-disable-next-line import/no-dynamic-require,@typescript-eslint/no-require-imports`` on the specificrequire(\./${repo.folder}/...`)` lines, so other issues in this file still get caught by ESLint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/dev-console/integration-tests/testData/git-import/repos.ts` at line 1, Remove the file-level eslint-disable comment and instead place targeted inline disables directly above the dynamic require expressions (the lines that call require(`./${repo.folder}/...`)); use eslint-disable-next-line import/no-dynamic-require, `@typescript-eslint/no-require-imports` so only those specific require calls bypass the rules while the rest of the file stays linted. Ensure you remove the top-of-file suppression and add the inline comments immediately before each dynamic require usage (e.g., the require in the loop that references repo.folder).
49-50: Consider consistent error-handling for required JSON files.
repo.jsonandcontents.jsonare required (no try/catch on Lines 49–50). If the test dataset is ever extended/mutated, missing required files will throw and may make failures harder to diagnose. Optional: either add explicit error messages (wrap in try/catch with a clear “missing test data for folder X”) or mirror the “optional” pattern for missing files if that’s acceptable for the integration tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/dev-console/integration-tests/testData/git-import/repos.ts` around lines 49 - 50, Wrap the synchronous requires for repoJson and contentsJson in a try/catch so missing or malformed JSON yields a clear, actionable error; specifically, catch errors around the require statements that load `./${repo.folder}/repo.json` and `./${repo.folder}/contents.json` and rethrow or log a descriptive message that includes `repo.folder` and the missing filename (e.g., "missing test data for folder <repo.folder>: repo.json") so failures are easier to diagnose when `repoJson` or `contentsJson` cannot be loaded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@frontend/packages/eslint-plugin-console/lib/config/rules/airbnb-base-overrides.js`:
- Around line 95-108: The airbnb override is redundantly disabling the import
rules 'import/named' and 'import/no-named-as-default-member' and the true cause
is the TypeScript parser applied globally; remove these two rule entries from
the airbnb-base-overrides module and instead scope parser usage to TypeScript
files in the root ESLint config by adding an "overrides" block that sets parser:
'@typescript-eslint/parser' for files matching *.ts and *.tsx (so plain JS uses
the default parser), or conversely remove them only from the root config if you
prefer keeping JS-only behavior in this override; locate the disabled rules by
name ('import/named', 'import/no-named-as-default-member') in the override file
and update the root ESLint config's overrides section to apply the TypeScript
parser only to TS file globs.
In `@frontend/packages/eslint-plugin-console/lib/config/rules/react.js`:
- Around line 48-58: Move the TypeScript-justified rule disables out of the
global rules block and into the existing .tsx override: remove
'react/no-unused-prop-types', 'react/forbid-prop-types', and
'react/require-default-props' from the base rules in lib/config/rules/react.js
and add them (with the same 'off' values and comments) to the overrides section
that targets .tsx files (where other TypeScript-specific disables live),
ensuring propTypes rules remain active for .js/.jsx while being relaxed for
.tsx.
In `@frontend/packages/eslint-plugin-console/lib/config/rules/typescript.js`:
- Around line 222-230: Update the restricted-paths entries in
frontend/packages/eslint-plugin-console/lib/config/rules/typescript.js so the
glob patterns match files recursively: change the offending "from" strings
'node_modules/@patternfly/*/dist/js' and 'node_modules/@patternfly/*/dist/cjs'
to include '/**/*' (e.g. 'node_modules/@patternfly/*/dist/js/**/*' and
'node_modules/@patternfly/*/dist/cjs/**/*') used in the objects with message
"Import from the package index instead of the CJS dist/js path." and the similar
CJS message so the import/no-restricted-paths rule catches nested files.
---
Outside diff comments:
In `@frontend/packages/dev-console/src/components/add/layout/Masonry.tsx`:
- Around line 39-44: The code in MeasuredItem treats a stored height of 0 as
"unknown" due to falsy checks; change the checks to explicitly test for
undefined/null instead of relying on truthiness. Replace `if (!currentHeight ||
Math.abs(currentHeight - newHeight) > 1)` with an explicit existence check such
as `if (currentHeight === undefined || currentHeight === null ||
Math.abs(currentHeight - newHeight) > 1)` (or `currentHeight === undefined` if
null isn't used), and replace `if (heights[itemKey])` with an explicit check
like `if (heights[itemKey] !== undefined && heights[itemKey] !== null)` (or
`heights.hasOwnProperty(itemKey)` / `heights[itemKey] !== undefined`) so a
legitimate height of 0 is treated as known.
In
`@frontend/packages/webterminal-plugin/src/components/cloud-shell/CloudShellTerminal.tsx`:
- Around line 148-157: The nested promise handlers for e.response.text() can
call setInitError after the component has unmounted; update the .then and .catch
handlers used in CloudShellTerminal so they re-check the same unmounted flag (or
an isMounted ref) before calling setInitError (and before any state updates)
inside the handlers for e.response.text(), ensuring setInitError is only invoked
when still mounted; locate the block around
e.response.text().then(...).catch(...) and add the guard check in both the then
and catch branches (or use AbortController to cancel the read) to prevent
post-unmount state updates.
---
Nitpick comments:
In
`@frontend/packages/console-app/src/hooks/useNamespacedNotificationAlertsPoller.ts`:
- Around line 18-63: The cleanup must prevent state updates from in-flight
coFetchJSON calls: in the useEffect containing poll/pollTimeout, add a mounted
guard (e.g., let isMounted = true) or use an AbortController if coFetchJSON
supports it, then check isMounted before calling
setAlerts/setLoaded/setLoadError inside the poll promise handlers (and call
controller.abort() or set isMounted = false in the returned cleanup). Update
references in this effect to use the guard (or controller) so no state setters
in poll run after unmount.
In
`@frontend/packages/dev-console/integration-tests/testData/git-import/repos.ts`:
- Line 1: Remove the file-level eslint-disable comment and instead place
targeted inline disables directly above the dynamic require expressions (the
lines that call require(`./${repo.folder}/...`)); use eslint-disable-next-line
import/no-dynamic-require, `@typescript-eslint/no-require-imports` so only those
specific require calls bypass the rules while the rest of the file stays linted.
Ensure you remove the top-of-file suppression and add the inline comments
immediately before each dynamic require usage (e.g., the require in the loop
that references repo.folder).
- Around line 49-50: Wrap the synchronous requires for repoJson and contentsJson
in a try/catch so missing or malformed JSON yields a clear, actionable error;
specifically, catch errors around the require statements that load
`./${repo.folder}/repo.json` and `./${repo.folder}/contents.json` and rethrow or
log a descriptive message that includes `repo.folder` and the missing filename
(e.g., "missing test data for folder <repo.folder>: repo.json") so failures are
easier to diagnose when `repoJson` or `contentsJson` cannot be loaded.
In `@frontend/packages/dev-console/src/components/add/layout/Masonry.tsx`:
- Around line 23-56: The effect creating the ResizeObserver is re-run whenever
currentHeight changes (because useEffect depends on currentHeight), causing
unnecessary observer recreation; to fix, keep the latest height in a ref (e.g.,
currentHeightRef) and update that ref in a separate useEffect that depends on
currentHeight, then change the ResizeObserver effect (the one using measureRef,
requestAnimationFrame/rafId, and calling onHeightMeasured(itemKey, newHeight))
to depend only on stable inputs (itemKey and onHeightMeasured) and read the
latest value from currentHeightRef inside the observer callback; preserve the
existing cancelAnimationFrame and resizeObserver.disconnect() cleanup logic in
the observer effect.
In
`@frontend/packages/webterminal-plugin/src/redux/actions/cloud-shell-actions.ts`:
- Around line 19-26: The actions object is intentionally only used via typeof to
derive CloudShellActions; to make the typing intent stronger change the
declaration of actions to be read-only by appending as const so TypeScript
preserves exact property/value types — update the const actions = {
setCloudShellExpanded, setCloudShellActive, setCloudShellCommand } to be
declared with as const (affecting the inferred type used by ActionType<typeof
actions> and referencing the symbols actions, setCloudShellExpanded,
setCloudShellActive, setCloudShellCommand, and CloudShellActions).
In `@frontend/public/components/app.tsx`:
- Around line 502-520: The load-test service-worker promise chain (starting from
the dynamic import of 'file-loader?name=load-test.sw.js!../load-test.sw.js'
through navigator.serviceWorker.register, the controller-ready Promise, and the
postMessage call) lacks terminal error handling; append a .catch handler to that
chain to catch any rejection and handle it (e.g., console.error or a dedicated
logger and optionally set a fallback flag/UI), so failures to import, register,
wait for controller, or postMessage are logged and do not rely solely on global
unhandled rejection handling.
🪄 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: a552f173-5c11-4bd6-b6f6-3dfbe70ff966
⛔ Files ignored due to path filters (1)
frontend/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (35)
frontend/.eslintignorefrontend/.eslintrc.jsfrontend/i18n-scripts/.eslintrc.jsfrontend/packages/console-app/src/components/volume-snapshot/create-volume-snapshot/create-volume-snapshot.tsxfrontend/packages/console-app/src/hooks/useNamespacedNotificationAlertsPoller.tsfrontend/packages/console-dynamic-plugin-sdk/src/app/core/actions/core.tsfrontend/packages/console-dynamic-plugin-sdk/src/app/k8s/actions/k8s.tsfrontend/packages/console-dynamic-plugin-sdk/src/webpack/ConsoleRemotePlugin.tsfrontend/packages/console-telemetry-plugin/integration-tests/.eslintrcfrontend/packages/dev-console/integration-tests/.eslintrcfrontend/packages/dev-console/integration-tests/testData/git-import/repos.tsfrontend/packages/dev-console/src/components/add/layout/Masonry.tsxfrontend/packages/eslint-plugin-console/lib/config/node.jsfrontend/packages/eslint-plugin-console/lib/config/prettier.jsfrontend/packages/eslint-plugin-console/lib/config/rules/airbnb-base-overrides.jsfrontend/packages/eslint-plugin-console/lib/config/rules/node.jsfrontend/packages/eslint-plugin-console/lib/config/rules/prettier-typescript.jsfrontend/packages/eslint-plugin-console/lib/config/rules/react.jsfrontend/packages/eslint-plugin-console/lib/config/rules/typescript.jsfrontend/packages/eslint-plugin-console/package.jsonfrontend/packages/helm-plugin/integration-tests/.eslintrcfrontend/packages/integration-tests/.eslintrcfrontend/packages/knative-plugin/integration-tests/.eslintrcfrontend/packages/shipwright-plugin/integration-tests/.eslintrcfrontend/packages/topology/src/redux/action.tsfrontend/packages/webterminal-plugin/integration-tests/.eslintrcfrontend/packages/webterminal-plugin/src/components/cloud-shell/CloudShellTerminal.tsxfrontend/packages/webterminal-plugin/src/redux/actions/cloud-shell-actions.tsfrontend/public/actions/dashboards.tsfrontend/public/actions/flags.tsfrontend/public/actions/k8s.tsfrontend/public/actions/observe.tsfrontend/public/actions/ui.tsfrontend/public/components/app.tsxfrontend/webpack.config.ts
💤 Files with no reviewable changes (1)
- frontend/packages/eslint-plugin-console/lib/config/rules/prettier-typescript.js
📜 Review details
🔇 Additional comments (23)
frontend/packages/console-app/src/hooks/useNamespacedNotificationAlertsPoller.ts (2)
2-2: LGTM: useimport typeforNotificationAlerts.Since
NotificationAlertsis only used for typing, the type-only import avoids unintended runtime/module loading and matches the intended ESLint v8/@typescript-eslint behavior.
14-16: Possible TS typing mismatch forloadErrorand non-portable timer type.
- Line 16 initializes
loadErrorwithnull, butNotificationAlerts['loadError']is derived from an optional type (loadError?: Error), which is typicallyError | undefined(notnull). UnderstrictNullChecks, this can be a type error or force unsafe casts.- Line 26 uses
NodeJS.Timeout, which is often awkward in frontend TS projects;ReturnType<typeof setTimeout>is usually the safer, environment-agnostic type.Consider switching initial state to
undefinedand usingReturnType<typeof setTimeout> | undefinedforpollTimeout.Also applies to: 26-26
frontend/packages/dev-console/src/components/add/layout/Masonry.tsx (1)
118-125: Looks good: targeted eslint-disable for stable column bucket keys.The added
eslint-disable-next-line react/no-array-index-keyis appropriately narrow, and usingcolumnIndexas the column “bucket” key is reasonable here since the bucket positions are determined bycolumnCountand only the items inside migrate as measurements arrive.frontend/public/components/app.tsx (1)
502-505: Good: eslint-disable-now-belongs-above-the-dynamic-import.Placing
// eslint-disable-next-line import/no-unresolvedimmediately above theimport('file-loader?...')call (and after the//@ts-expect-error...line) ensures the suppression applies to the actual unresolved “file-loader query” statement rather than accidentally targeting the wrong line. This matches the intent of the ESLint v8 migration cleanup.frontend/packages/dev-console/integration-tests/testData/git-import/repos.ts (1)
48-77: Caller enforces the repository whitelist—path traversal concern is already mitigated.
getResponseMocks()is only called fromgit-page.tswhererepositoryis obtained viagitImportRepos.find((repo) => repo.url === shortUrl)(line 19). Both invocations (lines 27 and 58) occur withinif (repository)blocks, guaranteeing thatrepo.folderalways originates from the hardcoded allow-list. The constraint is enforced at the call site, not within the function itself, so the dynamicrequire()paths are safe.> Likely an incorrect or invalid review comment.frontend/packages/console-app/src/components/volume-snapshot/create-volume-snapshot/create-volume-snapshot.tsx (1)
281-283: Good accessibility fix.Using
htmlForhere restores the label/control association in JSX, which improves screen-reader behavior and click targeting.frontend/packages/eslint-plugin-console/package.json (1)
12-31: Consider pinning exact versions for this publishable package.This package lacks a
"private": trueflag, marking it as publishable. While the frontendyarn.lockcurrently pins all dependencies to exact versions (@typescript-eslint/eslint-plugin→ 8.59.0,eslint→ 8.57.1, etc.), the caret ranges inpackage.jsonmean anyone installingeslint-plugin-consoleindependently would get flexible resolution. ESLint rule behavior can shift within major versions, risking the zero-warning policy you've maintained. Update the key entries to exact versions (e.g.,"eslint": "8.57.1"instead of"^8.57.0") to ensure lint-rule stability across installations.frontend/packages/knative-plugin/integration-tests/.eslintrc (1)
1-16: LGTM — Cypress unsafe-to-chain-command disabled for integration tests.This matches the intended pattern in the PR: keep the existing Cypress/promise overrides and additionally disable
cypress/unsafe-to-chain-commandto avoid new v8-lint failures in integration tests.frontend/public/actions/dashboards.ts (1)
35-43: LGTM — narrow unused-vars suppression for type-onlydashboardsActions.The
eslint-disable-next-line@typescript-eslint/no-unused-vars`` comment is precisely placed to coverdashboardsActionsbeing referenced only through `typeof` in the exported `DashboardsAction` type.frontend/public/actions/k8s.ts (1)
104-110: LGTM — narrow unused-vars suppression for type-onlyk8sActions.This is an appropriately scoped suppression for a constant that’s only referenced via
typeofin the exportedK8sActiontype.frontend/webpack.config.ts (1)
105-114: LGTM — lint suppression aligned with require-imports rule.Updating the
eslint-disable-next-lineto@typescript-eslint/no-require-importskeeps the configuration intent while matching the new eslint/@typescript-eslint rule naming.frontend/packages/shipwright-plugin/integration-tests/.eslintrc (1)
1-17: LGTM — Cypress unsafe-to-chain-command disabled for Shipwright integration tests.frontend/packages/console-dynamic-plugin-sdk/src/app/core/actions/core.ts (1)
31-42: LGTM — narrow unused-vars suppression for type-onlycoreActions.frontend/packages/webterminal-plugin/integration-tests/.eslintrc (1)
1-18: LGTM — Cypress unsafe-to-chain-command disabled for Webterminal integration tests.frontend/packages/integration-tests/.eslintrc (1)
1-17: LGTM — Cypress unsafe-to-chain-command disabled for general integration tests.frontend/packages/helm-plugin/integration-tests/.eslintrc (1)
15-16: LGTM!Disabling
cypress/unsafe-to-chain-commandin integration tests is reasonable — test code often chains commands intentionally for setup/teardown flows. This aligns with the same pattern applied across other integration-tests configurations in the repo.frontend/packages/eslint-plugin-console/lib/config/node.js (1)
13-13: LGTM!Migration from
plugins: ['node']toplugins: ['n']correctly aligns with the eslint-plugin-n package rename. The referenced rules file usesn/prefix consistently, so config consumers will receive the updated rule definitions.frontend/i18n-scripts/.eslintrc.js (1)
6-7: LGTM!Rule key rename from
node/no-unsupported-features/node-builtinston/no-unsupported-features/node-builtinsaligns with the eslint-plugin-n migration.frontend/packages/console-dynamic-plugin-sdk/src/webpack/ConsoleRemotePlugin.ts (1)
30-32: LGTM!Correctly updates the ESLint suppression from
@typescript-eslint/no-var-requiresto@typescript-eslint/no-require-imports— the rule was split in@typescript-eslintv8. The dynamicrequire()is intentional for build-time package resolution.frontend/packages/console-telemetry-plugin/integration-tests/.eslintrc (1)
14-16: LGTM!Consistent with other integration-tests configs — disabling
cypress/unsafe-to-chain-commandfor test code that may legitimately chain commands.frontend/packages/topology/src/redux/action.ts (1)
30-37: LGTM!The
actionsobject is intentionally used for type derivation viatypeof. Adding@typescript-eslint/no-unused-varssuppression is correct — the patternActionType<typeof actions>is a common TypeScript technique for extracting action type unions without runtime usage.frontend/packages/dev-console/integration-tests/.eslintrc (1)
15-16: LGTM!Consistent with other integration-tests configs. Disabling
cypress/unsafe-to-chain-commandfor test code flexibility.frontend/packages/eslint-plugin-console/lib/config/prettier.js (1)
1-18: LGTM!Removal of
prettier/reactand deletion ofprettier-typescript.jsaligns with modern Prettier setups where formatting rules are handled directly by Prettier without needing conflicting ESLint rule disables. The inlineprettier/prettier: 'error'in overrides ensures TypeScript files still enforce formatting.
| // Disallow named default exports (noisy with TypeScript re-exports) | ||
| 'import/no-named-as-default-member': 'off', | ||
|
|
||
| // Prefer arrow functions as callbacks (new in airbnb v15) | ||
| 'prefer-arrow-callback': 'off', | ||
|
|
||
| // Disallow returning values from Promise executor (new in eslint:recommended v8) | ||
| 'no-promise-executor-return': 'off', | ||
|
|
||
| // Require arrow function bodies to use braces (new in airbnb v15) | ||
| 'arrow-body-style': 'off', | ||
|
|
||
| // Ensure named imports match exported names (too many false positives with TypeScript) | ||
| 'import/named': 'off', |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that JS/JSX files are still part of the frontend lint surface.
fd -e js -e jsx . frontend | sed -n '1,40p'
printf '\n--- import rule disables in shared configs ---\n'
rg -n -C2 "import/no-named-as-default-member|import/named|airbnb-base-overrides" frontend/packages/eslint-plugin-console frontend/.eslintrc.jsRepository: openshift/console
Length of output: 4932
🏁 Script executed:
#!/bin/bash
# Understand the full config structure and how they cascade
echo "=== Structure of eslint-plugin-console configs ==="
fd --type f . frontend/packages/eslint-plugin-console/lib/config | sort
echo -e "\n=== Content of base.js (what it extends/requires) ==="
cat -n frontend/packages/eslint-plugin-console/lib/config/base.js | head -25
echo -e "\n=== Content of react.js (what it extends/requires) ==="
cat -n frontend/packages/eslint-plugin-console/lib/config/react.js | head -35
echo -e "\n=== Content of typescript.js (if it exists) ==="
cat -n frontend/packages/eslint-plugin-console/lib/config/typescript.js 2>/dev/null | head -40
echo -e "\n=== Root .eslintrc.js structure ==="
head -150 frontend/.eslintrc.js | tail -60Repository: openshift/console
Length of output: 6210
🏁 Script executed:
#!/bin/bash
# Check the full root .eslintrc.js structure, especially overrides section
echo "=== Full root .eslintrc.js (first 250 lines) ==="
head -250 frontend/.eslintrc.js
echo -e "\n=== Check for overrides section in root .eslintrc ==="
grep -n "overrides" frontend/.eslintrc.js | head -10
echo -e "\n=== Full airbnb-base-overrides.js ==="
cat -n frontend/packages/eslint-plugin-console/lib/config/rules/airbnb-base-overrides.jsRepository: openshift/console
Length of output: 8957
Consolidate import-rule relaxations to avoid duplication—and address the root cause of false positives.
The import/named and import/no-named-as-default-member rules are disabled here and again in frontend/.eslintrc.js (lines 113–114), creating redundancy. More importantly, the root .eslintrc.js uses @typescript-eslint/parser as the default parser for all files, which likely explains why these import rules trigger false positives even in plain JavaScript—the parser was designed for TypeScript.
Either:
- Remove these rules from
airbnb-base-overrides.js(they're already off in root config), or - Restructure the root config to use file-type-specific parsers (TypeScript parser only for
*.ts/*.tsx, leaving plain JS/JSX with the default parser). This would then allow proper TS-only overrides.
The current approach disables useful import validation across all modules without addressing the parser mismatch that causes the issue.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@frontend/packages/eslint-plugin-console/lib/config/rules/airbnb-base-overrides.js`
around lines 95 - 108, The airbnb override is redundantly disabling the import
rules 'import/named' and 'import/no-named-as-default-member' and the true cause
is the TypeScript parser applied globally; remove these two rule entries from
the airbnb-base-overrides module and instead scope parser usage to TypeScript
files in the root ESLint config by adding an "overrides" block that sets parser:
'@typescript-eslint/parser' for files matching *.ts and *.tsx (so plain JS uses
the default parser), or conversely remove them only from the root config if you
prefer keeping JS-only behavior in this override; locate the disabled rules by
name ('import/named', 'import/no-named-as-default-member') in the override file
and update the root ESLint config's overrides section to apply the TypeScript
parser only to TS file globs.
| // Disable prop-types related rules (TypeScript handles this) | ||
| 'react/no-unused-prop-types': 'off', | ||
| 'react/forbid-prop-types': 'off', | ||
|
|
||
| /* ---- Rules new/changed in eslint-config-airbnb v19 / eslint-plugin-react v7.37 ---- */ | ||
|
|
||
| // Enforce function component definition style (new in airbnb v19) | ||
| 'react/function-component-definition': 'off', | ||
|
|
||
| // Require defaultProps for non-required props (TypeScript handles this) | ||
| 'react/require-default-props': 'off', |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that plain JS/JSX files still exist under frontend and would inherit the shared React config.
fd -e js -e jsx . frontend | sed -n '1,40p'
printf '\n--- matching rule definitions / references ---\n'
rg -n -C2 "react/(no-unused-prop-types|forbid-prop-types|require-default-props)|rules/react" frontend/packages/eslint-plugin-console frontend/.eslintrc.jsRepository: openshift/console
Length of output: 4015
🏁 Script executed:
#!/bin/bash
# Check the main react config structure and how rules/react.js is used
cat -n frontend/packages/eslint-plugin-console/lib/config/react.js
echo -e '\n--- Check if there are overrides for TypeScript files ---'
cat -n frontend/packages/eslint-plugin-console/lib/config/typescript.js | head -60
echo -e '\n--- Check the index.js to understand how configs are exported ---'
cat -n frontend/packages/eslint-plugin-console/index.jsRepository: openshift/console
Length of output: 5726
🏁 Script executed:
#!/bin/bash
# Look for root-level eslintrc files
fd -name '.eslintrc*' -o -name 'eslint.config.*' | head -10
# Check if there's a pattern of how configs are applied
echo -e '\n--- Root-level config if present ---'
if [ -f .eslintrc.js ]; then
cat -n .eslintrc.js | head -50
elif [ -f .eslintrc.json ]; then
cat -n .eslintrc.json | head -50
fiRepository: openshift/console
Length of output: 348
🏁 Script executed:
#!/bin/bash
# Find eslintrc files at root level
find . -maxdepth 2 -name '.eslintrc*' -o -name 'eslint.config.*' 2>/dev/null | grep -v node_modules
# Check if there's a root .eslintrc
echo -e '\n--- Root eslintrc.js if present ---'
if [ -f .eslintrc.js ]; then
head -80 .eslintrc.js
fi
echo -e '\n--- Frontend eslintrc if present ---'
if [ -f frontend/.eslintrc.js ]; then
head -80 frontend/.eslintrc.js
fiRepository: openshift/console
Length of output: 2599
🏁 Script executed:
#!/bin/bash
# Search for all references to rules/react.js
rg -r "rules/react" frontend/packages/eslint-plugin-console/ --type js
echo -e "\n--- Check if lib/config/react.js is actually used ---"
rg "plugin:console/react" . --type js --type json 2>/dev/null | head -20Repository: openshift/console
Length of output: 289
Move TypeScript-justified prop-rules disables into the .tsx override.
These three rules are disabled globally in the base rules/react.js, but justified with comments about TypeScript handling. The existing overrides section in lib/config/react.js already targets .tsx files—move react/no-unused-prop-types, react/forbid-prop-types, and react/require-default-props there alongside the other TypeScript-specific disables. This preserves propTypes validation for .js/.jsx files while keeping the TypeScript relaxations scoped appropriately.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/packages/eslint-plugin-console/lib/config/rules/react.js` around
lines 48 - 58, Move the TypeScript-justified rule disables out of the global
rules block and into the existing .tsx override: remove
'react/no-unused-prop-types', 'react/forbid-prop-types', and
'react/require-default-props' from the base rules in lib/config/rules/react.js
and add them (with the same 'off' values and comments) to the overrides section
that targets .tsx files (where other TypeScript-specific disables live),
ensuring propTypes rules remain active for .js/.jsx while being relaxed for
.tsx.
| { | ||
| target: './', | ||
| from: `node_modules/@patternfly/${pkg}/dist/js`, | ||
| from: 'node_modules/@patternfly/*/dist/js', | ||
| message: 'Import from the package index instead of the CJS dist/js path.', | ||
| })) | ||
| .concat( | ||
| ['react-component-groups', 'react-data-view', 'react-user-feedback'].map((pkg) => ({ | ||
| target: './', | ||
| from: `node_modules/@patternfly/${pkg}/dist/cjs`, | ||
| message: 'Import from the package index instead of the CJS dist/cjs path.', | ||
| })), | ||
| ), | ||
| }, | ||
| { | ||
| target: './', | ||
| from: 'node_modules/@patternfly/*/dist/cjs', | ||
| message: 'Import from the package index instead of the CJS dist/cjs path.', |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's examine the actual file and its context
cat -n frontend/packages/eslint-plugin-console/lib/config/rules/typescript.js | sed -n '215,235p'Repository: openshift/console
Length of output: 778
🏁 Script executed (no clone):
#!/bin/bash
# Check eslint-plugin-import documentation on import/no-restricted-paths
curl -s https://raw.githubusercontent.com/import-js/eslint-plugin-import/main/docs/rules/no-restricted-paths.md | head -200
Length of output: 4468
🏁 Script executed:
#!/bin/bash
# Let's search for any existing patterns in the codebase using glob patterns to understand the expected behavior
rg "from:.*\*\*" frontend/packages/eslint-plugin-console --type js -A 2 -B 2Repository: openshift/console
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Search for PatternFly imports to see what patterns are being used
rg "from\s+['\"]@patternfly" frontend/packages/ -A 1 -B 1 | head -80Repository: openshift/console
Length of output: 7838
🏁 Script executed:
#!/bin/bash
# Also check for any imports from the dist/js or dist/cjs paths that should be caught
rg "from\s+['\"].*/@patternfly.*dist/(js|cjs)" frontend/packages/ -A 1 -B 1Repository: openshift/console
Length of output: 43
Add /**/* to glob patterns to match files within the dist directories.
The from patterns use glob syntax (due to *), and import/no-restricted-paths requires explicit /**/* for globs to match files recursively within a directory. Without it, the rule would only catch direct imports from the dist/js and dist/cjs directories themselves, not nested files. Per the eslint-plugin-import docs, glob patterns need explicit recursion.
Suggested fix
{
target: './',
- from: 'node_modules/@patternfly/*/dist/js',
+ from: 'node_modules/@patternfly/*/dist/js/**/*',
message: 'Import from the package index instead of the CJS dist/js path.',
},
{
target: './',
- from: 'node_modules/@patternfly/*/dist/cjs',
+ from: 'node_modules/@patternfly/*/dist/cjs/**/*',
message: 'Import from the package index instead of the CJS dist/cjs path.',
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/packages/eslint-plugin-console/lib/config/rules/typescript.js`
around lines 222 - 230, Update the restricted-paths entries in
frontend/packages/eslint-plugin-console/lib/config/rules/typescript.js so the
glob patterns match files recursively: change the offending "from" strings
'node_modules/@patternfly/*/dist/js' and 'node_modules/@patternfly/*/dist/cjs'
to include '/**/*' (e.g. 'node_modules/@patternfly/*/dist/js/**/*' and
'node_modules/@patternfly/*/dist/cjs/**/*') used in the objects with message
"Import from the package index instead of the CJS dist/js path." and the similar
CJS message so the import/no-restricted-paths rule catches nested files.
|
/lgtm |
Upgrades the frontend linting infrastructure from ESLint 6.7.2 (released 2019) to ESLint 8.57.x, and @typescript-eslint from v5 to v8 to support TypeScript 5.9.3 without version mismatch warnings. Dependency version changes in eslint-plugin-console/package.json: - eslint: 6.7.2 -> ^8.57.0 - @typescript-eslint/parser: 5.62.0 -> ^8.0.0 - @typescript-eslint/eslint-plugin: 5.62.0 -> ^8.0.0 - eslint-config-airbnb: 18.0.1 -> 19.0.4 - eslint-config-airbnb-base: 14.0.0 -> 15.0.0 - eslint-config-prettier: 6.7.0 -> ^8.10.0 - eslint-plugin-prettier: 3.1.4 -> ^4.2.1 - eslint-plugin-cypress: 2.11.3 -> ^3.6.0 - eslint-plugin-import: ^2.22.1 -> ^2.31.0 - eslint-plugin-jest: 23.1.1 -> ^27.9.0 - eslint-plugin-jsx-a11y: 6.2.3 -> ^6.8.0 - eslint-plugin-node: 10.0.0 -> eslint-plugin-n ^17.24.0 - eslint-plugin-promise: 4.2.1 -> ^6.6.0 - eslint-plugin-react: 7.17.0 -> ^7.37.0 - eslint-plugin-react-hooks: 4.1.2 -> ^4.6.0 - eslint-plugin-sort-class-members: 1.6.0 -> ^1.20.0 - eslint-plugin-tsdoc: ^0.2.14 -> ^0.4.0 - eslint-import-resolver-typescript: 2.0.0 -> ^3.7.0 Breaking changes addressed: eslint-config-prettier v8: - Removed 'prettier/react' from extends (merged into main 'prettier' config in v8). All framework-specific configs like prettier/react and prettier/@typescript-eslint were consolidated. @typescript-eslint v6/v7/v8 rule renames and removals: - ban-types was split into no-restricted-types, no-empty-object-type, and no-unsafe-function-type. Our old config allowed {}, Function, and object (set to false), so the new split rules are disabled to preserve that behavior. - Removed deleted rules: interface-name-prefix, prefer-interface, no-type-alias, no-parameter-properties, no-var-requires (merged into no-require-imports), no-empty-interface (renamed no-empty-object-type). - Removed all formatting rules dropped in v6 (handled by prettier): func-call-spacing, indent, member-delimiter-style, no-extra-parens, semi, type-annotation-spacing. Deleted prettier-typescript.js which existed solely to disable these. - Added 'import' selector to naming-convention (v6 added a separate import selector; previously imports matched the variable selector). - Added caughtErrors: 'none' to no-unused-vars (v8 changed default from 'none' to 'all', flagging unused catch parameters). - Added eslint-disable comments on 10 Redux action files where variables are defined solely for typeof usage in type exports (v8 now flags variables used only in type position as unused). eslint-plugin-node -> eslint-plugin-n: - Replaced the unmaintained eslint-plugin-node (10.0.0) with its actively maintained fork eslint-plugin-n (^17.24.0). - Renamed all rule prefixes from 'node/' to 'n/' across config files and i18n-scripts/.eslintrc.js. - Restored 8 Node.js/CommonJS rules that were removed from ESLint core in v7 and were unavailable in eslint-plugin-node but exist in eslint-plugin-n: n/callback-return, n/global-require, n/handle-callback-err, n/no-mixed-requires, n/no-new-require, n/no-path-concat, n/no-process-exit, n/no-sync. - Renamed node/shebang to n/hashbang (renamed in eslint-plugin-n). ESLint v7/v8 core changes: - Fixed eqeqeq option from deprecated 'allow-null' to { null: 'ignore' } (old syntax removed in ESLint v8). - Removed no-global-strict rule (removed from ESLint long ago). - Changed tsconfigRootDir from './' to __dirname (@typescript-eslint v8 requires an absolute path). - Simplified import/no-restricted-paths for @patternfly CJS imports to use glob patterns (now supported by eslint-plugin-import ^2.31.0) instead of manually enumerating each package. New rules disabled to preserve zero-warning policy (--max-warnings 0): These rules are new in eslint-config-airbnb v19, ESLint v8 recommended, or updated plugin versions. They are disabled for now and can be enabled in follow-up PRs: - From airbnb v19: react/function-component-definition, react/require-default-props, react/jsx-no-useless-fragment, react/no-unstable-nested-components, react/jsx-no-constructed-context-values, react/no-unused-prop-types, react/forbid-prop-types, arrow-body-style, prefer-arrow-callback, default-param-last - From ESLint v8 recommended: no-unsafe-optional-chaining, no-import-assign, no-constructor-return, prefer-regex-literals, no-restricted-exports, no-promise-executor-return - From updated plugins: cypress/unsafe-to-chain-command (v3), import/no-cycle, import/no-relative-packages, import/no-named-as-default-member, import/named, global-require Other code fixes: - Updated eslint-disable comments referencing removed rule @typescript-eslint/no-var-requires to no-require-imports. - Fixed html-for -> htmlFor in create-volume-snapshot.tsx. - Fixed import ordering in useNamespacedNotificationAlertsPoller.ts. - Fixed eslint-disable-next-line placement in app.tsx (was two lines above the violation instead of directly above). - Added eslint-disable block for promise/no-nesting in CloudShellTerminal.tsx (plugin v6 reports each chain method separately, old single-line disable was insufficient). - Added eslint-disable for react/no-array-index-key in Masonry.tsx. - Added .eslintrc.js and tsconfig.json to .eslintignore (not lintable source files; @typescript-eslint v8 errors on files outside tsconfig). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
No runtime changes. Verified changes to PatternFly CJS rule /verified by CI |
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. |
|
Verified main scripts
Verified following cases via
/verified by @vojtechszocs |
|
/lgtm |
|
@vojtechszocs: 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, 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 |
|
@logonoff: This pull request references CONSOLE-5227 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. |
|
@logonoff: This pull request references CONSOLE-5227 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. |
|
@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. |
4 participants
Analysis / Root cause:
The frontend linting infrastructure is on ESLint 6.7.2 (released 2019), which is four major versions behind and EOL. The
@typescript-eslintpackages are on v5, which produces version mismatch warnings with TypeScript 5.9.3. Several plugins (e.g.,eslint-plugin-node) are unmaintained, and their actively maintained forks cannot be adopted without upgrading the ESLint major version.Solution description:
Upgrades ESLint from 6.7.2 to 8.57.x and
@typescript-eslintfrom v5 to v8, along with all ESLint plugins and configs to compatible versions. Key changes:eslint-plugin-node->eslint-plugin-n: Replaced the unmaintained plugin with its actively maintained fork. Renamed allnode/rule prefixes ton/across config files. Restored 8 Node.js/CommonJS rules that were removed from ESLint core in v7.@typescript-eslintv8 rule renames:ban-typeswas split intono-restricted-types,no-empty-object-type, andno-unsafe-function-type. Removed deleted rules (interface-name-prefix,prefer-interface,no-type-alias,no-parameter-properties,no-var-requires,no-empty-interface). Removed all formatting rules dropped in v6 (handled by Prettier). Addedimportselector tonaming-convention. AddedcaughtErrors: 'none'tono-unused-vars(v8 changed default from'none'to'all').eslint-config-prettierv8: Removedprettier/reactfrom extends (merged into mainprettierconfig in v8). Deletedprettier-typescript.jswhich existed solely to disable formatting rules already removed in@typescript-eslintv6.eslint-config-airbnbv19: Disabled new rules to preserve the zero-warning policy (--max-warnings 0). These can be enabled in follow-up PRs.eqeqeqoption from deprecated'allow-null'to{ null: 'ignore' }. ChangedtsconfigRootDirfrom'./'to__dirname. Simplifiedimport/no-restricted-pathsfor@patternflyCJS imports to use glob patterns.eslint-disablecomments on 10 Redux action files where variables are defined solely fortypeofusage in type exports (v8 flags these as unused). Fixedhtml-for->htmlFor, import ordering, and eslint-disable placement issues.Screenshots / screen recording:
N/A — No visual changes. This is a developer tooling upgrade.
Test setup:
Test cases:
yarn lintpasses with zero warnings and zero errors across the entire frontend codebaseyarn buildcompletes successfully (no build regressions from config changes)yarn testpasses (no test regressions from eslint-disable comment changes)eslint-plugin-nrules correctly replaceeslint-plugin-noderules in CI.eslintrc.js(node/->n/prefix)Browser conformance:
N/A — No runtime changes. Linting tooling only.
Additional info:
Several rules from airbnb v19, ESLint v8 recommended, and updated plugins were disabled to preserve the zero-warning policy. These can be enabled individually in follow-up PRs:
react/function-component-definition,react/require-default-props,react/jsx-no-useless-fragment,react/no-unstable-nested-components,react/jsx-no-constructed-context-values,react/no-unused-prop-types,react/forbid-prop-types,arrow-body-style,prefer-arrow-callback,default-param-lastno-unsafe-optional-chaining,no-import-assign,no-constructor-return,prefer-regex-literals,no-restricted-exports,no-promise-executor-returncypress/unsafe-to-chain-command(v3),import/no-cycle,import/no-relative-packages,import/no-named-as-default-member,import/named,global-requireReviewers and assignees:
/assign @jhadvig @vojtechszocs
Summary by CodeRabbit
Release Notes
Bug Fixes
Chores