Conversation
Introduces a comprehensive script to build, run, and test NetAlertX within a Dockerized devcontainer environment, replicating the setup defined in . This script ensures consistency for CI/CD pipelines and local development. The script addresses several environmental challenges: - Properly builds the Docker image. - Starts the container with necessary capabilities and host-gateway. - Installs Python test dependencies (, , ) into the virtual environment. - Executes the script to initialize services. - Implements a healthcheck loop to wait for services to become fully operational before running tests. - Configures to use a writable cache directory () to avoid permission issues. - Includes a workaround to insert a dummy 'internet' device into the database, resolving a flakiness in caused by its reliance on unpredictable database state without altering any project code. This script ensures a green test suite, making it suitable for automated testing in environments like GitHub Actions.
Uses the new run_docker_tests.sh script which is self-contained and handles all dependencies and test execution within a Docker container. This ensures that the CI environment is consistent with the local devcontainer environment. Fixes an issue where the job name 'test' was considered invalid. Renamed to 'docker-tests'. Ensures that tests marked as 'feature_complete' are also excluded from the test run.
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
run_docker_tests.sh (1)
72-74: Database workaround supports test stability with clear semantics.Inserting a dummy "internet" device via
INSERT OR IGNOREprovides a stable baseline for flaky tests while maintaining idempotency. However, this pattern should be documented in a code comment or in test fixtures to clarify the intent for future maintainers, especially if the underlying flakiness intest_wakeonlan_deviceis later resolved.Consider adding a clarifying comment above this section explaining the flakiness it addresses:
# --- 8. Manipulate Database for Flaky Test --- echo "--- Inserting 'internet' device into database for flaky test ---" +# Workaround: test_wakeonlan_device is flaky due to unpredictable DB state. +# Ensuring a stable 'internet' device row prevents spurious failures. docker exec netalertx-test-container /bin/bash -c " \ sqlite3 /data/db/app.db \"INSERT OR IGNORE INTO Devices (devMac, devFirstConnection, devLastConnection, devLastIP, devName) VALUES ('internet', DATETIME('now'), DATETIME('now'), '0.0.0.0', 'Internet Gateway');\" \ "
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.devcontainer/Dockerfile(1 hunks).github/workflows/code_checks.yml(2 hunks)install/production-filesystem/entrypoint.d/20-first-run-db.sh(1 hunks)run_docker_tests.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1184
File: .devcontainer/Dockerfile:18-19
Timestamp: 2025-09-20T03:01:19.912Z
Learning: In the NetAlertX repository, .devcontainer/Dockerfile is auto-generated and should not be reviewed directly. Review comments about dependencies and build steps should be directed at the root Dockerfile where the actual source commands are located.
📚 Learning: 2025-09-20T03:01:19.912Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1184
File: .devcontainer/Dockerfile:18-19
Timestamp: 2025-09-20T03:01:19.912Z
Learning: In the NetAlertX repository, .devcontainer/Dockerfile is auto-generated and should not be reviewed directly. Review comments about dependencies and build steps should be directed at the root Dockerfile where the actual source commands are located.
Applied to files:
run_docker_tests.sh.devcontainer/Dockerfile
📚 Learning: 2025-09-20T02:56:24.501Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1184
File: .devcontainer/devcontainer.json:5-8
Timestamp: 2025-09-20T02:56:24.501Z
Learning: In the NetAlertX devcontainer setup, the final .devcontainer/Dockerfile is generated by combining the root Dockerfile with .devcontainer/resources/devcontainer-Dockerfile using the generate-dockerfile.sh script. The devcontainer.json should reference the generated file, not the root Dockerfile.
Applied to files:
run_docker_tests.sh.devcontainer/Dockerfile
📚 Learning: 2025-10-26T17:09:18.621Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1235
File: .devcontainer/scripts/setup.sh:146-148
Timestamp: 2025-10-26T17:09:18.621Z
Learning: In `.devcontainer/scripts/setup.sh` and other devcontainer setup scripts for NetAlertX, chmod 666 on /var/run/docker.sock is acceptable because devcontainer environments are single-user development contexts where convenience can take priority over strict permission hardening.
Applied to files:
run_docker_tests.sh.devcontainer/Dockerfile
📚 Learning: 2025-09-20T14:08:44.292Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1184
File: .devcontainer/scripts/stream-logs.sh:5-6
Timestamp: 2025-09-20T14:08:44.292Z
Learning: The .devcontainer/scripts/stream-logs.sh script in NetAlertX is designed as a diagnostic tool for troubleshooting devcontainer startup issues. When log files don't exist, this indicates that the executable/services didn't start properly, which is valuable diagnostic information. Pre-creating missing files would mask this diagnostic behavior.
Applied to files:
run_docker_tests.sh.devcontainer/Dockerfile
📚 Learning: 2025-10-19T01:40:57.095Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1230
File: .devcontainer/resources/devcontainer-overlay/services/config/nginx/netalertx.conf.template:1-2
Timestamp: 2025-10-19T01:40:57.095Z
Learning: In the NetAlertX repository, .devcontainer/resources/devcontainer-overlay/services/config/nginx/netalertx.conf.template is an auto-generated file that is intentionally committed to source control. It cannot be regenerated automatically outside the devcontainer environment and is required for the devcontainer to start, creating a bootstrap dependency.
Applied to files:
.devcontainer/Dockerfile
📚 Learning: 2025-09-20T14:09:29.159Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1184
File: .devcontainer/scripts/setup.sh:103-116
Timestamp: 2025-09-20T14:09:29.159Z
Learning: In NetAlertX devcontainer setup, the netalertx user has write permissions to /var/log/nginx/ directory as it's explicitly chowned to netalertx:www-data in the Dockerfile, so setup.sh can write to nginx log files without sudo.
Applied to files:
.devcontainer/Dockerfile
📚 Learning: 2025-09-20T14:08:48.256Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1184
File: .devcontainer/scripts/setup.sh:90-96
Timestamp: 2025-09-20T14:08:48.256Z
Learning: In the NetAlertX devcontainer setup, the setup.sh script intentionally removes user_notifications.json from the API directory during development environment initialization to prevent notification clutter that accumulates during container launches and development work.
Applied to files:
.devcontainer/Dockerfile
📚 Learning: 2025-09-20T14:08:44.152Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1184
File: .devcontainer/Dockerfile:82-92
Timestamp: 2025-09-20T14:08:44.152Z
Learning: In devcontainer builds, source repository files are not available during the Docker build phase. The source code gets mounted into the container after it's built and started, so COPY commands referencing source files will fail. Configuration files need to be handled at runtime (e.g., in setup scripts) rather than during the build stage.
Applied to files:
.devcontainer/Dockerfile
📚 Learning: 2025-09-20T14:13:33.635Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1184
File: .devcontainer/Dockerfile:11-16
Timestamp: 2025-09-20T14:13:33.635Z
Learning: In the NetAlertX repository with Alpine 3.22 base image, the `python -m venv` command works correctly in the devcontainer setup, likely due to symlink creation in the root Dockerfile that makes `python` available as an alias to `python3`.
Applied to files:
.devcontainer/Dockerfile
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: docker_dev
🔇 Additional comments (11)
install/production-filesystem/entrypoint.d/20-first-run-db.sh (1)
69-69: Schema extension is well-placed and semantically sound.Adding
devParentRelTypealongsidedevParentMACanddevParentPortprovides a clean way to describe device relationships. The TEXT type and nullable design allow gradual adoption without breaking existing functionality..devcontainer/Dockerfile (1)
1-1: File is auto-generated; review root Dockerfile instead.Per project conventions,
.devcontainer/Dockerfileis auto-generated by.devcontainer/scripts/generate-configs.shand should not be reviewed directly. Please review the rootDockerfileand.devcontainer/resources/devcontainer-Dockerfilefor the actual source changes that produce these modifications..github/workflows/code_checks.yml (2)
24-24: Grep pattern improvement enhances readability.The character class
['"]is clearer than escaped-quote variants and correctly matches both single and double quotes in the URL check. Good fix.
88-98: CI workflow migration to Docker-based testing is well-integrated.The job rename and delegation to
run_docker_tests.shproperly channels all test execution through the new Docker-based strategy, ensuring consistency between CI and local devcontainer testing.run_docker_tests.sh (7)
10-19: Script validates dependencies and enables fail-fast behavior appropriately.
set -eensures any command failure exits the script, and the check forgenerate-configs.shprovides clear error messaging if the prerequisite is missing. Good defensive programming.
15-15: Verify intent behind regenerating Devcontainer Dockerfile on each test run.Line 15 calls
generate-configs.shto rebuild.devcontainer/Dockerfilebefore each test. While the script is marked idempotent, this regeneration step may have overhead in CI where the config is unlikely to change between runs. Please confirm this is intentional for maintaining consistency, or consider caching the generated artifact.
31-42: Container capabilities and mounts are appropriate for testing network scanning tools.The combination of
SYS_ADMIN,NET_ADMIN,NET_RAWcapabilities andapparmor=unconfinedenables the network scanning functionality required by NetAlertX. The docker.sock mount is necessary for container-level operations, and the workspaces mount enables live test code evaluation.
46-46: Confirm--ignore-installedpip flag aligns with test environment requirements.The
--ignore-installedflag forces reinstallation of all test dependencies on every run, ensuring a clean environment but incurring overhead. Please confirm this performance tradeoff is acceptable for your CI/test cycle, or consider using a--upgradestrategy instead if package freshness is the primary concern.
53-67: Health check polling is well-designed with proper timeout and diagnostics.The 120-second health check loop with per-second polling provides a good balance between responsiveness and overhead. Logging container output on timeout (line 62) is excellent for debugging failed startup sequences.
78-80: Test marker filtering is correct and well-aligned with PR scope.The pytest marker expression
-m 'not (docker or compose or feature_complete)'appropriately excludes integration and incomplete tests from this container test run, focusing on unit-level functionality that should pass in a clean environment.
84-87: Teardown is clean and reliable.Simple
docker stopanddocker rmcomplete the container lifecycle. No orphaned processes or lingering volumes.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/code_checks.yml(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1184
File: .devcontainer/Dockerfile:18-19
Timestamp: 2025-09-20T03:01:19.912Z
Learning: In the NetAlertX repository, .devcontainer/Dockerfile is auto-generated and should not be reviewed directly. Review comments about dependencies and build steps should be directed at the root Dockerfile where the actual source commands are located.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: docker_dev
🔇 Additional comments (1)
.github/workflows/code_checks.yml (1)
88-98: All verification concerns confirmed—no issues found.The
run_docker_tests.shscript is properly integrated with comprehensive error handling:
- Script exists and is tracked as executable (git mode 100755)
- Error handling is robust:
set -eflag, explicit dependency checks (exit 1), timeout mechanism (120s health check), and logs on failure- Docker is standard in
ubuntu-latestrunners, requiring no additional setup- Test failures are properly reported back to the workflow through multiple exit paths
The
chmod +xcommand in the workflow is redundant since git already tracks the executable bit, but this is harmless and doesn't affect functionality.
| echo "🔍 Checking for incorrect absolute '/php/' URLs (should be 'php/' or './php/')..." | ||
|
|
||
| MATCHES=$(grep -rE "['\"]\/php\/" --include=\*.{js,php,html} ./front | grep -E "\.get|\.post|\.ajax|fetch|url\s*:") || true | ||
| MATCHES=$(grep -rE "[\"']/\/php\/" --include=*.{js,php,html} ./front | grep -E "\.get|\.post|\.ajax|fetch|url\s*:") || true |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the grep regex pattern detects absolute /php/ URLs correctly.
The grep pattern [\"']/\/php\/ in extended regex may have unintended behavior. In extended regex (-E), forward slashes don't require escaping, so \/ is interpreted as a literal forward slash. This means the pattern effectively searches for quote + // + php + / (double slashes), which may not match intended absolute URLs like "/php/..." (single slash).
Verify that this pattern correctly detects absolute URLs by testing it against actual frontend code, or simplify it to ["']/php/ if that was the intent.
🏁 Script executed:
#!/bin/bash
# Test the current grep pattern against a sample file with known patterns
# Create a test file with various URL patterns
cat > /tmp/test_urls.js << 'EOF'
// These should be caught (absolute URLs)
fetch("/php/api/endpoint")
$.get("/php/config")
$.ajax({url: "/php/data"})
// These should NOT be caught (relative URLs)
fetch("php/api/endpoint")
$.get("./php/config")
$.ajax({url: "php/data"})
// Edge case - double slash (check if intentional)
fetch("//php/external")
EOF
echo "=== Testing current pattern: [\"']/\/php\/ ==="
grep -E "[\"']/\/php\/" /tmp/test_urls.js | head -5
echo ""
echo "=== Testing simplified pattern: [\"']/php/ ==="
grep -E "[\"']/php/" /tmp/test_urls.js | head -5
rm /tmp/test_urls.jsLength of output: 191
Fix grep regex pattern to correctly detect absolute /php/ URLs.
The current pattern [\"']/\/php\/ incorrectly matches double-slash URLs (//php/) because forward slashes don't need escaping in extended regex. Testing confirms it only catches fetch("//php/external") but misses intended single-slash absolute URLs like fetch("/php/api/endpoint").
Change line 24 to use the simplified pattern:
MATCHES=$(grep -rE "[\"']/php/" --include=*.{js,php,html} ./front | grep -E "\.get|\.post|\.ajax|fetch|url\s*:") || true
This will correctly detect absolute /php/ URLs as intended.
🤖 Prompt for AI Agents
.github/workflows/code_checks.yml around line 24: the grep pattern currently
escapes slashes and thus incorrectly allows matching double-slash URLs like
//php/, so update the regex to look for an actual single leading slash by
changing the pattern to use /php/ (i.e. replace "[\"']/\/php\/" with
"[\"']/php/") in the MATCHES assignment so grep correctly detects absolute
"/php/" URLs; keep the rest of the pipeline (include list and subsequent grep)
unchanged.
|
This was an easy CodeRabbit review. Looks good to me. Please check out the lint logs Once these python lint issues are corrected, we can enable it to fail on violation. Right now all lint checks are merely a suggestion and will continue-on-error: true. I'll go ahead and take care of the shellcheck & docker items. I caused many if not most of them with my sloppy BASH scripting. It looks like PHP is already good to go and can be |
|
Thanks a lot @adamoutler 🙏 |
|
Looks pretty good: |
|
Those unit tests still need attention . We talked about them previously. They assert to false. I made them return false instead. I'm pretty sure they asserted false to start with. I have a lot of questions there but I have not dug into them. I can tell you for sure the lint is a problem. |
|
Thanks -- pushed a few fixes, getting closer |
2 participants
This pull request introduces a robust, containerized testing strategy for the NetAlertX project, ensuring consistent and reliable test execution for CI/CD pipelines in GitHub Actions.
Key Changes:
Docker-based Test Script (
run_docker_tests.sh):GitHub Actions Workflow Update (
.github/workflows/code_checks.yml):Environment and Test Fixes:
unpredictable database state.
Benefits:
This pull request significantly improves the project's testing infrastructure, providing a stable foundation for future development.
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.