- 
                Notifications
    
You must be signed in to change notification settings  - Fork 76
 
Fix/djarbz copyparty argcommas #516
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
Fix/djarbz copyparty argcommas #516
Conversation
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.
Pull Request Overview
This PR fixes a critical bug in argument parsing by switching from comma-separated string processing to proper quoted space-separated arguments. The previous implementation using IFS=',' read -r -a would incorrectly split arguments that legitimately contained commas (like -v /work:/work:A:c,dotsrch), breaking the module's functionality. The changes also standardize output formatting by removing extra newlines and fixing a minor inconsistency in the log file redirection.
- Changed argument handling from comma-delimited to space-delimited with proper quoting to support commas in argument values
 - Standardized script output by removing unnecessary double newlines throughout
 - Updated version references from 1.0.0 to 1.0.1 in documentation
 - Added comprehensive test case to verify handling of arguments containing commas
 
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description | 
|---|---|
| registry/djarbz/modules/copyparty/run.sh | Simplified argument parsing from IFS=',' read -r -a to direct array assignment; removed BOLD variable in favor of inline escape codes; standardized output by removing extra newlines; changed command log redirection from $LOG_PATH to /tmp/copyparty.cmd; changed actual execution from append (>>) to overwrite (>) | 
| registry/djarbz/modules/copyparty/main.tf | Changed argument serialization from comma-separated (join(",", var.arguments)) to space-separated with proper quoting (join(" ", formatlist("\"%s\"", var.arguments))) | 
| registry/djarbz/modules/copyparty/copyparty.tftest.hcl | Updated test assertions to expect new argument format; added comprehensive test case for arguments containing commas | 
| registry/djarbz/modules/copyparty/README.md | Updated version references from 1.0.0 to 1.0.1 in all examples | 
| 
           @DevelopmentCats Copilot review has been addressed.  | 
    
Description
I discovered that if we included a comma inside an argument that bash would split it out as a separate argument.
I added a test to verify.
I also cleaned up some log formatting.
Type of Change
Module Information
Path:
registry/djarbz/modules/copypartyNew version:
v1.0.1Breaking change: [ ] Yes [x] No
Testing & Validation
bun test)bun fmt)Related Issues
None