Add an option to check all executable scripts #109
Conversation
By default, this action only checks extension-less executable scripts if they have a known shebang such as `#!/usr/bin/env bash`. The new option `all_scripts: true` relaxes the shebang regex pattern so that any executable scripts are checked by shellcheck. This is suitable for repositories with scripts that have unusal shebangs.
📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduce enhancements to the GitHub Actions workflow in Changes
Sequence Diagram(s)sequenceDiagram
participant Workflow
participant ShellCheck
participant Verification
participant Script
Workflow->>ShellCheck: Run ShellCheck with all_scripts=true
ShellCheck-->>Workflow: Output results
Workflow->>Verification: Check for unknown shebang in testfiles/scandir/unknown-shebang
Verification-->>Workflow: Log error if not found
Script->>ShellCheck: Execute unknown-shebang script
ShellCheck-->>Script: Validate shebang and output
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (3)
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: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (3)
- .github/workflows/scandir.yml (1 hunks)
- action.yaml (3 hunks)
- testfiles/scandir/unknown-shebang (1 hunks)
🔇 Additional comments (4)
testfiles/scandir/unknown-shebang (2)
1-1: LGTM! The shebang line correctly represents the test case.The unconventional shebang
#!/usr/bin/with-contenv bashiois an excellent choice for testing the newall_scriptsoption, as it represents a real-world use case from HomeAssistant addons.
1-3: Verify file permissions.Since this file is intended to test the detection of executable scripts, ensure it has the correct executable permissions.
✅ Verification successful
File has correct executable permissions
The file
testfiles/scandir/unknown-shebanghas the executable bit set (-rwxr-xr-x), which is appropriate for a test file intended to verify executable script detection.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the test file has executable permissions # Expected: File should have executable bit set ls -l testfiles/scandir/unknown-shebangLength of output: 122
.github/workflows/scandir.yml (1)
65-71: LGTM! New ShellCheck step is properly configured.The step follows the established pattern and correctly implements the new
all_scriptsparameter.action.yaml (1)
9-12: LGTM! Well-documented input parameter.The new
all_scriptsinput parameter is well-defined with clear documentation that explains both its purpose and the default behavior. The empty default value ensures backward compatibility.
| @@ -147,6 +151,7 @@ runs: | |||
| shell: bash | |||
| id: check | |||
| env: | |||
| INPUT_ALL_SCRIPTS: ${{ inputs.all_scripts }} | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Implementation looks good, but consider documenting security implications.
The technical implementation correctly implements the new functionality to check all executable scripts with any shebang when all_scripts is true. However, there are some important considerations:
- Security: Checking all executable files with any shebang could potentially process non-shell scripts, which might lead to false positives or waste processing time.
- Performance: This change could significantly increase the number of files being processed in large repositories.
Consider adding documentation in the README to:
- Explain the security implications of enabling
all_scripts - Provide guidance on when to use this option vs.
additional_files - Include examples of scenarios where this option is beneficial (e.g., HomeAssistant addons)
Also applies to: 163-164
By default, this action only checks extension-less executable scripts if they have a known shebang such as
#!/usr/bin/env bash. The new optionall_scripts: truerelaxes the shebang regex pattern so that any executable scripts are checked by shellcheck.This is suitable for repositories with scripts that have unusual shebangs, such as the HomeAssistant addons repository that contains dozens of scripts with shebangs like
#!/usr/bin/with-contenv bashiothat were not included in the shellcheck linting CI check: home-assistant/addons#3803 (comment)I considered using
additional_filesfor that repository, but there were many unique script names, and my idea was to ensure that all scripts are linted by default, even future ones added with unique names that someone might forget to add toadditional_files.Please let me know if a new action input approach I took here is acceptable. Thank you!