-
-
Notifications
You must be signed in to change notification settings - Fork 365
Devcontainer-devices #1340
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
Devcontainer-devices #1340
Conversation
WalkthroughThe changes introduce a device loading workflow for development environments, including a new Bash script that generates synthetic device inventory and imports it into a local service via HTTP API, a corresponding VSCode task for convenience, and enhancements to the Python inventory generator to ensure proper timestamp ordering and include a synthetic Internet root device in the network topology. Changes
Sequence DiagramsequenceDiagram
participant User
participant BashScript as load-devices.sh
participant PythonGen as generate-device-inventory.py
participant DB as SQLite DB
participant API as HTTP API
participant GraphQL as GraphQL Endpoint
User->>BashScript: Execute load-devices.sh
activate BashScript
BashScript->>BashScript: Validate devcontainer & tools
BashScript->>PythonGen: Generate CSV inventory
activate PythonGen
PythonGen->>PythonGen: Create Internet root + devices
PythonGen->>BashScript: Return CSV file
deactivate PythonGen
BashScript->>DB: Read API_TOKEN & GRAPHQL_PORT
activate DB
DB->>BashScript: Return credentials
deactivate DB
BashScript->>API: POST CSV with Authorization header
activate API
API->>API: Import devices
API->>BashScript: HTTP 200 OK
deactivate API
BashScript->>GraphQL: Query device totals (sanity check)
activate GraphQL
GraphQL->>BashScript: Return device count
deactivate GraphQL
BashScript->>User: Print status & results
deactivate BashScript
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 0
🧹 Nitpick comments (3)
scripts/generate-device-inventory.py (2)
151-168: Timestamp ordering fix is correct; comparison is a bit opaqueUsing two
random_time(now)calls and ordering them sofirst_seen <= last_seenfixes the inverted timestamp issue, and lexicographic comparison is safe for theYYYY-MM-DD HH:MM:SSformat. If you want this to be more self-evident, consider havingrandom_timereturn adatetime(or adding a helper that does) and only formatting to string at assignment time.
219-255: Internet root device design looks good; confirm tolerance for non-MAC devMacThe synthetic “Internet” root node nicely anchors the topology and avoids consuming an IP, and the overrides (scan/log/alerts/static IP off,
devParentRelType="Root") match the intended semantics.One thing to double‑check:
devMacis the literal string"Internet", which is intentionally non-MAC. If any importer or UI paths assume a colon‑separated MAC format, this could surface as a parsing/validation edge case. If that turns out to be an issue, a reserved-but-valid pseudo‑MAC (e.g., a dedicated OUI range) for the root would avoid surprises while keeping it distinguishable..devcontainer/scripts/load-devices.sh (1)
60-77: Consider adding curl timeouts to avoid hanging devcontainer tasksThe HTTP import and follow‑up totals check are well‑instrumented, but both
curlcalls currently have no timeout. If the backend is unreachable or hangs,Load Sample Devicescould block indefinitely. Adding something like--connect-timeout 5 --max-time 60(or similar values that match your expectations) to thesecurlinvocations would make the workflow more robust during backend failures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.devcontainer/scripts/load-devices.sh(1 hunks).vscode/tasks.json(1 hunks)scripts/generate-device-inventory.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
.devcontainer/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Never
chmodorchownduring operations in devcontainer. Everything is already writable. If you need permissions, the devcontainer setup is broken - fix.devcontainer/scripts/setup.shor.devcontainer/resources/devcontainer-Dockerfileinstead.
Files:
.devcontainer/scripts/load-devices.sh
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Usemylog(level, [message])for logging; levels are: none/minimal/verbose/debug/trace. Usenonefor most important messages that should always appear, such as exceptions.
Always set explicit timeouts on subprocess calls. Default to 60s minimum unless plugin config specifies otherwise. Nested subprocess calls need their own timeout.
Never hardcode ports or secrets; always useget_setting_value()to retrieve configuration values.
Use environment variables (NETALERTX_DB,NETALERTX_LOG, etc.) everywhere instead of hardcoding paths like/data/dbor relative paths.
Usehelper.pyfunctions (timeNowDB,normalize_mac, sanitizers) for time/MAC/string operations. Validate MACs before DB writes.
Add/modify settings viaccd()inserver/initialise.pyor per-plugin manifest. Define config once and read it via helpers everywhere.
Always leave logging enabled. If there is a possibility it will be difficult to debug with current logging, add more logging.
Files:
scripts/generate-device-inventory.py
🧠 Learnings (2)
📓 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-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:
.vscode/tasks.json
🔇 Additional comments (4)
.vscode/tasks.json (1)
61-85: New “Load Sample Devices” task is wired correctly and matches existing patternsThe task command,
cwd, and env wiring line up with the new.devcontainer/scripts/load-devices.shscript and are consistent with the other Dev Container tasks. The explicitCSV_PATHis also helpful for debugging generated inventories..devcontainer/scripts/load-devices.sh (3)
4-15: Robust CSV_PATH handling and repo path resolutionThe SCRIPT_DIR/REPO_ROOT resolution and
CSV_PATHhandling (honouring an existing env var and falling back tomktempwith GNU/BusyBox compatibility) are solid and debug-friendly, especially combined with the explicitCSV_PATHset in the VS Code task.
16-41: Env‑driven defaults and preflight checks are appropriate for devcontainer useUsing
NETALERTX_DBwith a/data/dbfallback, plus the explicit checks for devcontainer context, DB existence, and required tools (sqlite3,python3,curl), should make failures obvious instead of failing later in less clear ways.
43-58: CSV generation and settings lookup match the intended workflowGenerating leaf devices via
--devices "${DEVICE_COUNT}"(on top of router/switch/AP nodes and the Internet root) aligns with the stress‑test intent, and pullingAPI_TOKEN/GRAPHQL_PORTfrom the SQLiteSettingstable keeps everything driven off the devcontainer’s own config. Just be aware that the total imported rows will be1 + switches + aps + DEVICE_COUNT, not exactlyDEVICE_COUNT.
📌 Description
🔍 Related Issues
📋 Type of Change
Please check the relevant option(s):
📷 Screenshots or Logs (if applicable)
🧪 Testing Steps
In VSCODE Devcontianer
network.phpto observe the new configuration✅ Checklist
🙋 Additional Notes
The default config set within
.devcontainer/scripts/load-devices.shis intended to demonstrate a problem with large networks.this large default is set because the purpose of injecting dummy devices is to stress test the network map.
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.