OCPBUGS-76554: Ban CJS imports of PF react-icons #16023
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@logonoff: This pull request references Jira Issue OCPBUGS-76554, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
openshift-ci
bot
added
component/core
|
@logonoff: This pull request references Jira Issue OCPBUGS-76554, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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 updates ESLint configurations and icon import paths across the frontend. It adds a new 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 5
🤖 Fix all issues with AI agents
In `@frontend/packages/console-telemetry-plugin/integration-tests/.eslintrc`:
- Line 6: The ESLint extends entry is pointing to "../../../.eslintrc" which is
incorrect; update the extends array value to "../../../.eslintrc.js" so the base
config resolves (modify the "extends" property in the .eslintrc file to use
"../../../.eslintrc.js" instead of "../../../.eslintrc"). Apply the same change
to other integration-tests .eslintrc files (dev-console, helm-plugin,
knative-plugin, shipwright-plugin, webterminal-plugin) to ensure CI linting
works consistently.
In `@frontend/packages/dev-console/integration-tests/.eslintrc`:
- Line 6: Update the extends entry in the ESLint config: replace the incorrect
"../../../.eslintrc" string in the "extends" array with the correct
"../../../.eslintrc.js" so the root config (frontend/.eslintrc.js) is loaded;
modify the value inside the "extends" array in the .eslintrc file located in the
integration-tests package to reference the .js extension.
In `@frontend/packages/dev-console/src/components/import/git/GitTypeSelector.tsx`:
- Line 4: The import statement in GitTypeSelector.tsx currently pulls icons from
'@patternfly/react-icons'; update it to use the explicit ESM path used elsewhere
by importing GithubIcon, GitlabIcon, BitbucketIcon, and GitAltIcon from
'@patternfly/react-icons/dist/esm/icons' so the module resolution matches the
rest of the dev-console package (see the import of these same icons in
render-utils.tsx).
In `@frontend/packages/helm-plugin/integration-tests/.eslintrc`:
- Line 6: Update the ESLint extends to point to the actual JS config file:
replace the extends entry "../../../.eslintrc" with "../../../.eslintrc.js" in
the extends array (the "extends" key in the .eslintrc used by the
integration-tests package), and apply the same change to the equivalent
integration-test package configs (dev-console, console-telemetry,
knative-plugin, topology, webterminal-plugin, shipwright-plugin) so they all
reference .eslintrc.js instead of .eslintrc.
In `@frontend/packages/webterminal-plugin/integration-tests/.eslintrc`:
- Line 6: The extends entry in the .eslintrc currently points to a non-existent
"../../../.eslintrc"; update the extends array item value to
"../../../.eslintrc.js" so it references the actual root config (change the
string value that appears in the extends array), and apply the same change to
the other six integration-tests .eslintrc files that use "../../../.eslintrc".
| "node": true | ||
| }, | ||
| "extends": ["../../.eslintrc", "plugin:cypress/recommended", "plugin:prettier/recommended"], | ||
| "extends": ["../../../.eslintrc", "plugin:cypress/recommended", "plugin:prettier/recommended"], |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the target ESLint config file exists
if test -f frontend/.eslintrc; then
echo "✓ frontend/.eslintrc exists"
echo "--- First 10 lines of frontend/.eslintrc ---"
head -n 10 frontend/.eslintrc
else
echo "✗ frontend/.eslintrc does NOT exist"
fi
# Also verify the relative path from the integration-tests directory
echo ""
echo "--- Path resolution check ---"
cd frontend/packages/console-telemetry-plugin/integration-tests 2>/dev/null && \
if test -f ../../../.eslintrc; then
echo "✓ ../../../.eslintrc resolves correctly from integration-tests/"
else
echo "✗ ../../../.eslintrc does NOT resolve from integration-tests/"
fi || echo "Could not verify relative path"Repository: openshift/console
Length of output: 189
🏁 Script executed:
#!/bin/bash
# Find all .eslintrc files in the repository
echo "=== All .eslintrc files ==="
find . -name ".eslintrc*" -type f | head -20
echo ""
echo "=== ESLint configs in console root ==="
ls -la | grep eslint
echo ""
echo "=== ESLint configs in frontend directory ==="
test -d frontend && ls -la frontend/ | grep eslintRepository: openshift/console
Length of output: 1188
🏁 Script executed:
#!/bin/bash
echo "=== Integration-tests .eslintrc files and their extends paths ==="
for file in $(find ./frontend/packages -path "*/integration-tests/.eslintrc" -type f); do
echo ""
echo "File: $file"
echo "Extends line:"
grep -n "extends" "$file" | head -3
done
echo ""
echo "=== Verify correct relative path from telemetry integration-tests ==="
cd frontend/packages/console-telemetry-plugin/integration-tests 2>/dev/null && \
if test -f ../../../.eslintrc.js; then
echo "✓ ../../../.eslintrc.js exists and is reachable"
else
echo "✗ ../../../.eslintrc.js NOT reachable"
fiRepository: openshift/console
Length of output: 1438
Update the ESLint extends path to use .eslintrc.js instead of .eslintrc.
The base config is frontend/.eslintrc.js, not .eslintrc. The current path reference will break ESLint resolution in CI and prevent linting from running. Change line 6 from:
"extends": ["../../../.eslintrc", ...
to:
"extends": ["../../../.eslintrc.js", ...
This issue affects multiple integration-tests directories across the codebase (dev-console, helm-plugin, knative-plugin, shipwright-plugin, and webterminal-plugin), so the same fix should be applied consistently.
🤖 Prompt for AI Agents
In `@frontend/packages/console-telemetry-plugin/integration-tests/.eslintrc` at
line 6, The ESLint extends entry is pointing to "../../../.eslintrc" which is
incorrect; update the extends array value to "../../../.eslintrc.js" so the base
config resolves (modify the "extends" property in the .eslintrc file to use
"../../../.eslintrc.js" instead of "../../../.eslintrc"). Apply the same change
to other integration-tests .eslintrc files (dev-console, helm-plugin,
knative-plugin, shipwright-plugin, webterminal-plugin) to ensure CI linting
works consistently.
| "node": true | ||
| }, | ||
| "extends": ["../../.eslintrc", "plugin:cypress/recommended", "plugin:prettier/recommended"], | ||
| "extends": ["../../../.eslintrc", "plugin:cypress/recommended", "plugin:prettier/recommended"], |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the root eslint config exists where the relative path points
if [ -f "frontend/.eslintrc" ]; then
echo "=== Root ESLint config found at frontend/.eslintrc ==="
head -20 "frontend/.eslintrc"
else
echo "Root config not found at frontend/.eslintrc"
fi
# Also verify the full path from the integration-tests directory
echo ""
echo "=== Checking path resolution from integration-tests directory ==="
cd "frontend/packages/dev-console/integration-tests" && pwd
ls -la .eslintrc 2>/dev/null || echo ".eslintrc file missing in this directory"Repository: openshift/console
Length of output: 296
🏁 Script executed:
#!/bin/bash
# Find all .eslintrc files in the repository
echo "=== All .eslintrc files in repo ==="
find . -maxdepth 3 -name ".eslintrc*" -type f 2>/dev/null | head -20
echo ""
echo "=== Directory structure near frontend root ==="
ls -la frontend/ | grep -E "eslint|config"Repository: openshift/console
Length of output: 615
Fix the missing .js extension in the ESLint config path.
The extends path ../../../.eslintrc points to a non-existent file. The root config is at frontend/.eslintrc.js, so the path should be ../../../.eslintrc.js. Without this fix, ESLint will fail to load the root configuration and skip the intended linting rules.
🤖 Prompt for AI Agents
In `@frontend/packages/dev-console/integration-tests/.eslintrc` at line 6, Update
the extends entry in the ESLint config: replace the incorrect
"../../../.eslintrc" string in the "extends" array with the correct
"../../../.eslintrc.js" so the root config (frontend/.eslintrc.js) is loaded;
modify the value inside the "extends" array in the .eslintrc file located in the
integration-tests package to reference the .js extension.
| BitbucketIcon, | ||
| GitAltIcon, | ||
| } from '@patternfly/react-icons/dist/esm/icons'; | ||
| import { GithubIcon, GitlabIcon, BitbucketIcon, GitAltIcon } from '@patternfly/react-icons'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find package.json files and check for `@patternfly/react-icons` version
find . -name "package.json" -type f | head -20 | xargs grep -l "@patternfly/react-icons" 2>/dev/nullRepository: openshift/console
Length of output: 119
🏁 Script executed:
# Check the main package.json for dependencies
cat package.json | grep -A 5 -B 5 "@patternfly/react-icons" 2>/dev/null || echo "Not in root package.json"Repository: openshift/console
Length of output: 147
🏁 Script executed:
# Look for package.json in the specific package directory
cat frontend/packages/dev-console/package.json 2>/dev/null | grep -A 2 -B 2 "@patternfly/react-icons"Repository: openshift/console
Length of output: 43
🏁 Script executed:
# Find and inspect the installed `@patternfly/react-icons` package.json to check exports
find node_modules/@patternfly/react-icons -name "package.json" -type f 2>/dev/null | head -1 | xargs cat 2>/dev/null | jq '.exports // .main // .module' 2>/dev/null || echo "Package not found or no exports field"Repository: openshift/console
Length of output: 43
🏁 Script executed:
# Find the GitTypeSelector.tsx file
find . -name "GitTypeSelector.tsx" -type fRepository: openshift/console
Length of output: 138
🏁 Script executed:
# Check frontend package.json for `@patternfly/react-icons`
cat frontend/package.json | grep -A 3 -B 3 "@patternfly/react-icons" 2>/dev/null || echo "Not found in frontend/package.json"Repository: openshift/console
Length of output: 360
🏁 Script executed:
# Check if there's a monorepo config file
ls -la | grep -E "pnpm-workspace|lerna|yarn" || echo "No obvious monorepo config in root"Repository: openshift/console
Length of output: 95
🏁 Script executed:
# Search for `@patternfly/react-icons` across all package.json files
rg -l "@patternfly/react-icons" --glob "package.json"Repository: openshift/console
Length of output: 115
🏁 Script executed:
# Find build config files
find . -maxdepth 3 -type f \( -name "tsconfig.json" -o -name "webpack.config.js" -o -name "vite.config.ts" \) | head -10Repository: openshift/console
Length of output: 121
🏁 Script executed:
# Search for other imports from `@patternfly/react-icons` to see the pattern
rg "from '@patternfly/react-icons" --type ts --type tsx -c | head -20Repository: openshift/console
Length of output: 88
🏁 Script executed:
# Check the actual tsconfig that applies to this file
find frontend/packages/dev-console -name "tsconfig*.json" -type fRepository: openshift/console
Length of output: 122
🏁 Script executed:
# Look at the tsconfig to understand module resolution
cat frontend/tsconfig.json 2>/dev/null | head -50Repository: openshift/console
Length of output: 2278
🏁 Script executed:
# Search for imports from `@patternfly/react-icons`
rg "from ['\"@patternfly/react-icons" --type tsRepository: openshift/console
Length of output: 214
🏁 Script executed:
# Check if any imports use specific paths vs root
rg "from ['\"]@patternfly/react-icons" -A 0 -B 0 | sort | uniq -c | sort -rnRepository: openshift/console
Length of output: 48653
🏁 Script executed:
# Look at package.json to see if there's any build config or export info
cat frontend/package.json | head -100Repository: openshift/console
Length of output: 7394
Use consistent ESM icon import path with rest of dev-console package.
The package root import resolves to ESM in this build configuration (tsconfig.json specifies module: "esnext"), but the same icons are imported via /dist/esm/icons/ specific paths elsewhere in the package (see render-utils.tsx). Use the explicit ESM path to align with the predominant pattern across the codebase:
- import { GithubIcon, GitlabIcon, BitbucketIcon, GitAltIcon } from '@patternfly/react-icons';
+ import { GithubIcon } from '@patternfly/react-icons/dist/esm/icons/github-icon';
+ import { GitlabIcon } from '@patternfly/react-icons/dist/esm/icons/gitlab-icon';
+ import { BitbucketIcon } from '@patternfly/react-icons/dist/esm/icons/bitbucket-icon';
+ import { GitAltIcon } from '@patternfly/react-icons/dist/esm/icons/git-alt-icon';📝 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.
| import { GithubIcon, GitlabIcon, BitbucketIcon, GitAltIcon } from '@patternfly/react-icons'; | |
| import { GithubIcon } from '@patternfly/react-icons/dist/esm/icons/github-icon'; | |
| import { GitlabIcon } from '@patternfly/react-icons/dist/esm/icons/gitlab-icon'; | |
| import { BitbucketIcon } from '@patternfly/react-icons/dist/esm/icons/bitbucket-icon'; | |
| import { GitAltIcon } from '@patternfly/react-icons/dist/esm/icons/git-alt-icon'; |
🤖 Prompt for AI Agents
In `@frontend/packages/dev-console/src/components/import/git/GitTypeSelector.tsx`
at line 4, The import statement in GitTypeSelector.tsx currently pulls icons
from '@patternfly/react-icons'; update it to use the explicit ESM path used
elsewhere by importing GithubIcon, GitlabIcon, BitbucketIcon, and GitAltIcon
from '@patternfly/react-icons/dist/esm/icons' so the module resolution matches
the rest of the dev-console package (see the import of these same icons in
render-utils.tsx).
| "node": true | ||
| }, | ||
| "extends": ["../../.eslintrc", "plugin:cypress/recommended", "plugin:prettier/recommended"], | ||
| "extends": ["../../../.eslintrc", "plugin:cypress/recommended", "plugin:prettier/recommended"], |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the target file exists
if test -f frontend/.eslintrc; then
echo "✓ frontend/.eslintrc exists"
echo "Content preview:"
head -20 frontend/.eslintrc
else
echo "✗ frontend/.eslintrc NOT found"
fi
echo ""
# Verify the file that references it
if test -f frontend/packages/helm-plugin/integration-tests/.eslintrc; then
echo "✓ frontend/packages/helm-plugin/integration-tests/.eslintrc exists"
echo "Content:"
cat frontend/packages/helm-plugin/integration-tests/.eslintrc
else
echo "✗ frontend/packages/helm-plugin/integration-tests/.eslintrc NOT found"
fi
echo ""
# Show directory structure to confirm path relationships
echo "Directory structure check:"
ls -la frontend/packages/helm-plugin/integration-tests/ 2>/dev/null | head -10 || echo "Directory not found"Repository: openshift/console
Length of output: 1384
🏁 Script executed:
#!/bin/bash
# Find all .eslintrc files in the repository
echo "=== All .eslintrc files in repo ==="
find . -name ".eslintrc*" -type f 2>/dev/null
echo ""
echo "=== Checking what the path actually resolves to ==="
cd frontend/packages/helm-plugin/integration-tests/
# Test what ../../../.eslintrc would point to
echo "From integration-tests dir, ../../../.eslintrc would point to:"
readlink -f ../../../.eslintrc 2>/dev/null || echo "File does not exist at: $(pwd)/../../../.eslintrc"
echo ""
echo "Actual parent structure:"
ls -la ../../../Repository: openshift/console
Length of output: 3101
Fix ESLint config reference: use .eslintrc.js instead of .eslintrc
The extends path ../../../.eslintrc will fail to resolve. The root ESLint config is frontend/.eslintrc.js (JavaScript file), not a JSON file. Change the extends to reference ../../../.eslintrc.js.
Note: This same misconfiguration appears across other integration-test packages (dev-console, console-telemetry, knative-plugin, topology, webterminal-plugin, shipwright-plugin).
🤖 Prompt for AI Agents
In `@frontend/packages/helm-plugin/integration-tests/.eslintrc` at line 6, Update
the ESLint extends to point to the actual JS config file: replace the extends
entry "../../../.eslintrc" with "../../../.eslintrc.js" in the extends array
(the "extends" key in the .eslintrc used by the integration-tests package), and
apply the same change to the equivalent integration-test package configs
(dev-console, console-telemetry, knative-plugin, topology, webterminal-plugin,
shipwright-plugin) so they all reference .eslintrc.js instead of .eslintrc.
| "node": true | ||
| }, | ||
| "extends": ["../../.eslintrc", "plugin:cypress/recommended", "plugin:prettier/recommended"], | ||
| "extends": ["../../../.eslintrc", "plugin:cypress/recommended", "plugin:prettier/recommended"], |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the target ESLint config and understand the directory structure
# Check for .eslintrc files at various levels
echo "=== ESLint config files in the repository ==="
find . -name ".eslintrc*" -type f | head -20
# Verify the specific path from the integration-tests directory
echo ""
echo "=== Verifying path from integration-tests to root config ==="
TARGET="frontend/.eslintrc"
if [ -f "$TARGET" ]; then
echo "✓ Target config exists at: $TARGET"
echo ""
echo "=== Content of $TARGET (first 10 lines) ==="
head -10 "$TARGET"
else
echo "✗ Target config NOT found at: $TARGET"
fi
# Also check the actual integration-tests directory context
echo ""
echo "=== Directory context ==="
ls -la frontend/packages/webterminal-plugin/integration-tests/.eslintrc 2>/dev/null || echo "File not found"Repository: openshift/console
Length of output: 1240
🏁 Script executed:
#!/bin/bash
# Check what the webterminal-plugin integration-tests .eslintrc currently extends
echo "=== Current webterminal-plugin integration-tests/.eslintrc content ==="
cat frontend/packages/webterminal-plugin/integration-tests/.eslintrc
# Also check a peer integration-tests to see the pattern
echo ""
echo "=== Peer pattern: dev-console integration-tests/.eslintrc ==="
cat frontend/packages/dev-console/integration-tests/.eslintrcRepository: openshift/console
Length of output: 1240
🏁 Script executed:
#!/bin/bash
# Verify ESLint's config resolution mechanism handles .eslintrc -> .eslintrc.js
# Check how many integration-tests use the same ../../../.eslintrc pattern
echo "=== Integration-tests using ../../../.eslintrc pattern ==="
grep -r "../../../.eslintrc" frontend/packages/*/integration-tests/.eslintrc 2>/dev/null | wc -l
echo ""
echo "=== All references in integration-tests ==="
grep -h "extends" frontend/packages/*/integration-tests/.eslintrc 2>/dev/null | sort | uniq -cRepository: openshift/console
Length of output: 272
🌐 Web query:
ESLint config file resolution .eslintrc to .eslintrc.js fallback
💡 Result:
-
ESLint v9+ default (flat config): ESLint looks for
eslint.config.*(e.g.eslint.config.js/mjs/cjs) and does not automatically search for.eslintrc*anymore. To use.eslintrc*, you must opt into the legacy system (e.g.ESLINT_USE_FLAT_CONFIG=false). [1][2] -
Legacy “.eslintrc” mode (deprecated): In any given directory ESLint will load only one config file, and the precedence is:
.eslintrc.js.eslintrc.cjs.eslintrc.yaml.eslintrc.yml.eslintrc.jsonpackage.json(eslintConfig) [3]
So there is no “fallback from.eslintrcto.eslintrc.js”—if.eslintrc.jsexists, it wins; otherwise ESLint tries the next supported name in that order.
-
About plain
.eslintrc(no extension): Older ESLint docs explicitly allowed.eslintrcas a deprecated JSON/YAML format. [4] In current docs, prefer renaming it to.eslintrc.json/.eslintrc.ymlor migrating toeslint.config.*. [2][3]
Sources:
[1] https://eslint.org/docs/latest/use/configure/configuration-files
[2] https://eslint.org/docs/latest/use/migrate-to-9.0.0
[3] https://eslint.org/docs/latest/use/configure/configuration-files-deprecated
[4] https://archive.eslint.org/docs/2.0.0/user-guide/configuring
Fix the base ESLint config path—it references a non-existent file.
The extends path ../../../.eslintrc resolves to a file that doesn't exist. The actual root config is frontend/.eslintrc.js. Update to ../../../.eslintrc.js to prevent lint failures in CI. Note that 6 other integration-tests in the repo have the same issue and should be corrected as well.
🤖 Prompt for AI Agents
In `@frontend/packages/webterminal-plugin/integration-tests/.eslintrc` at line 6,
The extends entry in the .eslintrc currently points to a non-existent
"../../../.eslintrc"; update the extends array item value to
"../../../.eslintrc.js" so it references the actual root config (change the
string value that appears in the extends array), and apply the same change to
the other six integration-tests .eslintrc files that use "../../../.eslintrc".
logonoff
force-pushed
the
OCPBUGS-76554-icons
branch
from
172f5fc to
1b9822f
Compare
openshift-ci
bot
added
the
kind/cypress
logonoff
force-pushed
the
OCPBUGS-76554-icons
branch
from
1b9822f to
c3fa81a
Compare
frontend/packages/eslint-plugin-console/lib/config/rules/typescript.js
Outdated
Show resolved
Hide resolved
logonoff
force-pushed
the
OCPBUGS-76554-icons
branch
from
c3fa81a to
4cedc3b
Compare
|
/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 |
|
@logonoff: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
1.6MiB fewer bits sent down the wire due to duplicate icons in bundle:
before:
after:
Summary by CodeRabbit