-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
build: Use action from relative-ci/.github #4922
Conversation
WalkthroughThe pull request modifies the GitHub Actions CI workflow configuration by removing several caching steps and the conditional installation of Node.js dependencies across multiple job sections. These have been replaced with a new setup step named "Setup node & npm," which utilizes a custom action from "relative-ci/.github/actions/setup-node-npm@main." This change consolidates the Node.js setup and dependency management into a single step, specifying parameters such as Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
63-64
: Consider standardizing the setup approach across all jobsWhile the new setup action is used in most jobs, the test-package-* jobs still use the direct Node.js setup approach. This inconsistency could lead to maintenance challenges.
Consider either:
- Using the new setup action consistently across all jobs, or
- Document why different approaches are necessary for different job types
Also applies to: 81-82, 99-100, 277-278
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci.yml
(5 hunks)
🔇 Additional comments (2)
.github/workflows/ci.yml (2)
16-17
: Verify the setup action's implementation and reliabilityBefore proceeding with this change, please ensure:
- The action's source code has been reviewed for security
- The action's implementation is documented
- There's a fallback plan if the action becomes unavailable
16-17
: Verify CI pipeline performance impactThe removal of explicit caching and dependency installation steps could affect build times and reliability. Please verify:
- Build times are not negatively impacted
- Dependencies are correctly installed
- Cache invalidation works as expected
Compare the workflow run times before and after this change:
✅ Verification successful
Setup action change is safe and follows best practices ✅
The change to use the organization's shared setup action is a positive improvement that:
- Maintains consistent build times (3-5 minutes for master branch)
- Centralizes CI configuration within the relative-ci organization
- Reduces maintenance overhead through code reuse
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Fetch recent workflow runs gh run list --workflow=ci.yml --limit 10 --json durationInMinutes,status,headBranchLength of output: 328
Script:
#!/bin/bash # Fetch recent workflow runs with correct fields gh run list --workflow=ci.yml --limit 10 --json startedAt,updatedAt,conclusion,headBranch,status | \ jq -r '.[] | [.headBranch, .conclusion, .status, (.startedAt | fromdate), (.updatedAt | fromdate)] | @tsv' | \ awk -F'\t' 'BEGIN {print "Branch\tConclusion\tStatus\tStarted\tUpdated"} {print}' # Check setup action implementation cat .github/actions/setup/action.yml || trueLength of output: 1219
Script:
#!/bin/bash # Check if the repository has any documentation about the setup action fd README.md rg -i "setup.*action|relative-ci.*setup" --type md # Look for any related configuration fd "relative-ci" --type fLength of output: 8537
.github/workflows/ci.yml
Outdated
- name: Setup | ||
uses: "relative-ci/.github/actions/setup@main" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Security: Pin the setup action to a specific version
Using @main
for external actions is a security risk as it points to a mutable reference. The action could change without notice, potentially introducing breaking changes or security vulnerabilities.
Pin the action to a specific version or commit SHA:
- uses: "relative-ci/.github/actions/setup@main"
+ uses: "relative-ci/.github/actions/setup@v1" # or specific SHA
Committable suggestion skipped: line range outside the PR's diff.
#11793 Bundle Size — 380.78KiB (0%).26ef91a(current) vs 7e3852d master#11791(baseline) Warning Bundle contains 2 duplicate packages – View duplicate packages Bundle metrics
|
Current #11793 |
Baseline #11791 |
|
---|---|---|
Initial JS | 333.89KiB |
333.89KiB |
Initial CSS | 46.89KiB |
46.89KiB |
Cache Invalidation | 0% |
0% |
Chunks | 3 |
3 |
Assets | 4 |
4 |
Modules | 700 |
700 |
Duplicate Modules | 0 |
0 |
Duplicate Code | 0% |
0% |
Packages | 39 |
39 |
Duplicate Packages | 1 |
1 |
Bundle size by type no changes
Current #11793 |
Baseline #11791 |
|
---|---|---|
JS | 333.89KiB |
333.89KiB |
CSS | 46.89KiB |
46.89KiB |
Bundle analysis report Branch use-shared-action Project dashboard
Generated by RelativeCI Documentation Report issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/ci.yml (1)
16-17
:⚠️ Potential issueSecurity: Pin the action to a specific version or commit SHA
Using
@main
for external actions is a security risk as it points to a mutable reference. The action could change without notice, potentially introducing breaking changes or security vulnerabilities.Pin the action to a specific version or commit SHA:
- uses: relative-ci/.github/actions/setup-node-npm@main + uses: relative-ci/.github/actions/setup-node-npm@v1 # or specific SHAAlso applies to: 63-64, 81-82, 99-100
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
128-133
: Consider enabling caching for faster buildsThe
cache: never
andinstall: false
parameters are set for package test jobs. While this ensures clean installations, it might significantly impact build times. Consider:
- Enabling caching for faster subsequent runs
- Document why caching is disabled if there's a specific reason
Also applies to: 180-185, 226-231
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci.yml
(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: webpack5 plugin ubuntu-latest nodejs 14
- GitHub Check: webpack4 plugin ubuntu-latest nodejs 14
- GitHub Check: webpack5 plugin ubuntu-latest nodejs 18
- GitHub Check: CLI ubuntu-latest nodejs 14
- GitHub Check: CLI ubuntu-latest nodejs 16
- GitHub Check: rollup 4 plugin nodejs 18
- GitHub Check: CLI ubuntu-latest nodejs 18
- GitHub Check: rollup 3 plugin nodejs 16
- GitHub Check: test-e2e
- GitHub Check: lint
🔇 Additional comments (2)
.github/workflows/ci.yml (2)
282-283
: LGTM: Secure publishing configurationThe publishing setup follows security best practices:
- Runs only for version tags
- Uses NPM provenance
- Has appropriate permissions and secrets
Also applies to: 291-295
Line range hint
119-124
: Review matrix configuration exclusionsThe matrix configurations have specific version exclusions:
- Webpack 4 is excluded for Node 18+ but included for Node 14-16
- Windows tests are limited to Node 14 for webpack
- Rollup tests have specific Node/version combinations
Please verify if these exclusions are still valid and document the reasoning in comments.
Also applies to: 161-177, 213-224
✅ Verification successful
Matrix exclusions are valid and technically justified
The matrix exclusions are based on technical requirements visible in the codebase:
- Webpack 4 exclusion for Node 18+ aligns with the plugin's version-specific implementation that uses different webpack hooks and asset emission methods
- Windows tests on Node 14 provide sufficient coverage while the plugin maintains separate code paths for webpack 4/5 compatibility
- Rollup version pairs (3/16, 4/18) match the peer dependency constraints (
^3.0.0 || ^4.0.0
)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check package.json files for peer dependency constraints echo "Checking webpack plugin peer dependencies:" find . -name "package.json" -exec grep -l "webpack" {} \; -exec cat {} \; | grep -A 5 -B 5 "peerDependencies" echo "Checking rollup plugin peer dependencies:" find . -name "package.json" -exec grep -l "rollup" {} \; -exec cat {} \; | grep -A 5 -B 5 "peerDependencies"Length of output: 1785
Script:
#!/bin/bash # Check the complete CI workflow file echo "Complete CI workflow file:" cat .github/workflows/ci.yml echo -e "\nChecking for version-specific test files:" rg -l "node.*version|webpack.*version|rollup.*version" --type js --type tsLength of output: 7756
Script:
#!/bin/bash # Check webpack-plugin.js for version-specific code echo "Webpack plugin version-specific code:" cat packages/webpack-plugin/src/webpack-plugin.js # Check for any webpack version related tests echo -e "\nWebpack version related tests:" rg -C 3 "webpack.*4|node.*1[468]" packages/webpack-plugin/testLength of output: 5743
Summary by CodeRabbit