Added option to ignore / include symlinks #104
Conversation
Root-Core
commented
Oct 18, 2024
Root-Core
commented
- Added option to ignore / include symlinks
- Automated tests are in place
- Fixed trailing whitespaces
- Fixed error message in ignore_names.yml
- Automated tests are in place - Fixed trailing whitespaces - Fixed error message in ignore_names.yml
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several modifications to GitHub Actions workflow files, enhancing their configuration and functionality. Key changes include the alignment of triggers for pull requests under the push directive in multiple workflow files, consistent formatting adjustments, and the addition of a new workflow for handling symbolic links. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GitHub Actions
participant ShellCheck
User->>GitHub Actions: Trigger push/pull request
GitHub Actions->>ShellCheck: Run ShellCheck with ignore_symlinks
ShellCheck->>ShellCheck: Check files with/without symbolic links
ShellCheck->>GitHub Actions: Return results
GitHub Actions->>User: Notify results
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: 7
🧹 Outside diff range comments (4)
testfiles/symlink.sh (2)
Line range hint
1-4: Consider adding a shebang lineWhile not strictly necessary for sourced scripts, it's generally good practice to include a shebang line at the beginning of shell scripts. This improves clarity and allows for direct execution if needed.
Consider adding the following as the first line of the script:
+#!/bin/sh ignore/symlink.sh test="test" echo "$test"
Line range hint
4-4: Add a newline at the end of the fileThe file is missing a newline at the end. This is considered bad practice and can cause issues with some tools. Please add a newline at the end of the file.
ignore/symlink.sh test="test" echo "$test" +.github/workflows/ignore_names.yml (1)
Line range hint
29-30: LGTM: Addedignore_namesparameterThe addition of the
ignore_namesparameter with the valueignore_single_file.shaligns with the PR objective of adding an option to ignore/include specific files. This change allows for more granular control over which files are checked by ShellCheck.Consider adding a comment explaining why
ignore_single_file.shis being ignored, to improve maintainability:with: ignore_paths: ignore ignore_names: ignore_single_file.sh # Ignore this file as it's used for testing the ignore functionality🧰 Tools
🪛 yamllint
[warning] 3-3: truthy value should be one of [false, true]
(truthy)
.github/workflows/additional_files.yml (1)
Line range hint
35-47: Address shellcheck warning and add newline at end of file
- The
expectvariable on line 36 is declared but never used. Consider removing it if it's not needed, or use it in the script if it serves a purpose.- Add a newline at the end of the file to comply with POSIX standards.
Here's a suggested fix:
run: | - expect="testfiles/scandir/run" + # expect="testfiles/scandir/run" # Uncomment and use if needed if [[ ! "${{ steps.check.outputs.files }}" =~ testfiles/scandir/run ]];then echo "::error:: Expected file testfiles/scandir/run not found in ${{ steps.check.outputs.files }}" exit 1 elif [[ ! "${{ steps.check.outputs.files }}" =~ testfiles/scandir/finish ]];then echo "::error:: Expected file testfiles/scandir/finish not found in ${{ steps.check.outputs.files }}" exit 1 elif [[ ! "${{ steps.check.outputs.files }}" =~ testfiles/scandir/discovery ]];then echo "::error:: Expected file testfiles/scandir/discovery not found in ${{ steps.check.outputs.files }}" exit 1 fi +This change comments out the unused
expectvariable (you can uncomment and use it if needed) and adds a newline at the end of the file.🧰 Tools
🪛 actionlint
35-35: shellcheck reported issue in this script: SC2034:warning:1:1: expect appears unused. Verify use (or export if used externally)
(shellcheck)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (7)
- .github/workflows/additional_files.yml (3 hunks)
- .github/workflows/check_together.yml (3 hunks)
- .github/workflows/ignore_names.yml (2 hunks)
- .github/workflows/ignore_symlinks.yml (1 hunks)
- action.yaml (4 hunks)
- testfiles/ignore/symlink.sh (1 hunks)
- testfiles/symlink.sh (1 hunks)
🧰 Additional context used
🪛 yamllint
.github/workflows/additional_files.yml
[warning] 3-3: truthy value should be one of [false, true]
(truthy)
[error] 47-47: no new line character at the end of file
(new-line-at-end-of-file)
.github/workflows/check_together.yml
[warning] 3-3: truthy value should be one of [false, true]
(truthy)
[error] 44-44: no new line character at the end of file
(new-line-at-end-of-file)
.github/workflows/ignore_names.yml
[warning] 3-3: truthy value should be one of [false, true]
(truthy)
.github/workflows/ignore_symlinks.yml
[warning] 3-3: truthy value should be one of [false, true]
(truthy)
[error] 23-23: wrong indentation: expected 6 but found 4
(indentation)
🪛 actionlint
.github/workflows/additional_files.yml
35-35: shellcheck reported issue in this script: SC2034:warning:1:1: expect appears unused. Verify use (or export if used externally)
(shellcheck)
.github/workflows/ignore_symlinks.yml
52-52: step ID "check" duplicates. previously defined at line:28,col:11. step ID must be unique within a job. note that step ID is case insensitive
(id)
🔇 Additional comments (17)
testfiles/ignore/symlink.sh (1)
1-1: LGTM: Appropriate shebang for a portable shell script.The use of
#!/bin/shis correct for a POSIX-compliant shell script, ensuring good portability across different Unix-like systems.testfiles/symlink.sh (2)
Line range hint
1-4: Verify the intended use of this file for symlink testingGiven the PR's objective to add an option for ignoring/including symlinks, please confirm that this file is intended to be used as a test case for symlink handling. If so, consider adding a comment at the beginning of the file to explain its purpose and how it should be used in tests.
For example:
+# This file is used as a test case for symlink handling. +# It should be symlinked to test the ignore/include symlink option. ignore/symlink.sh test="test" echo "$test"This will help other developers understand the file's purpose and how it fits into the testing strategy.
To check if this file is referenced in test scripts:
#!/bin/bash # Search for references to this file in test scripts rg --type sh 'testfiles/symlink\.sh' tests/
1-1: 🧹 Nitpick (assertive)Clarify the purpose of the first line
The first line
ignore/symlink.shappears to be a path or filename. If this is intentional (perhaps for symlink testing), please add a comment explaining its purpose. If it's not intentional, consider removing it.To check if this line is present in other test files:
.github/workflows/check_together.yml (2)
3-3: Improved formatting enhances readability.The changes to the
onsection and theruncommand in the "Verify check" step improve the overall formatting and consistency of the file. This makes the workflow more readable and maintainable.Also applies to: 34-44
🧰 Tools
🪛 yamllint
[warning] 3-3: truthy value should be one of [false, true]
(truthy)
Line range hint
3-44: Changes align with PR objectives for code quality.While this file doesn't directly implement the new symlink feature, the formatting improvements contribute to the overall code quality enhancements mentioned in the PR objectives. These changes help in fixing trailing whitespaces and improving code readability.
.github/workflows/ignore_names.yml (3)
Line range hint
3-9: LGTM: Improved workflow trigger configurationThe changes in the
onsection improve the readability and correctness of the workflow trigger configuration. Bothpushandpull_requestevents are now properly defined for the "master" branch, which aligns with best practices for GitHub Actions workflows.🧰 Tools
🪛 yamllint
[warning] 3-3: truthy value should be one of [false, true]
(truthy)
34-44: LGTM: Improved error message and code formattingThe changes in the
Verify checkstep improve both the accuracy of the workflow feedback and the overall code quality:
- The error message for an unexpected file has been correctly updated from "Expected file" to "Unexpected file", which more accurately describes the condition being checked.
- The indentation has been standardized, and an extraneous space before the closing
fistatement has been removed, improving code readability.These changes enhance the maintainability and clarity of the workflow.
3-3: Note on static analysis warningThe yamllint tool reports a warning for line 3: "truthy value should be one of [false, true]". This is a false positive in the context of GitHub Actions workflows. The
onkey is correctly used here to specify events that trigger the workflow, not as a boolean value. No changes are needed for this line.🧰 Tools
🪛 yamllint
[warning] 3-3: truthy value should be one of [false, true]
(truthy)
.github/workflows/additional_files.yml (3)
Line range hint
3-9: LGTM: Improved workflow trigger configurationThe restructuring of the
onsection improves clarity and correctness. Bothpushandpull_requestevents are now properly aligned and associated with the "master" branch, which is a best practice for GitHub Actions workflows.🧰 Tools
🪛 yamllint
[warning] 3-3: truthy value should be one of [false, true]
(truthy)
Line range hint
35-47: LGTM: Improved formatting in the Verify check stepThe addition of a newline before the closing
fistatement improves readability and follows good formatting practices.🧰 Tools
🪛 actionlint
35-35: shellcheck reported issue in this script: SC2034:warning:1:1: expect appears unused. Verify use (or export if used externally)
(shellcheck)
Line range hint
1-47: Summary of changes in additional_files.ymlThe changes in this file improve the workflow configuration and formatting:
- The
onsection now correctly alignspushandpull_requestevents.- Formatting in the
Verify checkstep has been improved.These changes align with the PR objectives of fixing trailing whitespaces and improving code quality. However, there are minor issues to address:
- An unused variable in the
Verify checkstep.- A missing newline at the end of the file.
Once these small issues are resolved, the changes in this file will be fully compliant with best practices.
🧰 Tools
🪛 actionlint
35-35: shellcheck reported issue in this script: SC2034:warning:1:1: expect appears unused. Verify use (or export if used externally)
(shellcheck)
.github/workflows/ignore_symlinks.yml (2)
1-11: LGTM: Workflow configuration looks goodThe workflow name is descriptive, and the trigger configuration is appropriate. Setting permissions to an empty object follows the principle of least privilege, which is a good security practice.
🧰 Tools
🪛 yamllint
[warning] 3-3: truthy value should be one of [false, true]
(truthy)
13-21: LGTM: Job configuration and matrix strategy are well-definedThe job configuration is clear and consistent with the workflow name. Using a matrix strategy to test on both Ubuntu and macOS is an excellent approach to ensure cross-platform compatibility.
action.yaml (4)
22-25: LGTM: Newignore_symlinksinput parameter added successfully.The new
ignore_symlinksinput parameter has been added correctly, aligning with the PR objective. It's optional, has a clear description, and the default value of "true" ensures backward compatibility.
93-93: LGTM: Proper implementation ofignore_symlinksoption.The changes correctly implement the
ignore_symlinksoption:
- The
INPUT_IGNORE_SYMLINKSenvironment variable is set from the input.- When
INPUT_IGNORE_SYMLINKSis not "true", the-Loption is added tofile_options.This implementation ensures that symlinks are followed only when explicitly requested by the user.
Also applies to: 101-103
159-159: LGTM: Correct integration ofignore_symlinksoption in the find command.The changes in the "Run the check" step properly integrate the
ignore_symlinksoption:
- The
INPUT_FILE_OPTIONSenvironment variable is set from the previous step's output.- The
findcommand now includes${INPUT_FILE_OPTIONS}before the directory argument.This implementation ensures that the
-Loption (when set) is correctly passed to thefindcommand, enabling symlink following when requested.Also applies to: 172-174
22-25: Summary: Excellent implementation of theignore_symlinksoption.The changes in this file successfully implement the new
ignore_symlinksoption, meeting the PR objective. Key points:
- A new input parameter is added with clear documentation.
- The option is properly integrated into the workflow's logic.
- The
findcommand is updated to respect the new option.- Backward compatibility is maintained with the default value of "true".
These changes provide users with fine-grained control over symlink behavior in the ShellCheck action while maintaining the existing functionality for current users.
Also applies to: 93-93, 101-103, 159-159, 172-174
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- action.yaml (5 hunks)
🧰 Additional context used
🔇 Additional comments (4)
action.yaml (4)
22-25: LGTM: Newignore_symlinksparameter added successfully.The new
ignore_symlinksparameter has been implemented correctly. It aligns with the PR objectives, maintains backward compatibility with its default value, and has a clear description.
159-159: LGTM: New environment variable for file options added.The new
INPUT_FILE_OPTIONSenvironment variable correctly references the output from the "Set options" step, allowing thefindcommand to use the symlink option if set.
Line range hint
1-238: Summary: PR objectives successfully implemented with minor suggestions.The new
ignore_symlinksoption has been successfully implemented, meeting the PR objectives. The changes are well-integrated into the existing workflow and maintain backward compatibility. A few minor suggestions were made:
- Verify the usage of the "-L" option for including symlinks in the
findcommand.- Consider reviewing the slight indentation changes to ensure they align with your project's formatting guidelines.
Overall, the implementation is solid and ready for merging after addressing these minor points.
93-93: Verify the-Loption for including symlinks.The implementation of the
ignore_symlinksoption looks correct. However, please verify that "-L" is the correct option for including symlinks in thefindcommand used later in the script.To verify the usage of the "-L" option, you can run the following command:
This should show that "-L" or "--follow" is indeed the correct option for following symbolic links.
Also applies to: 101-103
✅ Verification successful
Verified the
-Loption correctly includes symlinks in thefindcommand.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
find --help | grep -E "\-L|\-\-follow"Length of output: 115
|
@ludeeus Any chance to get this (and other PRs) merged? |
