fix: execute inside of matrix package not root #180
Conversation
WalkthroughThe pull request modifies the Changes
Possibly related PRs
Suggested reviewers
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 (
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (5)
.github/workflows/npm-publish.yaml(2 hunks)packages/add-stakeholder/package.json(1 hunks)packages/create-invoice-form/package.json(1 hunks)packages/invoice-dashboard/package.json(1 hunks)packages/payment-widget/package.json(1 hunks)
🔇 Additional comments (5)
packages/add-stakeholder/package.json (1)
15-16: 🧹 Nitpick (assertive)
Review script execution context to ensure matrix package isolation.
The current implementation still references scripts from the root directory using relative paths (../../scripts/). This might not fully address the original issue of ensuring execution within the matrix package context.
Consider these alternatives to improve package isolation:
- Move the shell scripts into the package directory
- Use workspace-aware npm commands instead of direct shell script calls
Let's verify the script locations and their execution context:
packages/create-invoice-form/package.json (1)
15-16: Verify script execution context
The new scripts use relative paths (../../scripts/), which assumes execution from the package directory. Let's verify this aligns with the PR objective of executing within the matrix package context.
✅ Verification successful
Scripts correctly handle package-level execution context
The scripts are designed to work correctly within the matrix package context:
- Both scripts use
./package.jsonto read package information, indicating they expect to run from the package directory check-release-type.shchecks git diff in the current directory (.) for changespublish-next-release.shhandles versioning and publishing for the current package
The relative path ../../scripts/ in package.json is appropriate since the scripts need to be executed from each package's directory, which aligns with the PR objective of matrix package execution.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the new scripts handle their execution context correctly
# Test: Look for path handling in the new scripts
echo "Checking check-release-type.sh:"
rg -A 5 "pwd|cd|dirname" "scripts/check-release-type.sh"
echo -e "\nChecking publish-next-release.sh:"
rg -A 5 "pwd|cd|dirname" "scripts/publish-next-release.sh"
Length of output: 271
Script:
#!/bin/bash
# Let's first check if these scripts exist and see their content
echo "Checking if scripts exist:"
ls -l scripts/check-release-type.sh scripts/publish-next-release.sh
echo -e "\nContent of check-release-type.sh (if exists):"
cat scripts/check-release-type.sh
echo -e "\nContent of publish-next-release.sh (if exists):"
cat scripts/publish-next-release.sh
# Let's also check for any package.json files that reference these scripts
echo -e "\nPackages using these scripts:"
rg -l "check-release-type|publish-next-release" "**/package.json"
Length of output: 2308
.github/workflows/npm-publish.yaml (2)
56-56: LGTM! Verify script existence in package.json files.
The change to use npm workspace for publishing aligns with the PR objective of executing within the package context.
Let's verify the script exists in all matrix packages:
#!/bin/bash
# Description: Verify publish-next-release script exists in all matrix packages
# Expected: Each package.json should contain "publish-next-release" script
for pkg in "@requestnetwork/add-stakeholder" "@requestnetwork/create-invoice-form" "@requestnetwork/invoice-dashboard" "@requestnetwork/payment-widget"; do
echo "Checking $pkg:"
fd -t f "package.json" | xargs rg "\"publish-next-release\":" -C 2
done44-44: LGTM! Verify script existence in package.json files.
The change to use npm workspace for checking release type aligns with the PR objective of executing within the package context.
Let's verify the script exists in all matrix packages:
✅ Verification successful
LGTM! The check-release-type script is properly configured in all packages
The verification confirms that all packages in the matrix have the check-release-type script defined in their respective package.json files:
- packages/add-stakeholder/package.json
- packages/create-invoice-form/package.json
- packages/invoice-dashboard/package.json
- packages/payment-widget/package.json
Each package executes the script from the root scripts directory: bash ../../scripts/check-release-type.sh
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify check-release-type script exists in all matrix packages
# Expected: Each package.json should contain "check-release-type" script
for pkg in "@requestnetwork/add-stakeholder" "@requestnetwork/create-invoice-form" "@requestnetwork/invoice-dashboard" "@requestnetwork/payment-widget"; do
echo "Checking $pkg:"
fd -t f "package.json" | xargs rg "\"check-release-type\":" -C 2
done
Length of output: 7809
packages/invoice-dashboard/package.json (1)
15-16: LGTM! Changes align with PR objectives.
The script changes correctly implement package-level execution using relative paths, which aligns with the PR's goal of executing within the matrix package instead of the root directory.
| "check-release-type": "bash ../../scripts/check-release-type.sh", | ||
| "publish-next-release": "bash ../../scripts/publish-next-release.sh" |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Document release process changes
Consider adding comments in the package.json or updating the repository's documentation to explain:
- The purpose of
check-release-typeandpublish-next-release - When and how these scripts should be used
- The relationship between these scripts and the CI/CD pipeline
| "check-release-type": "bash ../../scripts/check-release-type.sh", | ||
| "publish-next-release": "bash ../../scripts/publish-next-release.sh" |
There was a problem hiding this comment.
💡 Codebase verification
The referenced shell scripts exist and are consistently used across all packages, but they lack executable permissions:
scripts/check-release-type.sh(current: rw-r--r--)scripts/publish-next-release.sh(current: rw-r--r--)
Please add executable permissions to these scripts:
chmod +x scripts/check-release-type.sh scripts/publish-next-release.sh🔗 Analysis chain
LGTM! Verify referenced shell scripts exist.
The script changes correctly use relative paths to ensure execution from the package context, which aligns with the PR objective.
Let's verify the referenced shell scripts exist and have proper permissions:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and permissions of the referenced shell scripts
# Test: Check if the scripts exist and are executable
ls -l scripts/check-release-type.sh scripts/publish-next-release.sh
# Test: Verify these scripts are consistently used across all packages
fd -t f "package.json" packages/ -x grep -l "check-release-type\|publish-next-release"
Length of output: 471
Script:
#!/bin/bash
# Let's verify if the scripts have consistent usage across all packages by checking their content
for pkg in packages/*/package.json; do
echo "=== $pkg ==="
grep -A 2 "check-release-type\|publish-next-release" "$pkg"
done
Length of output: 1584
| "check-release-type": "bash ../../scripts/check-release-type.sh", | ||
| "publish-next-release": "bash ../../scripts/publish-next-release.sh" |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider improving script robustness and cross-platform compatibility.
While the current implementation works, consider these improvements:
- Use cross-platform npm scripts instead of direct bash calls
- Add script existence validation
- Implement proper error handling
Example improvement:
- "check-release-type": "bash ../../scripts/check-release-type.sh",
- "publish-next-release": "bash ../../scripts/publish-next-release.sh"
+ "check-release-type": "node ../../scripts/check-release-type.js",
+ "publish-next-release": "node ../../scripts/publish-next-release.js"This would:
- Improve Windows compatibility
- Allow better error handling through Node.js
- Enable TypeScript/JavaScript usage for better maintainability
Committable suggestion skipped: line range outside the PR's diff.
Resolves #178
Summary by CodeRabbit
New Features
check-release-typeandpublish-next-releaseacross multiple packages.Bug Fixes
is-release-needed, improving clarity in release management.Documentation