CONSOLE-4979: Speed up check-patternfly-modules + add dedupe step to CI #15835
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@logonoff: This pull request references CONSOLE-4979 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/label px-approved |
openshift-ci
bot
added
px-approved
WalkthroughReplace a Bash PatternFly lockfile checker with a TypeScript implementation, add yarn-deduplicate and a dedupe script, update frontend dependencies/resolutions (PatternFly and typings), add a dedupe pre-check to the frontend test script, and update README dedupe instructions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used📓 Path-based instructions (1)**⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (7)
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: 0
🧹 Nitpick comments (2)
frontend/scripts/check-patternfly-modules.ts (1)
75-82: Consider guarding againstsemver.coercereturning null
semver.coerce(v)can theoretically returnnullfor non‑semverish strings; passing that intosemver.satisfieswould throw. Yarn lock entries for these packages should all be normal semver versions, so this is low risk, but you can make the check more robust with a small guard:- .filter((v) => semver.satisfies(semver.coerce(v), pkg.semver)), + .filter((v) => { + const coerced = semver.coerce(v); + return coerced && semver.satisfies(coerced, pkg.semver); + }),frontend/package.json (1)
14-20: Postinstall TS check assumes devDependencies are installed on every installHooking
check-patternfly-modules.tsintopostinstallviayarn ts-node ./scripts/check-patternfly-modules.tsgives you strong guarantees on PatternFly resolutions, but it also means:
- Any
yarn install(includingyarn install --productionor environments that omit devDependencies) now requirests-node,@yarnpkg/lockfile,chalk, etc. to be present.- A failure in that check will abort installation entirely.
If your build and runtime flows never do production‑only installs for this package, that’s fine; otherwise you may want to gate this behind an env flag or move it into the CI/test path (similar to the new
dedupe-depsenforcement) to avoid surprising failures outside developer/CI installs.Also applies to: 238-251, 309-311, 316-316
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (1)
frontend/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
frontend/package.json(4 hunks)frontend/scripts/check-patternfly-modules.sh(0 hunks)frontend/scripts/check-patternfly-modules.ts(1 hunks)test-frontend.sh(1 hunks)
💤 Files with no reviewable changes (1)
- frontend/scripts/check-patternfly-modules.sh
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
frontend/scripts/check-patternfly-modules.tstest-frontend.shfrontend/package.json
🔇 Additional comments (2)
test-frontend.sh (1)
34-38: Lockfile dedupe gate fits well into CI pre-checksThis block cleanly enforces a deduped
yarn.lockbefore cycle checks, with consistent messaging and diff output in line with the other guards. Just ensure all CI jobs that invoketest-frontend.shhave thededupe-depsscript and its tooling available so this step can run everywhere it's expected.frontend/scripts/check-patternfly-modules.ts (1)
40-72: New @patternfly / @patternfly‑5 packages will hard‑fail this checkBecause
SCOPES_TO_CHECKis the set{'@patternfly', '@patternfly-5'}andparsePackageNamethrows when a key in those scopes doesn’t start with one of the explicitPKGS_TO_CHECKnames, any new or transitive@patternfly/*or@patternfly-5/*package will cause installs to fail until this list is updated.If that strict “force explicit registration for every PatternFly package” behavior is intentional, this is a good guardrail. If you only wanted to enforce versions on the known subset, you may want to narrow the filter (e.g., match only keys that start with the known
pkg.nameprefixes and ignore unknown PatternFly packages).
|
@logonoff: This pull request references CONSOLE-4979 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.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. |
| "node": ">=22.x" | ||
| }, | ||
| "resolutions": { | ||
| "@types/lodash": "4.14.106", |
There was a problem hiding this comment.
typedefs do not follow semver, this is needed to suppress a couple of lodash related type errors in new versions
|
@logonoff: This pull request references CONSOLE-4979 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
logonoff
force-pushed
the
CONSOLE-4979-dedupe-yarn
branch
from
d3518bd to
c0182f6
Compare
|
@logonoff: This pull request references CONSOLE-4979 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.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-4979 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
logonoff
force-pushed
the
CONSOLE-4979-dedupe-yarn
branch
from
8695653 to
c00768f
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (1)
frontend/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
frontend/package.json(5 hunks)frontend/packages/console-shared/src/components/links/ExternalLinkButton.tsx(2 hunks)frontend/scripts/check-patternfly-modules.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
frontend/scripts/check-patternfly-modules.tsfrontend/packages/console-shared/src/components/links/ExternalLinkButton.tsxfrontend/package.json
🧬 Code graph analysis (1)
frontend/packages/console-shared/src/components/links/ExternalLinkButton.tsx (1)
frontend/__mocks__/i18next.ts (1)
t(8-14)
🔇 Additional comments (10)
frontend/packages/console-shared/src/components/links/ExternalLinkButton.tsx (2)
12-18: LGTM!The implementation cleanly adopts the PatternFly component group's
ExternalLinkButton, maintaining the public API while delegating rendering to the library component. The translation for the icon title improves accessibility.
1-7: Library version and API compatibility confirmed.The imports from
@patternfly/react-component-groups@6.2.0are correct, the component properly exportsExternalLinkButtonPropsfor type consumers, and the translation key"(Opens in new tab)"is present in all locales. The wrapper implementation correctly merges the translated title intoiconPropsbefore passing through to the PatternFly component.frontend/scripts/check-patternfly-modules.ts (6)
1-15: LGTM: Imports and yarn.lock parsing setup.The imports are appropriate for the task, and the early error handling for yarn.lock parsing failures is good. The relative path to
yarn.lockis correct since this script runs from the repository root via the postinstall hook.
17-37: LGTM: PatternFly package list is well-structured.The package list is comprehensive and well-typed. Using
'*'as the semver range is appropriate since the script's goal is to detect any version duplicates across these packages.
39-39: LGTM: Efficient scope filtering.Using a Set for scope lookup provides O(1) performance when filtering yarn.lock entries.
41-57: LGTM: Robust package name parsing with helpful error messages.The function correctly identifies PatternFly packages and provides clear error messages when an unrecognized PatternFly package is encountered. The use of
basename(__filename)makes the error message maintenance-friendly.
59-70: LGTM: Efficient map construction.The reduce operation efficiently builds the package resolution map in a single pass through the yarn.lock entries. The explicit typing provides compile-time safety.
102-107: LGTM: Appropriate error handling and user guidance.The script correctly exits with code 1 on errors and provides helpful guidance for debugging resolution issues.
frontend/package.json (2)
14-14: LGTM: Postinstall script correctly migrated to TypeScript.The updated postinstall script properly uses ts-node to execute the new TypeScript implementation, maintaining the chain with prepare-husky.
18-18: LGTM: Dedupe script configured appropriately.The dedupe-deps script uses the
higheststrategy, which is a sensible default for ensuring the most recent compatible versions are used.
logonoff
force-pushed
the
CONSOLE-4979-dedupe-yarn
branch
4 times, most recently
from
bc6d0a2 to
a103520
Compare
|
@logonoff: This pull request references CONSOLE-4979 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.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. |
| const SCOPES_TO_CHECK = new Set(PKGS_TO_CHECK.map((pkg) => pkg.name.split('/')[0])); | ||
|
|
||
| /** Get the package name and version from a package key in yarn.lock */ | ||
| const parsePackageName = (pkgKey: string): { pkgName: string } => { |
There was a problem hiding this comment.
Nit - we could just return a string instead of { pkgName: string } object.
Also, can you give an example on pkgKey difference vs pkgName ?
There was a problem hiding this comment.
pkgkey is @patternfly/patternfly@^6.2.3, pkgname is @patternfly/patternfly. previously this also parsed the version from pkgkey (hence the object return type), but it wasn't used so I removed that part
logonoff
force-pushed
the
CONSOLE-4979-dedupe-yarn
branch
from
a103520 to
24250ba
Compare
logonoff
force-pushed
the
CONSOLE-4979-dedupe-yarn
branch
from
24250ba to
d98bd6b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (1)
frontend/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
README.md(1 hunks)frontend/package.json(5 hunks)frontend/scripts/check-patternfly-modules.sh(0 hunks)frontend/scripts/check-patternfly-modules.ts(1 hunks)test-frontend.sh(1 hunks)
💤 Files with no reviewable changes (1)
- frontend/scripts/check-patternfly-modules.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
frontend/package.jsontest-frontend.shfrontend/scripts/check-patternfly-modules.ts
🔇 Additional comments (11)
test-frontend.sh (1)
34-38: LGTM!The dedupe pre-check is correctly implemented and integrates well with the new
dedupe-depsscript. The error message is clear and actionable, guiding users to run the dedupe command and commit the updated lockfile.frontend/scripts/check-patternfly-modules.ts (7)
1-9: LGTM!The imports and initial setup are appropriate for a CLI validation script. The
eslint-disable no-consoledirective is correctly applied for this use case.
10-15: LGTM!The lockfile parsing and error handling are correctly implemented. Synchronous file operations are acceptable for build-time scripts.
17-39: LGTM!The package list and scope derivation are well-structured. Using wildcard semver ranges (
*) is appropriate since the goal is to detect duplicate resolutions rather than enforce specific versions.
41-55: LGTM!The
parsePackageNamefunction correctly maps yarn.lock keys to package names and ensures all PatternFly packages are explicitly handled. The error message provides clear guidance if a new package is encountered.
57-68: LGTM!The map-building logic correctly aggregates resolved versions for each PatternFly package from yarn.lock.
72-110: LGTM!The validation logic is robust and correctly handles edge cases. The defensive null check for
semver.coerce(lines 78-87) properly guards against invalid version strings, addressing the concern raised in the past review.
112-117: LGTM!The error handling and exit logic are correct. The suggestion to run
yarn why <pkg-name>provides a clear next step for debugging resolution issues.frontend/package.json (3)
14-14: LGTM!The postinstall script correctly references the new TypeScript implementation, leveraging the existing
ts-nodetooling.
18-18: LGTM!The dedupe-deps script is correctly configured with the
higheststrategy, which is a sensible default for deduplication. This aligns with the pre-check added in test-frontend.sh.
315-316: LGTM!The resolutions are appropriately configured. The
@types/lodashpinning at 4.14.106 is a known workaround for type errors with lodash-es, as noted in the past review.
logonoff
force-pushed
the
CONSOLE-4979-dedupe-yarn
branch
from
d98bd6b to
9a67a73
Compare
openshift-ci
bot
removed
the
do-not-merge/hold
|
/label acknowledge-critical-fixes-only |
openshift-ci
bot
added
the
acknowledge-critical-fixes-only
logonoff
force-pushed
the
CONSOLE-4979-dedupe-yarn
branch
2 times, most recently
from
db02cc4 to
a4e1e1a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
test-frontend.sh (1)
34-38: Limit diff output toyarn.lockto avoid noisy CI logs
Right nowgit --no-pager diffcan include unrelated workspace changes (generated docs/i18n if this is reordered later, or future steps). Safer to show only the lockfile diff.if ! yarn run dedupe-deps --fail ; then echo "You have duplicate version resolutions of some packages in yarn.lock. Run 'yarn dedupe-deps' and commit the updated yarn.lock." - git --no-pager diff + git --no-pager diff -- yarn.lock exit 1 fi
🧹 Nitpick comments (2)
frontend/scripts/check-patternfly-modules.ts (2)
39-66: Harden yarn.lock “key → package name” parsing (quotes / multi-selectors / prefix collisions)
resolutionKey.split('/')[0]andstartsWith(pkg.name)are brittle for Yarn Classic keys like"@scope/name@^1", "@scope/name@^2"and can also mis-match on prefix-y names. Consider normalizing to the first selector and extracting the actual package name before scope filtering + mapping.const SCOPES_TO_CHECK = new Set(PKGS_TO_CHECK.map((pkg) => pkg.name.split('/')[0])); +const firstSelector = (resolutionKey: string): string => + resolutionKey.split(',')[0].trim().replace(/^"|"$/g, ''); + +const packageNameFromSelector = (selector: string): string => { + // Handles scoped: @scope/name@range... and unscoped: name@range... + const m = selector.match(/^(@[^/]+\/[^@]+|[^@]+)@/); + return m ? m[1] : selector; +}; + /** Get the package name and version from a package key in yarn.lock */ const parsePackageName = (resolutionKey: string): string => { - const pkgName = PKGS_TO_CHECK.find((pkg) => resolutionKey.startsWith(pkg.name)); + const selector = firstSelector(resolutionKey); + const name = packageNameFromSelector(selector); + const pkgName = PKGS_TO_CHECK.find((pkg) => pkg.name === name); // Ensure that all packages within SCOPES_TO_CHECK are checked if (!pkgName) { throw new Error( @@ return pkgName.name; };
42-55: Avoid uncaught throw (clean CI error instead of stacktrace)
parsePackageNamethrows; since this is a CI script, it’s better to catch andconsole.error(...)+process.exit(1)for predictable output.Minimal pattern:
-const patternflyModules = Object.entries(lockFile.object).reduce( +let patternflyModules: Map<string, Array<{ resolvedVersion: string }>>; +try { + patternflyModules = Object.entries(lockFile.object).reduce( (acc, [resolutionKey, resolvedDependency]) => { ... }, new Map<string, Array<{ resolvedVersion: string }>>(), -); + ); +} catch (e) { + console.error(String(e)); + process.exit(1); +}Also applies to: 116-119
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (1)
frontend/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
README.md(1 hunks)frontend/package.json(5 hunks)frontend/scripts/check-patternfly-modules.ts(1 hunks)test-frontend.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- README.md
- frontend/package.json
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
test-frontend.shfrontend/scripts/check-patternfly-modules.ts
| const patternflyModules = Object.entries(lockFile.object).reduce( | ||
| (acc, [resolutionKey, resolvedDependency]) => { | ||
| if (SCOPES_TO_CHECK.has(resolutionKey.split('/')[0])) { | ||
| const pkgName = parsePackageName(resolutionKey); | ||
| if (!acc.has(pkgName)) { | ||
| acc.set(pkgName, []); | ||
| } | ||
| acc.get(pkgName).push({ resolvedVersion: resolvedDependency.version }); | ||
| } | ||
|
|
||
| return acc; | ||
| }, | ||
| new Map<string, Array<{ resolvedVersion: string }>>(), | ||
| ); |
There was a problem hiding this comment.
Scope filtering should use the parsed package name, not resolutionKey.split('/')
If keys are quoted / comma-delimited, resolutionKey.split('/')[0] may be "@patternfly (with a quote) and skip PatternFly entries entirely.
Suggested direction: compute const name = packageNameFromSelector(firstSelector(resolutionKey)), then check SCOPES_TO_CHECK.has(name.split('/')[0]) before aggregating.
🤖 Prompt for AI Agents
In frontend/scripts/check-patternfly-modules.ts around lines 58 to 71, the scope
check uses resolutionKey.split('/')[0] which can fail for quoted or
comma-delimited keys; instead derive the package selector first and compute the
parsed package name, e.g. const name =
packageNameFromSelector(firstSelector(resolutionKey)), then use
SCOPES_TO_CHECK.has(name.split('/')[0]) for the scope check before aggregating;
keep the existing parsePackageName(resolutionKey) usage for the map key and push
resolvedVersion as before, ensuring the scope check uses the parsed package name
rather than raw resolutionKey.split('/') output.
| for (const pkg of PKGS_TO_CHECK) { | ||
| const resolvedVersions = Array.from( | ||
| new Set((patternflyModules.get(pkg.name) || []).map((v) => v.resolvedVersion)), | ||
| ); | ||
|
|
||
| if (resolvedVersions.length === 0) { | ||
| console.error(`${chalk.red(pkg.name)} has no ${chalk.yellow(pkg.semver)} resolutions`); | ||
| hasResolutionErrors = true; | ||
| } else if (resolvedVersions.length === 1) { | ||
| const resolvedVersion = semver.coerce(resolvedVersions[0]); | ||
|
|
||
| if (!resolvedVersion) { | ||
| console.error( | ||
| `${chalk.red(pkg.name)} has a non-semver coercible resolved version: ${chalk.yellow( | ||
| resolvedVersions[0], | ||
| )}`, | ||
| ); | ||
| hasResolutionErrors = true; | ||
| continue; | ||
| } | ||
|
|
||
| if (!semver.satisfies(resolvedVersion, pkg.semver)) { | ||
| console.error( | ||
| `${chalk.red(pkg.name)} has one resolution ${chalk.yellow( | ||
| resolvedVersions[0], | ||
| )} which does not satisfy ${chalk.yellow(pkg.semver)}`, | ||
| ); | ||
| hasResolutionErrors = true; | ||
| } else { | ||
| console.log( | ||
| `${chalk.green(pkg.name)} has one ${chalk.yellow(pkg.semver)} resolution: ${ | ||
| resolvedVersions[0] | ||
| }`, | ||
| ); | ||
| } | ||
| } else { | ||
| console.error(`${chalk.red(pkg.name)} has multiple resolutions: ${resolvedVersions}`); | ||
| hasResolutionErrors = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
Resolved dependency version may be missing; guard before semver logic
resolvedDependency.version is assumed present. If the lock entry is malformed/partial, you’ll end up with undefined in resolvedVersions and then hit the “non-semver coercible” path with a confusing message.
Consider filtering falsy versions at collection time (Line 65) or when building resolvedVersions.
🤖 Prompt for AI Agents
In frontend/scripts/check-patternfly-modules.ts around lines 75 to 114,
resolvedVersions may include undefined/null/empty values from malformed lock
entries which leads to confusing "non-semver coercible" errors; filter out
falsy/non-string versions when building resolvedVersions (e.g., remove
undefined/null/empty before creating the Set) and treat the case where filtering
leaves zero entries as "no resolutions" so semver.coerce is only called with
valid strings.
|
Local run /verified by CI and @yapei |
openshift-ci-robot
added
the
verified
|
@yapei: 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. |
|
/retest |
Go from bash-based `yarn why` based approach (which parses yarn.lock multiple times) to typescript-based `@yarnpkg/lockfile` approach (which only reads it once). This represents a ~12x speedup for the check-patternfly-modules step (from ~12 seconds to ~1 second), and this step no longer fries your CPU.
In yarn classic, deduplication is not done automatically. See: https://www.npmjs.com/package/yarn-deduplicate#why-is-this-necessary This commit introduces a step that runs `yarn-deduplicate` and fails if the `yarn.lock` file is not deduplicated. After this commit, the main entrypoint size is reduced by ~250kb.
logonoff
force-pushed
the
CONSOLE-4979-dedupe-yarn
branch
from
a4e1e1a to
ee563d0
Compare
openshift-ci-robot
removed
the
verified
|
rebase w/ no changes /verified by CI and @yapei |
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. |
|
/override ci/prow/okd-scos-images |
|
@jhadvig: Overrode contexts on behalf of jhadvig: ci/prow/okd-scos-images 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 kubernetes-sigs/prow 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. |
|
/lgtm |
openshift-ci
bot
added
the
plugin-api-approved
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhadvig, logonoff, spadgett 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 |
6 participants
Deduplication
In yarn classic, deduplication is not done automatically. See https://www.npmjs.com/package/yarn-deduplicate#why-is-this-necessary:
This PR introduces a step that runs
yarn-deduplicateand fails if theyarn.lockfile is not deduplicated.After this PR, the vendor main chunk size is reduced by ~250kb due to the reduction of duplicated dependencies.
check-patternfly-modules speedup
Since the above change adds
@yarnpkg/lockfileas a transitive dependency, we can rewritecheck-patternfly-modulesusing that dependency with minimal size increase to our node modules. This PR changes thecheck-patternfly-modulesscript from bash-basedyarn whybased approach (which parses yarn.lock multiple times), to typescript-based@yarnpkg/lockfileapproach (which only reads it once).The script also now ensures that all PF installed packages are explicitly included in the
check-patternfly-modulesscript.This represents a ~12x speedup for the check-patternfly-modules step (from ~12 seconds to ~1 second) (on top of the 2x speedup in #14453), and this step no longer fries your CPU.
The script is now also a bit more informative: