Skip to content
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

Add manage domains script #113

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -31,3 +31,21 @@ Your assistance in maintaining and improving these lists is greatly appreciated.
## License

This project is licensed under a [Creative Commons Attribution-ShareAlike 4.0 International License(CC-BY-SA-4.0)](https://creativecommons.org/licenses/by-sa/4.0/).

## 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.

### Usage

To add a new domain to the list file:

```sh
./manage_domains.sh add <domain>
```

To move an inactive domain to the inactive list:

```sh
./manage_domains.sh move <domain>
```
64 changes: 64 additions & 0 deletions manage_domains.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
#!/bin/bash

# Get directory where script is located
SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
list_file="${SCRIPT_DIR}/list"
inactive_file="${SCRIPT_DIR}/inactive"

# Validate files exist
for file in "$list_file" "$inactive_file"; do
if [[ ! -f "$file" ]]; then
echo "Error: Required file $file not found"
exit 1
fi
done

add_domain() {
local domain=$1
# Validate domain format
if ! echo "$domain" | grep -qP '^(?:[a-z0-9](?:[a-z0-9-]{0,61}[a-z0-9])?\.)+[a-z]{2,}$'; then
echo "Error: Invalid domain format: $domain"
exit 1
fi
if ! grep -q "^$domain$" "$list_file"; then
echo "$domain" >> "$list_file"
# Preserve comments at top of file
(grep '^#' "$list_file"; grep -v '^#' "$list_file" | sort) > "$list_file.tmp" && mv "$list_file.tmp" "$list_file"
echo "Domain $domain added to $list_file."
else
echo "Domain $domain already exists in $list_file."
fi
}

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
}
Comment on lines +33 to +43
Copy link

@coderabbitai coderabbitai bot Jan 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve file operations safety in move_domain function.

The current implementation has several potential issues:

  1. Non-atomic file operations could lead to data loss
  2. sed operations could fail silently
  3. 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.

Suggested change
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
}

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this increase the complexity too much?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we perhaps consider a simpler implementation or explore workarounds instead of pursuing a complete, complex solution?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PeterDaveHello, here’s a simpler approach you could try if concurrency is not a major concern:

  1. Remove the domain using sed -i in place.
  2. Append the domain to the inactive file.
  3. 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.


if [ "$#" -ne 2 ]; then
echo "Usage: $0 {add|move} domain"
exit 1
fi

command=$1
domain=$2

case $command in
add)
add_domain "$domain"
;;
move)
move_domain "$domain"
;;
*)
echo "Invalid command. Use 'add' to add a domain or 'move' to move a domain."
exit 1
;;
esac
47 changes: 47 additions & 0 deletions tests/manage_domains_test.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
#!/bin/bash

# Create temporary test directory
TEST_DIR=$(mktemp -d)
trap 'rm -rf "$TEST_DIR"' EXIT

# Source files
list_file="../list"
inactive_file="../inactive"

# Test files
test_list_file="${TEST_DIR}/test_list"
test_inactive_file="${TEST_DIR}/test_inactive"

# Backup original files
cp "$list_file" "$test_list_file"
cp "$inactive_file" "$test_inactive_file"

# 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
Comment on lines +19 to +43
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance test coverage and reporting.

The test suite could be improved by:

  1. Adding error case testing
  2. Tracking test status for summary
  3. 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.


# Restore original files
mv "$test_list_file" "$list_file"
mv "$test_inactive_file" "$inactive_file"
Loading