Add manage domains script #113
Conversation
dosubot
bot
added
the
size:L
|
Warning Rate limit exceeded@PeterDaveHello has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 28 minutes and 2 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. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe pull request introduces a new shell script Changes
Sequence DiagramsequenceDiagram
participant User
participant Script as manage_domains.sh
participant ListFile as list
participant InactiveFile as inactive
alt Adding Domain
User->>Script: add newdomain.com
Script->>ListFile: Check if domain exists
alt Domain not in list
Script->>ListFile: Add domain
Script->>ListFile: Sort entries
else Domain already exists
Script-->>User: Show existing domain message
end
end
alt Moving Domain
User->>Script: move newdomain.com
Script->>ListFile: Check if domain exists
alt Domain in list
Script->>ListFile: Remove domain
Script->>InactiveFile: Add domain
Script->>InactiveFile: Sort entries
else Domain not found
Script-->>User: Show not found message
end
end
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 (
|
There was a problem hiding this comment.
Copilot reviewed 1 out of 3 changed files in this pull request and generated no comments.
Files not reviewed (2)
- manage_domains.sh: Language not supported
- tests/manage_domains_test.sh: Language not supported
dosubot
bot
added
documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
manage_domains.sh (1)
29-36: Enhance command-line argument validation.The current argument validation could be more robust by validating the command format and providing more helpful error messages.
+# Validate command format +valid_commands="add move" + if [ "$#" -ne 2 ]; then - echo "Usage: $0 {add|move} domain" + echo "Error: Incorrect number of arguments" + echo "Usage: $(basename "$0") {add|move} domain" + echo "Commands:" + echo " add Add a new domain to the active list" + echo " move Move a domain to the inactive list" exit 1 fi command=$1 domain=$2 + +if [[ ! " $valid_commands " =~ " $command " ]]; then + echo "Error: Invalid command '$command'" + echo "Valid commands are: $valid_commands" + exit 1 +fitests/manage_domains_test.sh (1)
38-40: Implement safer file restoration.The current file restoration process could be improved to prevent potential data loss.
# Restore original files -mv "$test_list_file" "$list_file" -mv "$test_inactive_file" "$inactive_file" +if ! cmp -s "$test_list_file" "$list_file"; then + cp "$test_list_file" "$list_file" || { + echo "Error: Failed to restore $list_file" + exit 1 + } +fi +if ! cmp -s "$test_inactive_file" "$inactive_file"; then + cp "$test_inactive_file" "$inactive_file" || { + echo "Error: Failed to restore $inactive_file" + exit 1 + } +fiREADME.md (1)
35-51: Enhance documentation with additional details.The documentation could be improved by:
- Adding examples of successful/error outputs
- Documenting the test script
- Explaining the comment preservation behavior
## Managing Domains -A shell script `manage_domains.sh` is provided to help manage the domains in the `list` and `inactive` files. The script ensures that the files remain sorted and comments are not affected. +A shell script `manage_domains.sh` is provided to help manage the domains in the `list` and `inactive` files. The script: +- Validates domain format +- Maintains file sorting +- Preserves comment headers +- Provides clear success/error messages ### Usage To add a new domain to the list file: ```sh ./manage_domains.sh add <domain> +# Success: Domain example.com added to list. +# Error: Invalid domain format: invalid..domainTo move an inactive domain to the inactive list:
./manage_domains.sh move <domain> +# Success: Domain example.com moved to inactive. +# Error: Domain example.com does not exist in list.
+### Testing
+
+A test scriptmanage_domains_test.shis provided to verify the functionality:
+sh +./tests/manage_domains_test.sh +</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between fe20514c404b6ce118c2e41a9057d47c8f028cf0 and c9907b0d37be419d1f50e4f0f85458693ee973c2. </details> <details> <summary>📒 Files selected for processing (3)</summary> * `README.md` (1 hunks) * `manage_domains.sh` (1 hunks) * `tests/manage_domains_test.sh` (1 hunks) </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| move_domain() { | ||
| local domain=$1 | ||
| if grep -q "^$domain$" "$list_file"; then | ||
| sed -i "/^$domain$/d" "$list_file" | ||
| echo "$domain" >> "$inactive_file" | ||
| (head -n 15 "$inactive_file" && tail -n +16 "$inactive_file" | sort) > "$inactive_file.tmp" && mv "$inactive_file.tmp" "$inactive_file" | ||
| echo "Domain $domain moved to $inactive_file." | ||
| else | ||
| echo "Domain $domain does not exist in $list_file." | ||
| fi | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve file operations safety in move_domain function.
The current implementation has several potential issues:
- Non-atomic file operations could lead to data loss
- sed operations could fail silently
- Same hardcoded line number issue as add_domain
move_domain() {
local domain=$1
if grep -q "^$domain$" "$list_file"; then
- sed -i "/^$domain$/d" "$list_file"
- echo "$domain" >> "$inactive_file"
- (head -n 15 "$inactive_file" && tail -n +16 "$inactive_file" | sort) > "$inactive_file.tmp" && mv "$inactive_file.tmp" "$inactive_file"
+ # Create temporary files
+ tmp_list=$(mktemp)
+ tmp_inactive=$(mktemp)
+
+ # Atomic operations using temporary files
+ grep -v "^$domain$" "$list_file" > "$tmp_list"
+ (echo "$domain"; cat "$inactive_file") | grep -v '^#' | sort > "$tmp_inactive"
+
+ # Preserve comments
+ (grep '^#' "$inactive_file"; cat "$tmp_inactive") > "$inactive_file"
+ mv "$tmp_list" "$list_file"
+
+ # Cleanup
+ rm -f "$tmp_list" "$tmp_inactive"
echo "Domain $domain moved to $inactive_file."
else
echo "Domain $domain does not exist in $list_file."
fi
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| move_domain() { | |
| local domain=$1 | |
| if grep -q "^$domain$" "$list_file"; then | |
| sed -i "/^$domain$/d" "$list_file" | |
| echo "$domain" >> "$inactive_file" | |
| (head -n 15 "$inactive_file" && tail -n +16 "$inactive_file" | sort) > "$inactive_file.tmp" && mv "$inactive_file.tmp" "$inactive_file" | |
| echo "Domain $domain moved to $inactive_file." | |
| else | |
| echo "Domain $domain does not exist in $list_file." | |
| fi | |
| } | |
| move_domain() { | |
| local domain=$1 | |
| if grep -q "^$domain$" "$list_file"; then | |
| # Create temporary files | |
| tmp_list=$(mktemp) | |
| tmp_inactive=$(mktemp) | |
| # Atomic operations using temporary files | |
| grep -v "^$domain$" "$list_file" > "$tmp_list" | |
| (echo "$domain"; cat "$inactive_file") | grep -v '^#' | sort > "$tmp_inactive" | |
| # Preserve comments | |
| (grep '^#' "$inactive_file"; cat "$tmp_inactive") > "$inactive_file" | |
| mv "$tmp_list" "$list_file" | |
| # Cleanup | |
| rm -f "$tmp_list" "$tmp_inactive" | |
| echo "Domain $domain moved to $inactive_file." | |
| else | |
| echo "Domain $domain does not exist in $list_file." | |
| fi | |
| } |
There was a problem hiding this comment.
I'm not sure if this increase the complexity too much?
There was a problem hiding this comment.
@PeterDaveHello, the approach suggested by the refactoring is indeed a bit more verbose, but it helps ensure safer operations (e.g., preventing data corruption if the script is interrupted), makes it clear which sections handle comments vs. domain entries, and minimizes the risk of silent failures. If you’re concerned about complexity, you might retain the current approach (keeping it simpler) unless you regularly expect concurrent file operations or potential interruptions that could corrupt the domain files. In most cases, the added safety is a worthwhile trade-off, but it does depend on how critical failure safety is for your use case.
There was a problem hiding this comment.
Could we perhaps consider a simpler implementation or explore workarounds instead of pursuing a complete, complex solution?
There was a problem hiding this comment.
@PeterDaveHello, here’s a simpler approach you could try if concurrency is not a major concern:
- Remove the domain using
sed -iin place. - Append the domain to the inactive file.
- Sort the domain lines without worrying about partially preserving the file contents in a separate temp file.
For instance:
move_domain() {
local domain=$1
if grep -q "^$domain$" "$list_file"; then
# Remove the domain in place
sed -i "/^$domain$/d" "$list_file"
# Append to inactive file, then re-sort domain lines
echo "$domain" >> "$inactive_file"
(grep '^#' "$inactive_file"; grep -v '^#' "$inactive_file" | sort) > "${inactive_file}.tmp" && mv "${inactive_file}.tmp" "$inactive_file"
echo "Domain $domain moved to $inactive_file."
else
echo "Domain $domain does not exist in $list_file."
fi
}This approach avoids multiple temporary files while still preserving comments by separating them from the domain lines before sorting. Although not fully atomic—interrupted runs may partially update the files—it’s often simpler and sufficient unless you routinely handle concurrent updates or need strong failure-safety guarantees.
| # Test adding a new domain | ||
| ./manage_domains.sh add "newdomain.com" | ||
| if grep -q "^newdomain.com$" "$test_list_file"; then | ||
| echo "Test add domain: PASSED" | ||
| else | ||
| echo "Test add domain: FAILED" | ||
| fi | ||
|
|
||
| # Test moving an inactive domain | ||
| ./manage_domains.sh move "newdomain.com" | ||
| if grep -q "^newdomain.com$" "$test_inactive_file" && ! grep -q "^newdomain.com$" "$test_list_file"; then | ||
| echo "Test move domain: PASSED" | ||
| else | ||
| echo "Test move domain: FAILED" | ||
| fi | ||
|
|
||
| # Test maintaining file sorting and comments | ||
| ./manage_domains.sh add "anotherdomain.com" | ||
| ./manage_domains.sh move "anotherdomain.com" | ||
| if diff -u <(grep -v -E '^(#|$)' "$test_list_file" | sort) <(grep -v -E '^(#|$)' "$test_list_file") && \ | ||
| diff -u <(grep -v -E '^(#|$)' "$test_inactive_file" | sort) <(grep -v -E '^(#|$)' "$test_inactive_file"); then | ||
| echo "Test file sorting and comments: PASSED" | ||
| else | ||
| echo "Test file sorting and comments: FAILED" | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage and reporting.
The test suite could be improved by:
- Adding error case testing
- Tracking test status for summary
- Verifying file contents more comprehensively
+# Test tracking
+tests_total=0
+tests_passed=0
+
+test_case() {
+ local name=$1
+ local result=$2
+ ((tests_total++))
+ if [ "$result" = 0 ]; then
+ echo "✓ $name: PASSED"
+ ((tests_passed++))
+ else
+ echo "✗ $name: FAILED"
+ fi
+}
# Test adding a new domain
+echo "Testing domain addition..."
./manage_domains.sh add "newdomain.com"
-if grep -q "^newdomain.com$" "$test_list_file"; then
- echo "Test add domain: PASSED"
-else
- echo "Test add domain: FAILED"
-fi
+test_case "Add domain" "$(grep -q '^newdomain.com$' "$test_list_file"; echo $?)"
+
+# Test adding invalid domain
+echo "Testing invalid domain addition..."
+./manage_domains.sh add "invalid..domain"
+test_case "Reject invalid domain" "$([ $? -eq 1 ]; echo $?)"
# Additional test cases...
+# Print summary
+echo
+echo "Test Summary: $tests_passed/$tests_total tests passed"
+[ "$tests_passed" -eq "$tests_total" ]Committable suggestion skipped: line range outside the PR's diff.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Summary by CodeRabbit
New Features
Documentation
Tests