Skip to content

Conversation

@adamoutler
Copy link
Collaborator

@adamoutler adamoutler commented Dec 8, 2025

📌 Description

  • Add Devcontainer Task : F1 -> Tasks -> Load Sample Devices
  • Add Internet device to root of device generation script
  • Set 255 devices as default generation

🔍 Related Issues


📋 Type of Change

Please check the relevant option(s):

  • 🐛 Bug fix
  • [ x ] ✨ New feature
  • ♻️ Code refactor
  • 📚 Documentation update
  • 🧪 Test addition or change
  • [ x ] 🔧 Build/config update
  • 🚀 Performance improvement
  • 🔨 CI/CD or automation
  • 🧹 Cleanup / chore

📷 Screenshots or Logs (if applicable)

image

🧪 Testing Steps

In VSCODE Devcontianer

  1. Press F1 -> Tasks: Run -> Load Sample Devices
  2. Press the reload button at the top of network.php to observe the new configuration

✅ Checklist

  • [ x ] I have read the Contribution Guidelines
  • [ x ] I have tested my changes locally
  • [ x ] I have updated relevant documentation (if applicable)
  • I have verified my changes do not break existing behavior
  • [ x ] I am willing to respond to requested changes and feedback

🙋 Additional Notes

The default config set within .devcontainer/scripts/load-devices.sh is intended to demonstrate a problem with large networks.

DEVICE_COUNT="${DEVICE_COUNT:-255}"

this large default is set because the purpose of injecting dummy devices is to stress test the network map.

Summary by CodeRabbit

  • New Features

    • Added development task to load sample devices from CSV into the local environment with HTTP import capability
    • Enhanced device inventory generation with synthetic Internet root device and improved timestamp ordering for consistency
  • Chores

    • Added development container initialization script featuring validation for required tools, API token retrieval, and database configuration

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 8, 2025

Walkthrough

The 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

Cohort / File(s) Change Summary
Device Loading Infrastructure
.devcontainer/scripts/load-devices.sh
New Bash script that validates devcontainer execution, ensures required tools (sqlite3, python3, curl) are present, generates synthetic CSV inventory via Python, reads API token and GraphQL port from SQLite database, uploads CSV via HTTP POST with authorization, and performs sanity check via GraphQL query.
VSCode Task Integration
.vscode/tasks.json
Added new Dev Container task "[Dev Container] Load Sample Devices" that executes a two-step shell command with configurable working directory and CSV_PATH environment variable.
Inventory Generation Enhancement
scripts/generate-device-inventory.py
Modified timestamp generation in build_row to enforce first_seen ≤ last_seen ordering. Added synthetic "Internet" root device (devType Gateway, non-IP-consuming) to topology before router/switches/APs with root-related field annotations.

Sequence Diagram

sequenceDiagram
    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
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • .devcontainer/scripts/load-devices.sh: Requires careful review of error handling, database query correctness, HTTP request construction, and validation logic; ensure all edge cases (missing tools, database connectivity, API failures) are handled gracefully.
  • scripts/generate-device-inventory.py: Verify timestamp ordering logic correctly enforces first_seen ≤ last_seen; confirm synthetic root device is properly initialized before IP allocation and doesn't break existing device generation flow.
  • .vscode/tasks.json: Simple addition; verify task definition is valid and environment variables are properly configured.

Possibly related PRs

Poem

🐰 A rabbit hops through the dev container gate,
Generates devices—seeds and networks await,
The Internet root takes its rightful place,
Timestamps in order, each in its space,
Load them up, sanity-check with delight—
The topology's perfect, the workflow's just right! 🚀

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Title check ❓ Inconclusive The title is vague and generic. 'Devcontainer-devices' does not clearly convey the main changes: adding a task to load sample devices and enhancing device generation with a root device. Consider a more descriptive title such as 'Add Dev Container task to load sample devices with topology root' that clarifies the primary changes.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 opaque

Using two random_time(now) calls and ordering them so first_seen <= last_seen fixes the inverted timestamp issue, and lexicographic comparison is safe for the YYYY-MM-DD HH:MM:SS format. If you want this to be more self-evident, consider having random_time return a datetime (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 devMac

The 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: devMac is 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 tasks

The HTTP import and follow‑up totals check are well‑instrumented, but both curl calls currently have no timeout. If the backend is unreachable or hangs, Load Sample Devices could block indefinitely. Adding something like --connect-timeout 5 --max-time 60 (or similar values that match your expectations) to these curl invocations would make the workflow more robust during backend failures.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f59f44a and 18e71c8.

📒 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 chmod or chown during operations in devcontainer. Everything is already writable. If you need permissions, the devcontainer setup is broken - fix .devcontainer/scripts/setup.sh or .devcontainer/resources/devcontainer-Dockerfile instead.

Files:

  • .devcontainer/scripts/load-devices.sh
**/*.py

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.py: Use mylog(level, [message]) for logging; levels are: none/minimal/verbose/debug/trace. Use none for 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 use get_setting_value() to retrieve configuration values.
Use environment variables (NETALERTX_DB, NETALERTX_LOG, etc.) everywhere instead of hardcoding paths like /data/db or relative paths.
Use helper.py functions (timeNowDB, normalize_mac, sanitizers) for time/MAC/string operations. Validate MACs before DB writes.
Add/modify settings via ccd() in server/initialise.py or 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 patterns

The task command, cwd, and env wiring line up with the new .devcontainer/scripts/load-devices.sh script and are consistent with the other Dev Container tasks. The explicit CSV_PATH is also helpful for debugging generated inventories.

.devcontainer/scripts/load-devices.sh (3)

4-15: Robust CSV_PATH handling and repo path resolution

The SCRIPT_DIR/REPO_ROOT resolution and CSV_PATH handling (honouring an existing env var and falling back to mktemp with GNU/BusyBox compatibility) are solid and debug-friendly, especially combined with the explicit CSV_PATH set in the VS Code task.


16-41: Env‑driven defaults and preflight checks are appropriate for devcontainer use

Using NETALERTX_DB with a /data/db fallback, 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 workflow

Generating leaf devices via --devices "${DEVICE_COUNT}" (on top of router/switch/AP nodes and the Internet root) aligns with the stress‑test intent, and pulling API_TOKEN / GRAPHQL_PORT from the SQLite Settings table keeps everything driven off the devcontainer’s own config. Just be aware that the total imported rows will be 1 + switches + aps + DEVICE_COUNT, not exactly DEVICE_COUNT.

@jokob-sk jokob-sk merged commit c5f29be into netalertx:main Dec 10, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants