Fix discovering executable files on macOS #106
Conversation
macOS has BSD version of the find utility, which doesn't support the `-perm /111` expression, but it does support `-perm +111` (which the GNU version has since deprecated).
|
Warning Rate limit exceeded@mislav has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 19 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request include modifications to the GitHub Actions workflow file Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GitHub Actions
participant ShellCheck
participant Bash Script
User->>GitHub Actions: Trigger workflow
GitHub Actions->>ShellCheck: Run ShellCheck with parameters
ShellCheck->>GitHub Actions: Return results
GitHub Actions->>User: Output verification results
User->>Bash Script: Execute script
Bash Script-->>User: Output "I am executable"
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
🧹 Outside diff range comments (1)
action.yaml (1)
Line range hint
8-19: Consider improving the deprecation transition strategy.While the deprecation notice is good, the implementation could be more migration-friendly. Instead of an if-else structure that only uses either the old or new parameters, consider merging both during the transition period.
Here's a suggested approach for the exclude paths gathering section:
declare -a excludes excludes+=("! -path *./.git/*") excludes+=("! -path *.go") excludes+=("! -path */mvnw") -if [[ -n "${INPUT_IGNORE}" ]]; then - for path in ${INPUT_IGNORE}; do - excludes+=("! -path *./$path/*") - excludes+=("! -path */$path/*") - excludes+=("! -path $path") - done -else - for path in ${INPUT_IGNORE_PATHS}; do - excludes+=("! -path *./$path/*") - excludes+=("! -path */$path/*") - excludes+=("! -path $path") - done -fi +# Support both old and new parameters during transition +for path in ${INPUT_IGNORE} ${INPUT_IGNORE_PATHS}; do + excludes+=("! -path *./$path/*") + excludes+=("! -path */$path/*") + excludes+=("! -path $path") +done
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (3)
- .github/workflows/scandir.yml (1 hunks)
- action.yaml (1 hunks)
- testfiles/scandir/exec (1 hunks)
🔇 Additional comments (4)
testfiles/scandir/exec (1)
1-2: LGTM! Portable shebang line with good spacing.The shebang line uses
/usr/bin/env bashwhich is the recommended approach for better portability across different systems..github/workflows/scandir.yml (2)
34-35: LGTM! Variables properly defined for cross-platform testingThe new test variables appropriately cover both regular bash scripts and extension-less executables, using portable regex syntax.
34-42: Verify test files exist in the repositoryLet's ensure the test files referenced in the workflow actually exist in the repository.
✅ Verification successful
Based on the shell script results, I can generate the final response as we have sufficient information about the test files' existence and permissions.
Test files exist and have appropriate permissions
The verification confirms that:
- Both test files exist in the expected locations:
testfiles/scandir/run me.bash(with space in filename)testfiles/scandir/exec- The
execfile has executable permissions (-rwxr-xr-x)- The file paths and permissions align with the workflow's expectations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the test files exist echo "Checking for test files..." ls -la testfiles/scandir/run*me.bash testfiles/scandir/exec 2>/dev/null || { echo "Warning: Test files not found" exit 1 } # Check if exec file is actually executable if [ -f testfiles/scandir/exec ]; then ls -l testfiles/scandir/exec | grep -q "^-..x" || { echo "Warning: exec file exists but may not be executable" } fiLength of output: 358
action.yaml (1)
207-207: Verify permission check coverage for different executable scenarios.The current permission check (
-perm "$perm_executable") might miss files that are executable only for specific users or groups. Consider if this matches your intended coverage.
testfiles/scandir/exec
Outdated
| @@ -0,0 +1,3 @@ | |||
| #!/usr/bin/env bash | |||
|
|
|||
| echo "I am executable" | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
💡 Codebase verification
Based on the script results, I can generate the final response as we have all the necessary information:
Add newline at end of file (permissions are correct)
The file already has the correct executable permissions (-rwxr-xr-x), but it's missing a newline at the end as verified by the script output. Please add a newline at the end of the file to follow POSIX standards.
Note: I chose the tag because while the executable permissions are correct (one part of the verification), the script confirmed the missing newline issue (the other part of the verification), which needs to be addressed.
🔗 Analysis chain
Add newline at end of file and verify executable permissions.
- Add a newline at the end of the file to follow POSIX standards.
- Ensure the file has executable permissions for testing.
Let's verify the file permissions and content:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify file permissions and content
# Expected: File should be executable and end with a newline
# Check if the file is executable
ls -l testfiles/scandir/exec
# Check if file ends with a newline
if [ -n "$(tail -c1 testfiles/scandir/exec)" ]; then
echo "Warning: No newline at end of file"
fi
Length of output: 231
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 1
🛑 Comments failed to post (1)
.github/workflows/scandir.yml (1)
64-64:
⚠️ Potential issueFix syntax error in second verification block
The
fistatement is correctly added, but it creates invalid syntax with the followingelif. Theelifshould come before thefi.Apply this fix:
- fi - elif [[ "${{ steps.two.outputs.files }}" =~ $notexpect ]];then + elif [[ "${{ steps.two.outputs.files }}" =~ $notexpect ]];then + fiCommittable suggestion was skipped due to low confidence.
macOS has BSD version of the find utility, which doesn't support the
-perm /111expression, but it does support-perm +111(which the GNU version has since deprecated).This allows discovering extension-less executable scripts on macOS runners.