CONSOLE-5285: remove corepack dep for build-(frontend/demos), bump yarn#16426
Conversation
logonoff
changed the title
📝 WalkthroughWalkthroughThis PR upgrades Yarn from version 4.12.0 to 4.14.1 across the project and refactors build scripts to resolve the Yarn executable path from 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
build-demos.sh (1)
6-8: ⚡ Quick winConsider hardening
yarnPathresolution for better error diagnostics.The current implementation correctly resolves
yarnPathfrom.yarnrc.yml, but explicit guards and array invocation would improve debuggability if parsing ever breaks or the path becomes invalid. This is low effort and catches edge cases early.Proposed patch
pushd dynamic-demo-plugin -LOCAL_YARN="node $(awk '/yarnPath:/{print $2}' .yarnrc.yml)" -$LOCAL_YARN install --immutable -$LOCAL_YARN run build +YARN_PATH="$(awk -F': ' '$1 == "yarnPath" { print $2; exit }' .yarnrc.yml)" +if [[ -z "${YARN_PATH}" || ! -f "${YARN_PATH}" ]]; then + echo "Failed to resolve yarnPath from dynamic-demo-plugin/.yarnrc.yml: '${YARN_PATH}'" >&2 + exit 1 +fi +LOCAL_YARN=(node "${YARN_PATH}") +"${LOCAL_YARN[@]}" install --immutable +"${LOCAL_YARN[@]}" run build popd🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@build-demos.sh` around lines 6 - 8, The script's LOCAL_YARN assignment using awk to extract yarnPath lacks guards and uses a plain string that can break on spaces; update the logic that sets LOCAL_YARN so it validates the parsed value from ".yarnrc.yml", ensures it's non-empty and points to an existing file, and exposes clear error messages before proceeding; also store the command as an array (e.g., LOCAL_YARN=("node" "$YARN_PATH") conceptually) and invoke it as an array to preserve arguments when calling the install and build steps referenced in the diff (LOCAL_YARN install --immutable and LOCAL_YARN run build).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@build-demos.sh`:
- Around line 6-8: The script's LOCAL_YARN assignment using awk to extract
yarnPath lacks guards and uses a plain string that can break on spaces; update
the logic that sets LOCAL_YARN so it validates the parsed value from
".yarnrc.yml", ensures it's non-empty and points to an existing file, and
exposes clear error messages before proceeding; also store the command as an
array (e.g., LOCAL_YARN=("node" "$YARN_PATH") conceptually) and invoke it as an
array to preserve arguments when calling the install and build steps referenced
in the diff (LOCAL_YARN install --immutable and LOCAL_YARN run build).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: bdf5bc59-3bbb-47a2-a06e-a105a69a18d8
⛔ Files ignored due to path filters (4)
dynamic-demo-plugin/yarn.lockis excluded by!**/yarn.lock,!**/*.lockfrontend/.yarn/releases/yarn-4.12.0.cjsis excluded by!**/.yarn/**frontend/.yarn/releases/yarn-4.14.1.cjsis excluded by!**/.yarn/**frontend/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
Dockerfilebuild-demos.shbuild-frontend.shdynamic-demo-plugin/package.jsonfrontend/.yarnrc.ymlfrontend/package.json
📜 Review details
🔇 Additional comments (5)
build-frontend.sh (1)
6-8: SameyarnPathguard concern as inbuild-demos.sh.This block duplicates the same command-construction pattern and should receive the same hardening.
dynamic-demo-plugin/package.json (1)
76-76:packageManagerbump looks consistent with the PR scope.No issues here; this aligns with the Yarn 4.14.1 upgrade.
frontend/package.json (1)
347-347: FrontendpackageManagerupdate is aligned.This change is in sync with the Yarn version bump.
frontend/.yarnrc.yml (1)
13-15: Yarn RC updates look coherent and intentional.The preapproved package allowlist, architecture structure, and
yarnPathupdate are consistent with the PR goals.Also applies to: 18-25, 27-27
Dockerfile (1)
18-19: Docker build-stage env update looks good.Setting Playwright/Cypress download skips before frontend build is a clean optimization for this stage.
Also applies to: 21-21
|
/label px-approved these changes do not affect runtime |
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. |
openshift-ci
Bot
added
px-approved
|
/jira refresh |
|
@logonoff: This pull request explicitly references no jira issue. 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-robot
added
the
jira/valid-reference
logonoff
changed the title
|
@logonoff: This pull request references CONSOLE-5285 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. |
Also update yarn config to exclude Red Hat-maintained packages from `npmMinimalAgeGate`
openshift-ci-robot
removed
the
verified
|
these changes do not affect runtime |
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. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: logonoff, rhamilto 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 |
openshift-ci
Bot
added
the
approved
rhamilto
added
the
plugin-api-approved
|
@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. |
3 participants
PLAYWRIGHT_SKIP_BROWSER_DOWNLOADin production Dockerfile (we don't yet use@playwright/browser-chromiumor anything but I'd imagine we would want to)build-frontend.shandbuild-demos.sh(taken from NO-JIRA: 4.22 breaking changes and assorted upstream alignment console-plugin-template#107)Summary by CodeRabbit