Refactor/import utils package - #11
Closed
marcinpsk wants to merge 63 commits into
Closed
Conversation
Add serial-based device matching to import validation: - Match devices by serial number when librenms_id lookup fails - Detect serial/hostname conflicts and offer resolution actions - Track serial_action, serial_confirmed, serial_duplicate, name_sync states in validation results - Flag device_type_mismatch when existing device type differs Add conflict resolution views and UI: - DeviceConflictActionView: resolve conflicts via link, update, update_serial, sync_name, sync_serial, sync_platform, sync_device_type - _build_sync_info: compare serial, platform, device type between NetBox and LibreNMS for details modal - UpdateDeviceNameView: sync device name from LibreNMS sysName - Conflict/details buttons in import table with contextual styling Template and JS improvements: - Rewrite device_validation_details.html for conflict UI - Add name row with sync button to sync base template - Expand import modal to modal-xl for conflict details - DRY hideModal usage in librenms_import.js with Bootstrap fallback Tests: - TestSerialNumberMatching: 20+ test cases covering serial matching, hostname conflicts, serial drift, duplicate detection, device type mismatch, and linked device validation - Fix test_add_device_duplicate_error status code (500→200)
Resolve conflicts in urls.py and views/imports/actions.py: - Keep both DeviceConflictActionView (this branch) and SaveUserPrefView (develop) - Remove obsolete save_import_toggle_prefs call (replaced by SaveUserPrefView endpoint)
…custom field Devcontainer & CI: - Add proxy/CA bundle support with ALLOW_GIT_SSL_DISABLE opt-in - Add Codespaces configuration loader - Remove unnecessary proxy env vars from postgres/redis services - Extract detect_plugin_workspace() helper, idempotent .bashrc guard - Consolidate aliases into load-aliases.sh as single source of truth - Fix CI test workflow to run from correct NetBox directory - Add media/configuration.testing.py for CI - Update lint workflow: actions v4/v5, Python 3.12, fail on lint errors - Exclude tests from package distribution - Fix MD031 markdown lint in README - Add security note about embedding proxy credentials in URLs Plugin: - Auto-create librenms_id custom field via post_migrate signal - Log exceptions instead of silently swallowing them in custom field creation - Add inline comments on _executed flag lifecycle assumptions - Raise KeyError for non-default missing server keys in LibreNMSAPI Tests: - Add setup_method for consistent _executed flag reset - Assert exception logging in test_exception_does_not_propagate - Fix fragile getLogger assertion in test_no_log_when_field_already_exists
…provements
Add serial-based device matching to the import pipeline, a full conflict
resolution UI, per-user toggle persistence, device field sync from the
import modal, and multiple safety/bug fixes across views and JS.
Serial Number Matching
- Serial number as a blocking match criterion (checked between hostname
and IP), giving hardware-identity priority over network-layer matches.
- Serial drift detection on devices already linked by librenms_id: flags
update_serial when serials diverge, or conflict when the incoming
serial belongs to another NetBox device.
- serial_confirmed flag set when linked device serial matches LibreNMS.
- serial_duplicate flag distinguishes true duplicates (incoming serial on
another device) from devices found by serial match.
- serial_action semantics: None | link | conflict | update_serial |
hostname_differs.
Conflict Resolution
- DeviceConflictActionView (HTMX POST) with actions: link, update,
update_serial, sync_name, sync_serial, sync_platform, sync_device_type,
update_type.
- Serial ownership checks on update/sync actions return 409 when the
incoming serial is already assigned to a different NetBox device.
- Device type mismatch detection: warns when existing device type differs
from LibreNMS hardware; requires force checkbox to proceed.
- Conflict/Details button in import table: red for type mismatch, yellow
for serial/hostname conflicts, blue for info-only.
- Modal closes after successful action via HX-Trigger: closeModal.
Import Validation Modal
- Redesigned two-column layout (LibreNMS Status + Device Info table).
- Inline sync buttons for name, serial, platform, and device type.
- Badge-style status indicators: Linked, Name match/differs, Serial
confirmed/differs, Type mismatch.
- _build_sync_info() computes comparison data between LibreNMS device
and existing NetBox device (serial, platform, device type).
- Modal widened from modal-lg to modal-xl.
Device Field Sync Views
- UpdateDeviceNameView: sync NetBox device name from LibreNMS sysName.
- name_sync_available / suggested_name flags for linked devices whose
name differs from sysName (Device only; VMs intentionally excluded
since UpdateDeviceNameView does not support VM objects).
- _refresh_existing_device() re-fetches cached devices from DB so that
role/name/type changes in NetBox are reflected immediately; recomputes
readiness with correct VM branching (site+role for VMs, site+type+role
for devices) using .get("found") consistently.
Toggle & Preference Persistence
- SaveUserPrefView endpoint for persisting toggle state via JS fetch.
- use_sysname, strip_domain, and interface_name_field persist per-user
via NetBox user.config across page reloads.
- Import page reads user prefs with fallback chain: request param →
user pref → LibreNMSSettings model → plugin config.
- Settings page save also updates current user preferences.
Safety & Bug Fixes
- Fix critical bug: enabled was set after interface.save() and never
persisted; moved into update_interface_attributes before save().
- Add full_clean()/try-except to all device field update views
(serial, type, platform, create-platform, VC serial, name) with
proper rollback of the original value on failure.
- Use address__net_host instead of address__startswith for exact IP
matching (prevents false positives with overlapping prefixes).
- Handle DoesNotExist/ValueError/TypeError for user-submitted
selected_device_id in interface sync (fallback to obj).
- Use .get() for ifSpeed/ifType to prevent KeyError.
- Guard None coordinates in create_librenms_location.
- Remove unreachable dead code in locations.py q-filtering.
- Stabilize VC member ordering with order_by("vc_position", "name").
- Fix wrong reverse URL (vm_interface_sync → device_interface_sync).
- Fix </th> → </td> tag mismatch in librenms_sync_base.html.
- Wrap Platform.objects.create in try/except for slug collision.
- Initialize name_sync_available and suggested_name in result dict.
- Remove redundant pass after logger.error.
JavaScript
- hideModal: recover existing Bootstrap getInstance before falling back
to manual close; cache filter-processing-modal element once at top of
pollJobStatus instead of 9 repeated getElementById calls.
- Use hideModal() consistently for results modal auto-close.
- CSRF fallback to csrftoken cookie when HTMX swaps remove hidden input.
- interface_name_field pref saved to user.config via fetch on change.
- WONTFIX comment on fallbackBackdropRef (single-modal, Tabler env).
Docstrings & Templates
- ~130 docstrings across 26 files (62.6% → 98.9% coverage).
- Sync page: empty state messages on tabs before data is loaded.
- Sync page: Name row with sync button in Device Information table.
- Sync page: device type sync button styled as btn-outline-danger.
- Compute existing_device_url once for VM-safe links in modal template.
Tests (1071 new lines, 280 total tests passing)
- 12 serial matching tests (by serial, hostname+serial drift, librenms_id
serial drift, duplicate detection, VM exclusion).
- 8 conflict resolution action tests (link, update, update_serial,
sync_name, sync_platform, sync_device_type).
- 3 toggle persistence tests.
- _build_sync_info comparison tests.
- Device type mismatch and serial confirmation tests.
- Refactored TestSerialNumberMatching to setup_method/teardown_method.
Devcontainer
- Fix MITM proxy SSL for pre-commit (CA bundle cert splitting).
- Fix aliases not loading in postAttach terminal.
…custom field Devcontainer & CI: - Add proxy/CA bundle support with ALLOW_GIT_SSL_DISABLE opt-in - Add Codespaces configuration loader - Remove unnecessary proxy env vars from postgres/redis services - Extract detect_plugin_workspace() helper, idempotent .bashrc guard - Consolidate aliases into load-aliases.sh as single source of truth - Fix CI test workflow to run from correct NetBox directory - Add media/configuration.testing.py for CI - Update lint workflow: actions v4/v5, Python 3.12, fail on lint errors - Exclude tests from package distribution - Fix MD031 markdown lint in README - Add security note about embedding proxy credentials in URLs Plugin: - Auto-create librenms_id custom field via post_migrate signal - Log exceptions instead of silently swallowing them in custom field creation - Add inline comments on _executed flag lifecycle assumptions - Raise KeyError for non-default missing server keys in LibreNMSAPI Tests: - Add setup_method for consistent _executed flag reset - Assert exception logging in test_exception_does_not_propagate - Fix fragile getLogger assertion in test_no_log_when_field_already_exists
Security:
- Add LibreNMSPermissionMixin to DeviceConflictActionView with write
permission check
- Use SAFE_METHODS instead of hardcoded 'GET' in LibreNMSPluginPermission
- Escape LibreNMS API data in settings test_connection HTML responses (XSS)
Bug fixes:
- Close 4 <form> tags missing '>' after hx-swap attribute in validation
modal (csrf_token and inputs were parsed as tag attributes)
- Use {% url %} for Full Sync Page link instead of string concatenation
(was constructing wrong URL path)
- Remove stray {{ member.name }} text node before <option> in VC member
select modal
- Fix use_sysname fallback default to True (matches model default)
- Reset interface_name at top of delete loop to prevent stale names in
error messages
- Wrap ifAdminStatus in str() to prevent AttributeError on int values
- Use get_object_or_404 for VirtualMachine fallback in AddDeviceToLibreNMS
- Fix test status_code from 500 to 200 to match actual code path tested
- Restore member.serial on validation failure in AssignVCSerialView
- Return explicit bool from create_cable; handle_cable_creation checks
return value and reports failure status
- Include interface name in StopIteration result from
process_single_interface so flash messages show the affected interface
- Wrap Platform create + device assignment in transaction.atomic() in
CreateAndAssignPlatformView to prevent orphaned platforms on save failure
- Move transaction.atomic() to per-interface scope in cable sync so
individual failures roll back only that cable
Code quality:
- Extract _get_librenms_poller_group_choices() shared helper from two
identical form methods; add caching and exception logging
- Consolidate two permission checks into require_all_permissions_json
in DeleteNetBoxInterfacesView
- Save interface_name_field pref only when value differs from stored
- Add error handling to savePref JS (CSRF check, response/fetch errors)
- Use install -d -m 755 instead of mkdir -p -m 755 in setup.sh
- Add view.request mock for DeviceConflictActionView tests
- Update LibreNMSPluginPermission docstring with full permission strings
Docs:
- Fix grammar in README proxy section
- Add missing permission prefixes in permissions docs
- Fix heading spacing and ordered list numbering in permissions docs
Resolve interface_name_field at request time in get_links_data() instead of as a class-level attribute. The class attribute called get_interface_name_field() without a request object, so it always defaulted to ifName regardless of user preference.
Add read-only inventory view that fetches LibreNMS device inventory data and matches items against NetBox module bays and module types. New files: - tables/modules.py: LibreNMSModuleTable with status badges and links - views/base/modules_view.py: BaseModuleTableView with inventory fetch, caching, bay/type matching, and heuristic name matching - templates: _module_sync.html wrapper and _module_sync_content.html Integration: - Add Modules tab to librenms_sync_base.html (conditional on device model) - Add get_module_context() hook to BaseLibreNMSSyncView - Add DeviceModuleTableView and URL pattern for device_module_sync - Matching: module→slot, powerSupply→PS bay, fan→fan bay by name/number
- Add InstallModuleView to create Module in ModuleBay from LibreNMS data - Add Install button (actions column) to module table for matched rows - Fix module countdown timer by adding module-countdown-timer to JS init - Move inline re imports to module level in modules_view.py - Track can_install, module_bay_id, module_type_id in table row data
- Show sub-modules (transceivers, converters, mezzanines) with tree indentation - Recursively collect descendants with models, skipping empty containers - Keep parent-child grouping when sorting by status - Add port-number matching for sub-components (Converter 3/1 ↔ X2 Port 1) - Fix bay number regex to use trailing number (avoid X2 prefix confusion) - Fix install redirect to stay on Modules tab via ?tab=modules - Add icons for port and other inventory classes
Introduces a user-defined mapping table that maps LibreNMS hardware strings to NetBox DeviceType objects. The mapping table is checked first during device type matching, before falling back to exact part_number/model lookup. Includes: - DeviceTypeMapping model with librenms_hardware (unique) -> netbox_device_type FK - CRUD views, forms, filter, table following InterfaceTypeMapping pattern - API serializer/viewset with REST endpoint - Navigation menu item under Settings - Templates for list and detail views - Migration 0009 - Test for mapping-based match (281 tests pass)
ModuleTypeMapping: - Model mapping LibreNMS inventory model names (entPhysicalModelName) to NetBox ModuleType objects - Full CRUD stack: views, forms, filter, table, templates - API serializer/viewset at /api/plugins/librenms_plugin/module-type-mappings/ - Navigation menu item under Settings with Add/Import buttons - Module tab matching updated to check mapping before part_number/model fallback - Migration 0010 InterfaceTypeMapping ordering fix: - Added Meta.ordering to fix API pagination error (QuerySetNotOrdered) Contrib directory: - contrib/README.md with import instructions - contrib/interface_type_mappings.yaml — common interface type examples - contrib/device_type_mappings.yaml — Juniper/Nokia/Cisco device type examples - contrib/module_type_mappings.yaml — module type mapping examples - All files importable via NetBox built-in YAML bulk import
…istics - Add ModuleBayMapping model (librenms_name, librenms_class, netbox_bay_name) - Full CRUD: forms, filters, table, views, URLs, navigation, API - Replace _names_match() heuristics with mapping table lookup - Keep exact parent name match as final fallback - Add contrib/module_bay_mappings.yaml with seed data examples
When LibreNMS hardware string doesn't match any NetBox device type, try chassis entity fields (entPhysicalName, entPhysicalModelName) as additional part_number/model lookups. Handles cases like MX480 where hardware='Juniper MX480 Internet Backbone Router' but chassis entPhysicalName='CHAS-BP-MX480-S' matches the device type part_number. - Add _try_chassis_device_type_match() helper in import_utils.py - Call from validate_device_for_import() when primary match fails - Show match source tooltip in import validation template
Module types with interface templates using {module_path} (e.g., GLC-T,
GLC-TE, X2-10GB-SR) cannot be installed on NetBox < 4.9.0 as the token
would create interfaces with literal '{module_path}' names.
- Add supports_module_path() and module_type_uses_module_path() utilities
- Show 'Requires Upgrade' warning badge in modules table
- Block install with error message when module_path unsupported
- Install button hidden for affected module types
Phase 8: Nested module installation with tree-walk. - InstallBranchView: walks inventory tree depth-first, installs parent first then children into newly-created child bays, applies InterfaceNameRule at each level - 'Install Branch' button on parent items with installable children - Fix URL naming: modulebaymapping_import → modulebaymapping_bulk_import, interfacenamerule_import → interfacenamerule_bulk_import (match convention) - Track entPhysicalIndex and has_installable_children in table row data
Fetch transceiver data from LibreNMS /transceivers endpoint and merge with ENTITY-MIB inventory. Supplements existing inventory items with model/serial from transceiver data when missing. Creates synthetic inventory items for vendors (e.g., Nokia) that report transceivers outside ENTITY-MIB. Filters out container-type entries (Port Container, Port) that carry no useful transceiver data to avoid noise in the modules table.
Extend INVENTORY_CLASSES with Nokia-specific SNMP classes (ioModule, cpmModule, mdaModule, fabricModule, xioModule) so Nokia modules appear in the modules tab. Add item name fallback to module bay matching: when no ModuleBayMapping exists and the parent container name doesn't match, try matching the item's own name against module bays (e.g., 'Slot 1' matches directly). Filter out items whose parent is also in INVENTORY_CLASSES to prevent duplicates when child modules (MDAs inside IOMs) would otherwise appear as both sub-components and top-level entries. Add Nokia-specific icons for vendor SNMP classes in the table renderer. Update contrib YAML examples with Nokia module type mappings (3HE part numbers to SROS board names) and module bay mappings (chassis-prefixed names to NetBox bay names).
- Change _get_module_bays to return (device_bays, module_scoped_bays) tuple to handle duplicate bay names across installed modules (e.g. X2 Port 1-8 exist under both Slot 1 Supervisor and Slot 3 linecard modules) - Sub-components now match against bays belonging to their parent's installed module, not a flat dict that loses duplicates - Add _find_parent_module_id to InstallBranchView for scoped bay lookup during branch install operations - Add #librenms-module-table anchor to all module tab redirects to preserve scroll position after install/branch install actions - Add Cisco X2 Port Container bay mappings to contrib examples
- Replace incorrect Nokia module type mappings (s36-400gb-qsfpdd, xcm-7s-b, xcm2-7s) with correct ones (IOM-s-e, IOM-s, SFM-s) - Add new mappings for CMA2-7s, XIOM-s-3.0t, PS-7750-SR-S-AC, FAN-7750-SR-S - Fix Nokia bay name mappings: Fan Tray→Fan, PSU→PM, CPM→Slot - Add CMA bay mapping for CPM mini-expansion - Add PM 5-10 bay mappings for full power shelf - Add 24 sub-module bay mappings for MDA/XIOM 3-level hierarchy across all 6 IOM slots
- Fix ModuleBayMapping detail URL name (modulebaymapping → modulebaymapping_detail)
- Add depth-scoped bay tracking in _build_context for nested modules
so sub-items match against parent module's child bays, not device-level bays
- Add positional bay matching fallback (_match_bay_by_position) for SFPs
inside converters where containers lack model names
- Fix bay_position resolution when module bay position contains template
expressions like {module} instead of numeric values
- Update Nokia bay mapping names to match actual module bay template output
(remove 'Slot ' prefix, fix MDA x/x1/1 → N/1 for XIOM child bays)
…atching
Adds a reusable NormalizationRule model that applies regex substitution
to input strings before matching lookups. One rule engine serves module
types, device types, and module bays — a single building block that
eliminates the need for hundreds of individual mapping entries.
Integration points:
- Module type matching: normalizes LibreNMS model strings before lookup
in _get_module_types() (modules_view.py)
- Device type matching: normalizes hardware strings as final fallback
in match_librenms_hardware_to_device_type() (utils.py)
Rules chain in priority order: each transforms the output of the
previous rule. Regex patterns are validated on save.
Full CRUD: model, migration (0013), table, forms (create/edit/import/
filter), views, URL patterns, API serializer + viewset, templates,
and navigation menu entry.
Example use case: a single rule with pattern
'^(3HE\w{5}[A-Z]{2})[A-Z]{2}\d{2}$' → '\1'
replaces 9 individual ModuleTypeMapping entries for Nokia devices
by stripping revision suffixes (RA01, RB01, RG01, etc.) so that
LibreNMS model strings match NetBox part numbers directly.
- NormalizationRule: optional manufacturer FK — manufacturer-specific rules run first, then vendor-agnostic rules - apply_normalization_rules(): accepts optional manufacturer parameter - Migration 0014: adds manufacturer column - contrib/normalization_rules.yaml: Nokia suffix, Finisar suffix, Prolabs LGI- prefix, Nokia transceiver model cleanup - contrib/module_type_mappings.yaml: 40+ transceiver vendor→generic mappings (Juniper 740-xxx, Finisar, Cisco OEM, Ciena, T1 Nexus, Innolight, FS.com) - Updated forms, serializer, table, filterset, detail template
…esult The 'found' key was only being set in the else branch when matched=True, causing a KeyError when matched=False. This resulted in devices appearing importable (green) in the UI but failing during actual import with 'Device type is required but not provided'. Move found key assignment to always execute after result['device_type'] is set, and remove the redundant else branch since matched/device_type/ match_type keys already exist in dt_match.
- Include top-level 'port' class items with model names in module table
(Arcos/UfiSpace reports SFP transceivers as entPhysicalClass='port')
- Add module-scoped slot resolution for interface name rules when
module bay has no parent but belongs to an installed module
(fixes X2 transceiver TenGigabitEthernet{slot}/{port} naming)
- Update contrib interface_name_rules.yaml to use Generic module type
names (SFP-1G-T, QSFP-4X10G-LR/SR) and add QSFP-4X10G-SR rule
- Deduplicate transceiver API items by serial number: skip synthetic items when the serial already exists in entity inventory - Walk full ancestor chain when checking if a port item belongs under an INVENTORY_CLASSES parent (fixes Cisco X2 ports appearing both as sub-items of Supervisor AND as top-level items) - Cisco WS-C4900M: 37 rows → 25 rows (12 duplicates eliminated)
- Enhance _merge_transceiver_data to resolve port_id to ifName via ports API - Synthetic transceivers now show interface name (e.g. 1/1/c1) instead of opaque port ID - Add Nokia 3HE part number mappings to existing NetBox types (8 mappings) - Fix MDA-under-XIOM bay mapping (x2/3 format for nested XIOM MDA bays) - Add Nokia MDA/XIOM/connector bay mapping comments
Containers with empty model names (like Cisco 'Slot 1', 'Fan Tray Bay') are physical slot representations, not real modules. Previously these blocked their children from matching device-level bays. Changes: - Filter containers with empty model from top-level items - Skip empty-model containers in ancestor walk so children like Supervisor(slot 1) and Linecard(slot 3) become top-level items and match device bays directly - Cisco WS-C4900M device 14 improved from 75% to 89% match rate - No regression on Juniper/Nokia/UfiSpace devices
…sync - Restore full NormalizationRule CRUD (views, forms, filters, tables, API, templates, navigation, URLs) that was accidentally removed in 9d2ff1b - Add migration 0013 for NormalizationRule model with manufacturer FK - Integrate apply_normalization_rules() into module sync pipeline: both _build_row (display) and _install_single (branch install) now fall back to normalization when direct model lookup fails - Remove 10 redundant ModuleTypeMapping entries (Nokia 3HE part numbers now handled by normalization rules, Juniper exact matches unneeded) - Fix wrong mapping: 3HE12391AARK01 was mapped to m36-800g-qsfpdd, corrected to s36-100gb-qsfp28 (now handled by normalization) - Update contrib/module_type_mappings.yaml to reflect cleanup Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Rename MPC-3D-16XGE-SFPP bays from flat 0/0-0/15 to pic/port format
(Transceiver 0/0 through Transceiver 3/3) matching Juniper ENTITY-MIB
hierarchy (4 PICs x 4 ports per MPC)
- Add regex ModuleBayMapping for Juniper MX SFPs:
entPhysicalDescr 'SFP+-10G-SR @ {fpc}/{pic}/{port}'
-> 'Transceiver {pic}/{port}' in MPC bay
- Create vendor-specific SFP module types (no interface templates
since MX device types define the interfaces):
* Juniper/SFP-1G-T (740-013111)
* Juniper/SFP+-10G-SR (740-031980, covers 740-021308 via mapping)
* Juniper/SFP+-10G-LR (740-031981)
* Avago/SFBR-709SMZ-CS1
* Finisar/FTLX1474D3BCL (normalized from FTLX1474D3BCL-C1)
* Sourcelight/SPP5200LR-C5
- Remove redundant ModuleTypeMappings replaced by direct part_number
lookups or normalization rules
- Fix hostname mismatch detection: compare full FQDN first, fall back
to first-component only when one side has no dots (cherry-pick fix)
All 6 Cisco e2e tests pass.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Change GLC-T/GLC-TE interface template from {module} to {module_path}
so deeply-nested installs (linecard -> converter -> SFP) produce unique
interface names instead of always resolving to the linecard slot position.
- Add INR rules pk=96/97 for GLC-T/GLC-TE on WS-C4900M via CVR-X2-SFP:
GigabitEthernet{slot}/{({parent_bay_position} - 1) * 2 + {sfp_slot}}
This correctly renames GigabitEthernet3/5/1 -> GigabitEthernet3/9 etc.
- Add _fpc_slot_matches() to BaseModuleTableView: when a regex bay mapping
matches an item whose descriptor contains '@ FPC/pic/port', validate that
the matched bay's parent module slot position equals the FPC number.
Prevents orphaned top-level items (e.g. QSFP from uninstalled FPC1)
from incorrectly matching bays on FPC0 and showing Serial Mismatch.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Rename _get_user_pref/_save_user_pref to drop leading underscore (used across modules, not private) - Remove redundant POST save path (save_import_toggle_prefs) from BulkImportConfirmView; keep JS-only persistence via SaveUserPrefView - Extract inline savePref JS from librenms_import.html into librenms_import.js with data-save-pref-url attribute - Add LibreNMSPermissionMixin to SaveUserPrefView - Improve error handling in settings_views.py (catch specific exceptions, log unexpected ones with traceback) - Add tests for SaveUserPrefView (valid prefs, invalid key, invalid JSON, permission mixin inheritance) - Add help text on settings page explaining defaults vs user preferences - Document user preference sync behavior in import_settings.md
- modules_view: wrap int(parent_index) in ValueError guard with redirect - modules_view: apply _fpc_slot_matches in _lookup_regex_bay_mapping install flow - utils: guard re.sub() against re.error in apply_normalization_rules - utils: add MultipleObjectsReturned handler in match_librenms_hardware_to_device_type - cables_view: safe port.get() access instead of dict key lookup - actions: set device_type_synced=False when hw present but unmatched - __init__: hoist import logging to top of try block, remove duplicate imports - forms: scope poller group cache key by LibreNMS server URL - cables: remove unused netbox_remote_device_id from required_fields - tests/test_init: assert info log emitted on custom field creation - interfaces: remove redundant interface.enabled assignment after update_interface_attributes - migrations/0013: remove dead table_exists function - views/__init__: mark re-exports with noqa: F401 - test.yaml: pin actions/checkout and actions/setup-python to SHA - librenms_sync_base.html: guard sysName != "-" before showing Sync button - diagnose.sh: quote $PID and guard against empty PID file - setup.sh: omit plaintext password from superuser creation log - start-netbox.sh: remove unconditional debug echo; SIGTERM before SIGKILL - welcome.sh: fix "you browser" typo Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Extract graceful_kill_pid, graceful_kill_pattern, is_expected_pid into shared process-helpers.sh, sourced by both load-aliases.sh and start-netbox.sh with fail-fast guards - Convert aliases to functions for reliability in non-interactive shells; add $@ passthrough on netbox-test and netbox-manage - Compute PLUGIN_DIR from BASH_SOURCE instead of hardcoding path - Validate PID identity before killing tracked processes (netbox-stop, netbox-status, rq-status) to prevent killing recycled PIDs - Use explicit if/else for uv/pip fallback instead of short-circuit || - Quote all variable expansions in diagnose.sh test commands - Replace eval with bash indirect expansion for CA bundle cleanup - Use two-phase SIGTERM/SIGKILL termination consistently - Normalize superuser credentials via os.environ with .strip()
- __init__.py: hoist import logging before try block to avoid NameError in except - cables_view.py: guard None port_id before str() to prevent bogus 'None' map key - actions.py: validate librenms_id before int() cast; return 400 on invalid/missing - cables.py: per-interface try/except so one failure doesn't abort the whole sync loop - forms.py: server-scope locations cache key by LibreNMS URL (mirrors poller groups fix) - interfaces.py: drop vlan_synced tracking and redundant save (update_interface_attributes already saves) - modules_view.py: guard cache.ttl() with getattr fallback for non-Redis backends - templates: move _module_sync.html into inc/ subfolder; update include path Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Gate handle_mac_address() on "mac_address" not in exclude_columns so the UI's MAC exclude checkbox is respected.
Break the 2,576-line import_utils.py monolith into focused modules: - permissions.py: user permission checks - cache.py: cache key generation and management - filters.py: device filtering and retrieval from LibreNMS - virtual_chassis.py: VC detection, creation, member management - device_operations.py: device validation, import, and fetch - vm_operations.py: VM creation and bulk import - bulk_import.py: bulk device import orchestration and filter processing The __init__.py re-exports all public names, so existing callers (views, jobs, tests) continue working without import changes.
📝 WalkthroughWalkthroughAdds mapping and normalization models with migrations; refactors import utilities into a modular package; implements module inventory sync and installation workflows with UI/API endpoints; enhances devcontainer tooling and process helpers; auto-creates librenms_id post-migrate; adds serializers, forms, views, templates, tests, and contrib YAMLs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Browser as Browser (UI)
participant NetBox as NetBox Server
participant LibreNMS as LibreNMS API
participant DB as Database
Browser->>NetBox: HTMX POST "refresh modules" / "install branch"
NetBox->>LibreNMS: GET /api/devices/{id}/inventory and /transceivers
LibreNMS-->>NetBox: Inventory + transceiver data
NetBox->>NetBox: Merge transceiver data, apply normalization rules & mappings
NetBox->>DB: Query ModuleBay / ModuleType mappings and Module state
NetBox->>NetBox: Build hierarchical module table, determine installable items
NetBox->>DB: Create Module(s), assign ModuleBays (InstallModule/InstallBranch)
DB-->>NetBox: Persist confirmation
NetBox-->>Browser: HTMX response (updated table / modal close)
Browser->>User: UI updates show installed modules / success
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 34
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/usage_tips/permissions.md (1)
22-29:⚠️ Potential issue | 🟡 MinorRestore sequential numbering for clarity.
The two permission tiers are now both numbered "1." instead of sequentially "1." and "2.". While markdown resets numbering after intervening text (line 27), the semantic intent is to show two sequential requirements for completing an action. The current numbering can confuse readers about the relationship between Tier 1 and Tier 2.
📝 Proposed fix to maintain sequential numbering
Restructure to preserve sequential numbering without intervening text:
-1. **Tier 1: Plugin permission**: User needs View AND Change permission on **LibreNMS Settings** +1. **Tier 1: Plugin permission**: User needs View AND Change permission on **LibreNMS Settings**. The plugin also enforces NetBox object permissions, so the following permission is also required. - - View: allows access to the plugin pages and pulling data from LibreNMS. - - Change: allows performing actions that modify Netbox or Librenms data - -The Plugin also enforces Netbox object permissions so the following permission would also be required: - -1. **Tier 2: Object permission**: User needs `dcim.add_device` (to create the device in NetBox) +2. **Tier 2: Object permission**: User needs `dcim.add_device` (to create the device in NetBox)Alternatively, move the View/Change explanation details elsewhere to keep the two-item list clean and sequential.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/usage_tips/permissions.md` around lines 22 - 29, The two top-level permission entries "Tier 1: Plugin permission" and "Tier 2: Object permission" are both rendered as "1." which breaks sequential numbering; update the markdown so the second entry is numbered "2." (or restructure so the View/Change explanation is a nested bullet under "Tier 1: Plugin permission" and keep "Tier 2: Object permission" as the next top-level item) — locate the block containing "Tier 1: Plugin permission" and the following "Tier 2: Object permission" (and the `dcim.add_device` text) and either change the second item's leading "1." to "2." or convert the View/Change lines into sub-bullets under Tier 1 to preserve clear sequential numbering.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.devcontainer/scripts/process-helpers.sh:
- Around line 6-11: The helper function graceful_kill_pid currently performs
signals but never returns a status; modify graceful_kill_pid to return a
meaningful exit code indicating success (process terminated) or failure (still
running or error). After sending SIGTERM and optional SIGKILL, check the process
existence (e.g., via kill -0 or wait) and return 0 when the PID was successfully
terminated and a non-zero value otherwise; preserve existing signaling behavior
in graceful_kill_pid while ensuring callers can inspect the function's exit
status.
In @.github/workflows/lint-format.yaml:
- Around line 21-22: Replace the unpinned Ruff install command so CI uses a
fixed version: change the pip install invocation that currently runs "pip
install ruff" to install a specific version (e.g., "pip install
ruff==<version>") and pick a stable pinned version; ensure the workflow step
that runs "python -m pip install --upgrade pip" is left intact and update any
related matrix or caching notes if present so the pinned Ruff version is used
consistently across CI runs.
In `@netbox_librenms_plugin/api/views.py`:
- Around line 56-89: Add filterset support to the API viewsets by creating and
assigning FilterSet classes for each model and then referencing them via the
filterset_class attribute on DeviceTypeMappingViewSet, ModuleTypeMappingViewSet,
ModuleBayMappingViewSet, and NormalizationRuleViewSet; e.g., implement
DeviceTypeMappingFilterSet, ModuleTypeMappingFilterSet,
ModuleBayMappingFilterSet, and NormalizationRuleFilterSet (defining the fields
you want filterable such as librenms_hardware or other model fields) and set
filterset_class = DeviceTypeMappingFilterSet (and similarly for the other
viewsets) so clients can filter with query params.
In `@netbox_librenms_plugin/filters.py`:
- Around line 46-53: NormalizationRuleFilterSet currently exposes only "scope"
and "manufacturer"; add the "priority" field so consumers can filter/sort by
rule order. Update the NormalizationRuleFilterSet (and its Meta.fields) to
include "priority" and, if you want range queries, add a
django_filters.NumberFilter or RangeFilter for priority on the class (e.g.,
priority = django_filters.RangeFilter() or NumberFilter()) and ensure
django_filters is imported; keep the model set to NormalizationRule and adjust
tests/serializers that depend on available filters if necessary.
- Around line 26-33: ModuleTypeMappingFilterSet's Meta currently lists fields =
["librenms_model", "description"] but omits the foreign-key field
netbox_module_type; update the Meta.fields on the ModuleTypeMappingFilterSet to
include "netbox_module_type" (or add a tuple/list entry for it) so the
ModuleTypeMapping model's FK can be filtered, ensuring you reference the
ModuleTypeMappingFilterSet class and its Meta inner class and the
ModuleTypeMapping model when making the change.
- Around line 16-23: DeviceTypeMappingFilterSet's Meta.fields currently omits
the foreign-key field netbox_device_type so add "netbox_device_type" to the
fields list in the DeviceTypeMappingFilterSet Meta class; update the Meta.fields
declaration (in the DeviceTypeMappingFilterSet class) to include
"netbox_device_type" alongside "librenms_hardware" and "description" so users
can filter by the target NetBox device type.
In `@netbox_librenms_plugin/forms.py`:
- Around line 447-453: The form field named manufacturer_id should be renamed to
manufacturer so it matches NormalizationRuleFilterSet's fields = ["scope",
"manufacturer"]; locate the DynamicModelChoiceField definition (currently
manufacturer_id = DynamicModelChoiceField(...)) and change its attribute name to
manufacturer, leaving the queryset, required and label intact, and then update
any references in the same form class (e.g., validations, initial data, or uses
of manufacturer_id) to use manufacturer instead.
In `@netbox_librenms_plugin/import_utils/bulk_import.py`:
- Around line 416-420: The early-return branches inside the bulk_import.py
function that performs VC prefetch (the block handling exceptions like
BrokenPipeError/ConnectionError/IOError and the other early-return ranges around
lines 437-445, 464-474, 480-483, 525-529) unconditionally return [] and thus
break callers when return_cache_status=True; update each of those early-return
sites to call and return _empty_result() instead of [] so the function
consistently returns the (devices, from_cache) tuple shape when
return_cache_status is requested. Ensure every except/early-exit path in that
function (including the request-handling branch that logs "Client disconnected
during VC prefetch") uses _empty_result() so callers never get the wrong type.
- Around line 288-291: The VM readiness recompute in the import_as_vm branch
uses the wrong prerequisite — it checks validation.get("site", {}) instead of
the cluster mapping; update the logic in the block that sets
validation["is_ready"] (inside the import_as_vm conditional) to require
validation.get("cluster", {}).get("found") and validation.get("device_role",
{}).get("found") so VM imports are only marked ready when a cluster is mapped.
- Around line 307-331: The current lookup in bulk_import.py only tries
dcim.models.Device (variables new_device, match_type) and thus misses
newly-created VirtualMachine records; update the lookup logic so that whenever
Device.objects.filter(...) returns no result (both in the librenms_id branch and
the hostname/sysName branch) you also query virtualization.models.VirtualMachine
with the same filters (by custom_field_data__librenms_id and name__iexact
respectively) and set match_type the same way when a VM is found; reference the
variables/functions new_device, librenms_id, hostname, sys_name, and match_type
and import VirtualMachine from virtualization.models before using it.
- Around line 160-167: The current de-dup key uses device_id so stack members
won't deduplicate; change construction of vc_domain to derive a stack-unique
identifier from the virtual_chassis payload instead of the device id: inspect
vc_data (from validation.get("virtual_chassis")) for a stack identifier field
(e.g. "domain", "id", "vc_id", "stack_id", or a "master" value) and build
vc_domain from that (falling back to device_id only if none exist), then use
processed_vc_domains.add(vc_domain) and the membership check as before; update
the code around vc_data, vc_domain and processed_vc_domains to use this
stack-level key.
- Around line 74-79: The current permission check in bulk_import.py uses the
required_perms list and calls require_permissions(user, required_perms, "import
devices") which improperly requires "dcim.add_virtualchassis" for all imports;
remove "dcim.add_virtualchassis" from the initial required_perms and keep only
the base perms ("dcim.add_device", "dcim.add_interface"), then perform a
separate permission check for "dcim.add_virtualchassis" later in the code path
that handles stacks/virtual-chassis creation (e.g., before calling the virtual
chassis creation logic or the function that performs VC creation), using
require_permissions(user, ["dcim.add_virtualchassis"], "create virtual chassis")
so VC permission is only enforced when VC creation is actually needed.
In `@netbox_librenms_plugin/import_utils/cache.py`:
- Around line 74-78: Wrap the parsing of metadata["cached_at"] inside
get_active_cached_searches() with a try/except that catches TypeError and
ValueError from datetime.fromisoformat; if parsing fails or cached_at is
missing, log a warning (including the offending metadata or key) and skip that
cache entry instead of letting the exception propagate. After successful parse,
ensure the resulting cached_at is timezone-aware (if naive, treat it as UTC via
cached_at.replace(tzinfo=timezone.utc)) before computing age_seconds and
remaining_seconds so the (now - cached_at).total_seconds() calculation remains
correct. Use the existing module logger or processLogger to emit the warning and
continue processing other entries.
- Around line 133-136: Replace the use of Python's non-deterministic built-in
hash() for filter_hash with a deterministic digest (e.g., compute a canonical
JSON string of sorted(filters.items()) and take a sha256/hexdigest) so cache
keys produced in the block that builds
validated_device_{server_key}_{filter_hash}_{device_id}_{vc_part} are stable
across processes; also harden the datetime parsing by guarding the call to
datetime.fromisoformat(metadata.get("cached_at"))—check that cached_at exists
and is a str and wrap parsing in a try/except (or return a safe default/None on
failure) to avoid TypeError for missing or malformed timestamps.
In `@netbox_librenms_plugin/import_utils/device_operations.py`:
- Around line 247-248: Replace direct custom_field_data__librenms_id queries
with the shared accessor by calling LibreNMSAPI.get_librenms_id to obtain the
mapped librenms_id before querying; specifically, change usages inside
device_operations.py where
VirtualMachine.objects.filter(custom_field_data__librenms_id=...) and similar
filters are used (e.g., the existing_vm lookup and the other occurrence around
lines 271-272) to first call LibreNMSAPI.get_librenms_id(obj_or_id) to
retrieve/validate the ID (handle None/invalid returns the same way you currently
handle ValueError/TypeError) and then use that returned integer in the ORM
filter, ensuring all mapping logic flows through LibreNMSAPI.get_librenms_id
instead of touching custom_field_data directly.
- Around line 235-236: The hostname variable (libre_device.get("hostname", ""))
can be None and is later used with hostname.lower(), causing AttributeError;
update the code that sets/uses hostname (the hostname variable in
device_operations.py and the subsequent comparisons around where
hostname.lower() is called) to normalize nullable values by coercing None to an
empty string (e.g., set hostname = (libre_device.get("hostname") or "") or use
(hostname or "").lower() in comparisons) before any .lower() call so the
comparisons never operate on None.
- Around line 676-678: The call to validate_device_for_import in
device_operations.py omits the initialized API client, so pass the api variable
into it (i.e., call validate_device_for_import(libre_device, api=api)) so
chassis-inventory fallback matching and VC detection run; update the invocation
where validation is set (the if block referencing validation,
validate_device_for_import, and libre_device) to include api=api.
In `@netbox_librenms_plugin/import_utils/filters.py`:
- Around line 233-239: The type filter currently uses exact matching while the
os filter uses substring matching, causing inconsistent behavior; update the
filtering logic in the block that handles filters (variables: filters,
device_type, os_filter, filtered) to use the same matching strategy for both
(either make device_type use substring containment like os_filter or make
os_filter use exact equality) and add a short note to the function docstring
clarifying which matching semantics (exact or partial) are applied so callers
know what to expect.
- Line 174: The cache_key uses Python's non-deterministic hash(), causing
inconsistent keys; replace hash(str(api_filters)) and hash(str(client_filters))
with deterministic digests by serializing the filters with json.dumps(...,
sort_keys=True, separators=(',',':')) and computing a stable hex digest (e.g.
via hashlib.sha256(...).hexdigest()); update the construction of cache_key (the
variable named cache_key that uses server_key, api_filters and client_filters)
and add imports for json and hashlib so all workers produce identical keys for
the same filter content.
In `@netbox_librenms_plugin/import_utils/permissions.py`:
- Around line 10-11: The function check_user_permissions currently iterates
permissions directly, which breaks when permissions is None or when a string is
passed; normalize and validate the input first: ensure permissions becomes an
iterable of permission strings (e.g., if permissions is None treat as empty
list, if isinstance(permissions, str) wrap it into [permissions], otherwise
coerce/validate into a list of str), then perform the existing checks against
each permission (use the same internal permission-check logic referenced in
check_user_permissions); apply the same normalization/validation to the other
permission-checking blocks noted around lines 27-28 and 31-47 so you never
iterate raw None or a bare string and only process valid permission strings.
In `@netbox_librenms_plugin/import_utils/virtual_chassis.py`:
- Around line 372-415: The loop over members_info currently only increments the
position counter after a successful Device.objects.create, causing subsequent
members to shift positions when earlier members are skipped; update the loop so
that position is incremented for every iteration of the for member in
members_info loop (immediately or in a finally/at-end-of-iteration step)
regardless of whether the member was skipped due to matching master serial,
duplicate serial check, or duplicate name check, ensuring vc_position passed to
Device.objects.create remains the original intended stack position; refer to the
variables and functions position, members_info, master_device,
Device.objects.create, vc_position, and the skip checks that compare
member.get("serial") and Device.objects.filter(...).exists() when making this
change.
In `@netbox_librenms_plugin/import_utils/vm_operations.py`:
- Around line 180-201: The code only sets role when device_role_id is provided,
dropping any role already resolved in validation; modify the logic so that if
role_id is not set you attempt to read the resolved role from validation (e.g.,
validation.get("role") or validation.role depending on validation's shape) and
assign that to the local role variable before calling create_vm_from_librenms;
ensure you still call apply_role_to_validation when role_id exists and pass the
final role variable into create_vm_from_librenms so auto-detected roles from
validation are preserved.
In `@netbox_librenms_plugin/tables/mappings.py`:
- Line 143: The scope column in mappings.py is defined as scope =
tables.Column(verbose_name="Scope", linkify=True) but scope is a CharField with
choices (a plain string), so linkify=True will cause django-tables2 to call
get_absolute_url() on the string and raise an AttributeError; fix this by
removing linkify=True from the scope Column (or replace it with a proper
callable/url accessor that returns a URL or model instance), updating the scope
Column definition (symbol: scope = tables.Column) accordingly.
In
`@netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html`:
- Around line 100-101: Update each anchor tag that opens a new tab to include
rel="noopener noreferrer": find the <a> elements that use target="_blank" (for
example the link using existing_device_url that renders {{
validation.existing_device.name }}) and add rel="noopener noreferrer" to those
tags; apply the same change to the other target="_blank" anchors in this
template (the anchors referenced in the review: the ones around lines rendering
existing_device_url, the other similar external links at the noted locations) so
all external-tab links use rel="noopener noreferrer".
In `@netbox_librenms_plugin/tests/test_import_utils.py`:
- Around line 2171-2174: The test patches the wrong symbol: _build_sync_info
calls netbox_librenms_plugin.utils.find_matching_platform, not
dcim.models.Platform, so update the patch target in the test to
patch("netbox_librenms_plugin.utils.find_matching_platform") and make that
mock's return_value = new_platform (instead of mocking
mock_platform_cls.objects.get); keep the rest of the assertion flow unchanged so
DeviceValidationDetailsView._build_sync_info(libre_device, existing) uses the
mocked find_matching_platform.
- Around line 2121-2128: The test is patching dcim.models.Platform but
_build_sync_info calls find_matching_platform (which imports Platform inside
netbox_librenms_plugin.utils), so that patch has no effect; update the test to
patch netbox_librenms_plugin.utils.find_matching_platform instead and control
its return value (e.g., return the expected platform object or dict) so
DeviceValidationDetailsView._build_sync_info sees the mocked platform
resolution; also keep the existing patch of
netbox_librenms_plugin.utils.match_librenms_hardware_to_device_type
(mock_hw_match) as before.
In `@netbox_librenms_plugin/tests/test_utils.py`:
- Around line 22-24: Replace the broad assignment mock_mapping.DoesNotExist =
Exception with a test-specific exception type: define a small class (e.g. class
MockDoesNotExist(Exception): pass) inside the test and set
mock_mapping.DoesNotExist = MockDoesNotExist, then use
mock_mapping.objects.get.side_effect = mock_mapping.DoesNotExist so the test
raises the specific mock exception; apply the same change for the other
occurrences that set DoesNotExist (the blocks around mock_mapping at the other
comment locations).
In `@netbox_librenms_plugin/utils.py`:
- Around line 219-230: The except block swallowing
DeviceTypeMapping.MultipleObjectsReturned should emit a warning to surface this
data-integrity issue: catch DeviceTypeMapping.MultipleObjectsReturned in the try
around DeviceTypeMapping.objects.get(librenms_hardware__iexact=hardware_name)
and call the module logger (e.g., logger.warning) including the problematic
hardware_name and context (e.g., "Multiple DeviceTypeMapping entries for
librenms_hardware=%s") so ops can investigate, then continue returning no
mapping as before.
In `@netbox_librenms_plugin/views/base/librenms_sync_view.py`:
- Around line 235-236: Remove the redundant re-assignment of the variable
found_in_librenms in the mismatch branch; since found_in_librenms is already set
to True earlier in the successful fetch path, delete the extra
"found_in_librenms = True" statement in the mismatch handling block so only the
original assignment remains and behavior is unchanged.
In `@netbox_librenms_plugin/views/base/modules_view.py`:
- Line 44: partial_template_name is pointing to
netbox_librenms_plugin/_module_sync_content.html but HTMX fragments must live
under the htmx fragment tree; update the value of partial_template_name to the
new htmx path (move the template file into
templates/netbox_librenms_plugin/htmx/ and set partial_template_name
accordingly) so the view (partial_template_name) references the HTMX fragment in
the htmx/ directory.
- Around line 994-1013: The parent-module resolution is ignoring nested
installed modules and can pick the wrong bay mapping; update the ModuleBay
queries and mapping lookup so nested installs are considered and mappings are
disambiguated: remove the module_id__isnull=True filter from the ModuleBay.query
used both in the initial device_bays loop and in the mapping-driven lookup (so
installed_module on nested bays is not excluded), and tighten the
ModuleBayMapping lookup by adding a stronger disambiguator (e.g., include a
variant/device/netbox_bay_name constraint available on ModuleBayMapping) instead
of querying only librenms_name; finally, when you locate a mapping use
ModuleBay.objects.filter(device=device,
name=mapping.netbox_bay_name).select_related("installed_module").first() and
ensure you return bay.installed_module.pk only when installed_module exists.
In `@tests/e2e/test_module_install.py`:
- Around line 54-69: In _netbox_shell, the subprocess.run call that invokes
docker exec (using variables container and escaped) doesn't handle failures or
timeouts; modify the call to include a sensible timeout and after it returns
check result.returncode, and if non-zero raise a clear exception that includes
result.stderr and result.stdout (or log them) so callers can fail fast and tests
don't hang or continue on bad state; ensure the filtering of result.stdout
remains but only runs after the success check.
- Around line 133-140: Multiple hardcoded time.sleep() calls in
tests/e2e/test_module_install.py (e.g., around the "Refresh Modules" click where
btn is found via page.query_selector('button:has-text("Refresh Modules")'))
cause flakiness; replace each fixed sleep (lines noted in the review) with
condition-based waits such as page.wait_for_selector or locator.wait_for for the
specific elements that indicate readiness, or use
page.wait_for_load_state("networkidle") / page.wait_for_response for
network-dependent steps, and after actions like btn.click() wait for the
expected UI change (new element, text, or navigation) rather than
sleeping—update every occurrence (the btn click block and the other 11 sleep
usages) to use these explicit waits so tests become deterministic.
- Around line 36-45: The current logic uses subprocess.run to list Docker
containers and only picks a container whose name contains the hardcoded
substring "devcontainer-devcontainer", then calls pytest.skip if none found,
which can silently skip valid E2E runs; update the selection in the
container-discovery block (the place that sets CONTAINER_NAME) to accept an
environment variable override (e.g., read DOCKER_E2E_CONTAINER), fallback to
matching with a configurable/looser pattern or regex (not the exact
"devcontainer-devcontainer" literal), and if still none found raise an explicit
error (or fail the test) instead of calling pytest.skip so failures are visible;
keep the subprocess.run usage but broaden matching and add the env-var check
around CONTAINER_NAME and pytest.skip.
---
Outside diff comments:
In `@docs/usage_tips/permissions.md`:
- Around line 22-29: The two top-level permission entries "Tier 1: Plugin
permission" and "Tier 2: Object permission" are both rendered as "1." which
breaks sequential numbering; update the markdown so the second entry is numbered
"2." (or restructure so the View/Change explanation is a nested bullet under
"Tier 1: Plugin permission" and keep "Tier 2: Object permission" as the next
top-level item) — locate the block containing "Tier 1: Plugin permission" and
the following "Tier 2: Object permission" (and the `dcim.add_device` text) and
either change the second item's leading "1." to "2." or convert the View/Change
lines into sub-bullets under Tier 1 to preserve clear sequential numbering.
ℹ️ Review info
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (83)
.devcontainer/README.md.devcontainer/scripts/diagnose.sh.devcontainer/scripts/load-aliases.sh.devcontainer/scripts/process-helpers.sh.devcontainer/scripts/setup.sh.devcontainer/scripts/start-netbox.sh.devcontainer/scripts/welcome.sh.github/workflows/lint-format.yaml.github/workflows/test.yamlcontrib/README.mdcontrib/device_type_mappings.yamlcontrib/interface_name_rules.yamlcontrib/interface_type_mappings.yamlcontrib/module_bay_mappings.yamlcontrib/module_type_mappings.yamlcontrib/normalization_rules.yamldocs/usage_tips/custom_field.mddocs/usage_tips/permissions.mdnetbox_librenms_plugin/__init__.pynetbox_librenms_plugin/api/serializers.pynetbox_librenms_plugin/api/urls.pynetbox_librenms_plugin/api/views.pynetbox_librenms_plugin/filters.pynetbox_librenms_plugin/forms.pynetbox_librenms_plugin/import_utils.pynetbox_librenms_plugin/import_utils/__init__.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/cache.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/import_utils/filters.pynetbox_librenms_plugin/import_utils/permissions.pynetbox_librenms_plugin/import_utils/virtual_chassis.pynetbox_librenms_plugin/import_utils/vm_operations.pynetbox_librenms_plugin/librenms_api.pynetbox_librenms_plugin/migrations/0009_add_devicetypemapping.pynetbox_librenms_plugin/migrations/0010_add_moduletypemapping.pynetbox_librenms_plugin/migrations/0011_modulebaymapping.pynetbox_librenms_plugin/migrations/0012_add_is_regex_to_modulebaymapping.pynetbox_librenms_plugin/migrations/0013_normalizationrule.pynetbox_librenms_plugin/models.pynetbox_librenms_plugin/navigation.pynetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.jsnetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.jsnetbox_librenms_plugin/tables/device_status.pynetbox_librenms_plugin/tables/mappings.pynetbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_module_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/devicetypemapping.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/devicetypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/inc/_module_sync.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_import.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/modulebaymapping.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/modulebaymapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/moduletypemapping.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/moduletypemapping_list.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/normalizationrule.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/normalizationrule_list.htmlnetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_init.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/urls.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/__init__.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/imports/__init__.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/mapping_views.pynetbox_librenms_plugin/views/object_sync/__init__.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/views/sync/cables.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/views/sync/devices.pynetbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/views/sync/locations.pytests/e2e/__init__.pytests/e2e/conftest.pytests/e2e/test_module_install.py
💤 Files with no reviewable changes (3)
- netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html
- netbox_librenms_plugin/views/sync/locations.py
- netbox_librenms_plugin/import_utils.py
Comment on lines
+6
to
+11
| graceful_kill_pid() { | ||
| local pid="$1" | ||
| kill -15 "$pid" 2>/dev/null | ||
| sleep 2 | ||
| kill -0 "$pid" 2>/dev/null && kill -9 "$pid" 2>/dev/null | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider returning an exit status for callers that need it.
Currently graceful_kill_pid doesn't indicate success/failure. While not critical for current usage, a return value could be useful for future callers that need to know if the process was successfully terminated.
♻️ Optional: Add return status
graceful_kill_pid() {
local pid="$1"
kill -15 "$pid" 2>/dev/null
sleep 2
- kill -0 "$pid" 2>/dev/null && kill -9 "$pid" 2>/dev/null
+ if kill -0 "$pid" 2>/dev/null; then
+ kill -9 "$pid" 2>/dev/null
+ fi
+ ! kill -0 "$pid" 2>/dev/null # return 0 if process is gone
}📝 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
| graceful_kill_pid() { | |
| local pid="$1" | |
| kill -15 "$pid" 2>/dev/null | |
| sleep 2 | |
| kill -0 "$pid" 2>/dev/null && kill -9 "$pid" 2>/dev/null | |
| } | |
| graceful_kill_pid() { | |
| local pid="$1" | |
| kill -15 "$pid" 2>/dev/null | |
| sleep 2 | |
| if kill -0 "$pid" 2>/dev/null; then | |
| kill -9 "$pid" 2>/dev/null | |
| fi | |
| ! kill -0 "$pid" 2>/dev/null # return 0 if process is gone | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.devcontainer/scripts/process-helpers.sh around lines 6 - 11, The helper
function graceful_kill_pid currently performs signals but never returns a
status; modify graceful_kill_pid to return a meaningful exit code indicating
success (process terminated) or failure (still running or error). After sending
SIGTERM and optional SIGKILL, check the process existence (e.g., via kill -0 or
wait) and return 0 when the PID was successfully terminated and a non-zero value
otherwise; preserve existing signaling behavior in graceful_kill_pid while
ensuring callers can inspect the function's exit status.
Comment on lines
21
to
22
| python -m pip install --upgrade pip | ||
| pip install ruff |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
latest version of Ruff Python linter 2026
💡 Result:
As of February 26, 2026, the latest released version of the Ruff Python linter on PyPI is 0.15.1 (released February 12, 2026). [1]
Pin the Ruff version for reproducible CI results.
Installing ruff without a version pin means CI behavior could change unexpectedly when Ruff releases new versions with different rules or defaults. Pin to a specific version:
📌 Proposed fix to pin Ruff version
run: |
python -m pip install --upgrade pip
- pip install ruff
+ pip install ruff==0.15.1📝 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
| python -m pip install --upgrade pip | |
| pip install ruff | |
| python -m pip install --upgrade pip | |
| pip install ruff==0.15.1 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/lint-format.yaml around lines 21 - 22, Replace the
unpinned Ruff install command so CI uses a fixed version: change the pip install
invocation that currently runs "pip install ruff" to install a specific version
(e.g., "pip install ruff==<version>") and pick a stable pinned version; ensure
the workflow step that runs "python -m pip install --upgrade pip" is left intact
and update any related matrix or caching notes if present so the pinned Ruff
version is used consistently across CI runs.
Comment on lines
+56
to
+89
| class DeviceTypeMappingViewSet(NetBoxModelViewSet): | ||
| """API viewset for DeviceTypeMapping CRUD operations.""" | ||
|
|
||
| permission_classes = [LibreNMSPluginPermission] | ||
|
|
||
| queryset = DeviceTypeMapping.objects.all() | ||
| serializer_class = DeviceTypeMappingSerializer | ||
|
|
||
|
|
||
| class ModuleTypeMappingViewSet(NetBoxModelViewSet): | ||
| """API viewset for ModuleTypeMapping CRUD operations.""" | ||
|
|
||
| permission_classes = [LibreNMSPluginPermission] | ||
|
|
||
| queryset = ModuleTypeMapping.objects.all() | ||
| serializer_class = ModuleTypeMappingSerializer | ||
|
|
||
|
|
||
| class ModuleBayMappingViewSet(NetBoxModelViewSet): | ||
| """API viewset for ModuleBayMapping CRUD operations.""" | ||
|
|
||
| permission_classes = [LibreNMSPluginPermission] | ||
|
|
||
| queryset = ModuleBayMapping.objects.all() | ||
| serializer_class = ModuleBayMappingSerializer | ||
|
|
||
|
|
||
| class NormalizationRuleViewSet(NetBoxModelViewSet): | ||
| """API viewset for NormalizationRule CRUD operations.""" | ||
|
|
||
| permission_classes = [LibreNMSPluginPermission] | ||
|
|
||
| queryset = NormalizationRule.objects.all() | ||
| serializer_class = NormalizationRuleSerializer |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding filterset classes for API query filtering.
The viewsets don't define filterset_class, which means API consumers can't filter results by specific fields. If filtering is needed (e.g., ?librenms_hardware=...), consider adding filtersets.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@netbox_librenms_plugin/api/views.py` around lines 56 - 89, Add filterset
support to the API viewsets by creating and assigning FilterSet classes for each
model and then referencing them via the filterset_class attribute on
DeviceTypeMappingViewSet, ModuleTypeMappingViewSet, ModuleBayMappingViewSet, and
NormalizationRuleViewSet; e.g., implement DeviceTypeMappingFilterSet,
ModuleTypeMappingFilterSet, ModuleBayMappingFilterSet, and
NormalizationRuleFilterSet (defining the fields you want filterable such as
librenms_hardware or other model fields) and set filterset_class =
DeviceTypeMappingFilterSet (and similarly for the other viewsets) so clients can
filter with query params.
Comment on lines
+16
to
+23
| class DeviceTypeMappingFilterSet(django_filters.FilterSet): | ||
| """Filter set for DeviceTypeMapping model.""" | ||
|
|
||
| class Meta: | ||
| """Meta options for DeviceTypeMappingFilterSet.""" | ||
|
|
||
| model = DeviceTypeMapping | ||
| fields = ["librenms_hardware", "description"] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding the FK field to the filter.
DeviceTypeMappingFilterSet is missing netbox_device_type from the filterable fields. Users may want to filter mappings by the target NetBox device type.
♻️ Suggested enhancement
class DeviceTypeMappingFilterSet(django_filters.FilterSet):
"""Filter set for DeviceTypeMapping model."""
class Meta:
"""Meta options for DeviceTypeMappingFilterSet."""
model = DeviceTypeMapping
- fields = ["librenms_hardware", "description"]
+ fields = ["librenms_hardware", "netbox_device_type", "description"]📝 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
| class DeviceTypeMappingFilterSet(django_filters.FilterSet): | |
| """Filter set for DeviceTypeMapping model.""" | |
| class Meta: | |
| """Meta options for DeviceTypeMappingFilterSet.""" | |
| model = DeviceTypeMapping | |
| fields = ["librenms_hardware", "description"] | |
| class DeviceTypeMappingFilterSet(django_filters.FilterSet): | |
| """Filter set for DeviceTypeMapping model.""" | |
| class Meta: | |
| """Meta options for DeviceTypeMappingFilterSet.""" | |
| model = DeviceTypeMapping | |
| fields = ["librenms_hardware", "netbox_device_type", "description"] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@netbox_librenms_plugin/filters.py` around lines 16 - 23,
DeviceTypeMappingFilterSet's Meta.fields currently omits the foreign-key field
netbox_device_type so add "netbox_device_type" to the fields list in the
DeviceTypeMappingFilterSet Meta class; update the Meta.fields declaration (in
the DeviceTypeMappingFilterSet class) to include "netbox_device_type" alongside
"librenms_hardware" and "description" so users can filter by the target NetBox
device type.
Comment on lines
+26
to
+33
| class ModuleTypeMappingFilterSet(django_filters.FilterSet): | ||
| """Filter set for ModuleTypeMapping model.""" | ||
|
|
||
| class Meta: | ||
| """Meta options for ModuleTypeMappingFilterSet.""" | ||
|
|
||
| model = ModuleTypeMapping | ||
| fields = ["librenms_model", "description"] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding the FK field to the filter.
ModuleTypeMappingFilterSet is missing netbox_module_type from the filterable fields.
♻️ Suggested enhancement
class ModuleTypeMappingFilterSet(django_filters.FilterSet):
"""Filter set for ModuleTypeMapping model."""
class Meta:
"""Meta options for ModuleTypeMappingFilterSet."""
model = ModuleTypeMapping
- fields = ["librenms_model", "description"]
+ fields = ["librenms_model", "netbox_module_type", "description"]📝 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
| class ModuleTypeMappingFilterSet(django_filters.FilterSet): | |
| """Filter set for ModuleTypeMapping model.""" | |
| class Meta: | |
| """Meta options for ModuleTypeMappingFilterSet.""" | |
| model = ModuleTypeMapping | |
| fields = ["librenms_model", "description"] | |
| class ModuleTypeMappingFilterSet(django_filters.FilterSet): | |
| """Filter set for ModuleTypeMapping model.""" | |
| class Meta: | |
| """Meta options for ModuleTypeMappingFilterSet.""" | |
| model = ModuleTypeMapping | |
| fields = ["librenms_model", "netbox_module_type", "description"] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@netbox_librenms_plugin/filters.py` around lines 26 - 33,
ModuleTypeMappingFilterSet's Meta currently lists fields = ["librenms_model",
"description"] but omits the foreign-key field netbox_module_type; update the
Meta.fields on the ModuleTypeMappingFilterSet to include "netbox_module_type"
(or add a tuple/list entry for it) so the ModuleTypeMapping model's FK can be
filtered, ensuring you reference the ModuleTypeMappingFilterSet class and its
Meta inner class and the ModuleTypeMapping model when making the change.
| """ | ||
|
|
||
| model = None | ||
| partial_template_name = "netbox_librenms_plugin/_module_sync_content.html" |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Move HTMX partial template path under the htmx/ fragment directory.
partial_template_name currently points to netbox_librenms_plugin/_module_sync_content.html; this fragment should live under the HTMX fragment tree.
As per coding guidelines: netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/**/*.html: HTMX fragments should live in templates/netbox_librenms_plugin/htmx/. Keep server responses and HTMX targets in sync.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@netbox_librenms_plugin/views/base/modules_view.py` at line 44,
partial_template_name is pointing to
netbox_librenms_plugin/_module_sync_content.html but HTMX fragments must live
under the htmx fragment tree; update the value of partial_template_name to the
new htmx path (move the template file into
templates/netbox_librenms_plugin/htmx/ and set partial_template_name
accordingly) so the view (partial_template_name) references the HTMX fragment in
the htmx/ directory.
Comment on lines
+994
to
+1013
| device_bays = ModuleBay.objects.filter(device=device, module_id__isnull=True).select_related( | ||
| "installed_module" | ||
| ) | ||
|
|
||
| for bay in device_bays: | ||
| if hasattr(bay, "installed_module") and bay.installed_module: | ||
| if bay.name == parent_name or (parent_descr and bay.name == parent_descr): | ||
| return bay.installed_module.pk | ||
|
|
||
| # Also check ModuleBayMapping for indirect matches | ||
| for name in [parent_name, parent_descr]: | ||
| if not name: | ||
| continue | ||
| mapping = ModuleBayMapping.objects.filter(librenms_name=name).first() | ||
| if mapping: | ||
| bay = ( | ||
| ModuleBay.objects.filter(device=device, name=mapping.netbox_bay_name, module_id__isnull=True) | ||
| .select_related("installed_module") | ||
| .first() | ||
| ) |
There was a problem hiding this comment.
Parent-module resolution can break nested branch installs.
Line 994 and Line 1010 restrict lookup to module_id__isnull=True, so installed nested modules are ignored. Also, Line 1007 uses an overly broad mapping lookup (librenms_name only), which can resolve the wrong bay mapping variant.
Proposed fix
- device_bays = ModuleBay.objects.filter(device=device, module_id__isnull=True).select_related(
+ device_bays = ModuleBay.objects.filter(device=device).select_related(
"installed_module"
)
@@
- mapping = ModuleBayMapping.objects.filter(librenms_name=name).first()
+ mapping = (
+ ModuleBayMapping.objects.filter(
+ librenms_name=name,
+ is_regex=False,
+ )
+ .order_by("pk")
+ .first()
+ )
if mapping:
bay = (
- ModuleBay.objects.filter(device=device, name=mapping.netbox_bay_name, module_id__isnull=True)
+ ModuleBay.objects.filter(device=device, name=mapping.netbox_bay_name)
.select_related("installed_module")
.first()
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@netbox_librenms_plugin/views/base/modules_view.py` around lines 994 - 1013,
The parent-module resolution is ignoring nested installed modules and can pick
the wrong bay mapping; update the ModuleBay queries and mapping lookup so nested
installs are considered and mappings are disambiguated: remove the
module_id__isnull=True filter from the ModuleBay.query used both in the initial
device_bays loop and in the mapping-driven lookup (so installed_module on nested
bays is not excluded), and tighten the ModuleBayMapping lookup by adding a
stronger disambiguator (e.g., include a variant/device/netbox_bay_name
constraint available on ModuleBayMapping) instead of querying only
librenms_name; finally, when you locate a mapping use
ModuleBay.objects.filter(device=device,
name=mapping.netbox_bay_name).select_related("installed_module").first() and
ensure you return bay.installed_module.pk only when installed_module exists.
Comment on lines
+36
to
+45
| result = subprocess.run( | ||
| ["docker", "ps", "--format", "{{.Names}}"], | ||
| capture_output=True, | ||
| text=True, | ||
| ) | ||
| for name in result.stdout.strip().split("\n"): | ||
| if "devcontainer-devcontainer" in name: | ||
| CONTAINER_NAME = name | ||
| return name | ||
| pytest.skip("No devcontainer found") |
There was a problem hiding this comment.
Avoid silently skipping the E2E suite due to hardcoded container naming.
Line 42 matches only one container-name pattern, and Line 45 skips the whole module if it is not found. That can produce false-green E2E runs in valid environments with different names.
Proposed hardening
+CONTAINER_NAME_ENV = os.environ.get("NETBOX_DEVCONTAINER_NAME")
+
def _get_container():
"""Find the devcontainer name."""
global CONTAINER_NAME
if CONTAINER_NAME:
return CONTAINER_NAME
+ if CONTAINER_NAME_ENV:
+ CONTAINER_NAME = CONTAINER_NAME_ENV
+ return CONTAINER_NAME
result = subprocess.run(
["docker", "ps", "--format", "{{.Names}}"],
capture_output=True,
text=True,
)
+ if result.returncode != 0:
+ pytest.fail(f"docker ps failed: {result.stderr.strip()}")
for name in result.stdout.strip().split("\n"):
if "devcontainer-devcontainer" in name:
CONTAINER_NAME = name
return name
- pytest.skip("No devcontainer found")
+ pytest.fail("No devcontainer found. Set NETBOX_DEVCONTAINER_NAME explicitly.")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/test_module_install.py` around lines 36 - 45, The current logic
uses subprocess.run to list Docker containers and only picks a container whose
name contains the hardcoded substring "devcontainer-devcontainer", then calls
pytest.skip if none found, which can silently skip valid E2E runs; update the
selection in the container-discovery block (the place that sets CONTAINER_NAME)
to accept an environment variable override (e.g., read DOCKER_E2E_CONTAINER),
fallback to matching with a configurable/looser pattern or regex (not the exact
"devcontainer-devcontainer" literal), and if still none found raise an explicit
error (or fail the test) instead of calling pytest.skip so failures are visible;
keep the subprocess.run usage but broaden matching and add the env-var check
around CONTAINER_NAME and pytest.skip.
Comment on lines
+54
to
+69
| result = subprocess.run( | ||
| [ | ||
| "docker", | ||
| "exec", | ||
| container, | ||
| "bash", | ||
| "-c", | ||
| f"cd /opt/netbox/netbox && python3 manage.py shell -c {escaped}", | ||
| ], | ||
| capture_output=True, | ||
| text=True, | ||
| env={"PATH": "/usr/bin:/bin", "HOME": "/root"}, | ||
| ) | ||
| # Filter out config loading lines | ||
| lines = [line for line in result.stdout.strip().split("\n") if not line.startswith(("🧬", "156 objects"))] | ||
| return "\n".join(lines).strip() |
There was a problem hiding this comment.
Handle docker exec failures explicitly in _netbox_shell.
Line 54 executes a critical command but never checks returncode and has no timeout. A hung/failing shell call can stall or corrupt assertions downstream.
Proposed hardening
result = subprocess.run(
[
"docker",
"exec",
container,
"bash",
"-c",
f"cd /opt/netbox/netbox && python3 manage.py shell -c {escaped}",
],
capture_output=True,
text=True,
env={"PATH": "/usr/bin:/bin", "HOME": "/root"},
+ timeout=120,
)
+ if result.returncode != 0:
+ pytest.fail(f"netbox shell failed (rc={result.returncode}): {result.stderr.strip()}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/test_module_install.py` around lines 54 - 69, In _netbox_shell, the
subprocess.run call that invokes docker exec (using variables container and
escaped) doesn't handle failures or timeouts; modify the call to include a
sensible timeout and after it returns check result.returncode, and if non-zero
raise a clear exception that includes result.stderr and result.stdout (or log
them) so callers can fail fast and tests don't hang or continue on bad state;
ensure the filtering of result.stdout remains but only runs after the success
check.
Comment on lines
+133
to
+140
| time.sleep(2) | ||
|
|
||
| # Click Refresh Modules | ||
| btn = page.query_selector('button:has-text("Refresh Modules")') | ||
| assert btn is not None, "Refresh Modules button not found" | ||
| btn.click() | ||
| time.sleep(8) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C1 'time\.sleep\(' tests/e2e/test_module_install.pyRepository: marcinpsk/netbox-librenms-plugin
Length of output: 1186
Replace fixed sleeps with condition-based waits to reduce flakiness.
This file contains 12 hardcoded time.sleep() calls (lines 133, 139, 188, 215, 217, 247, 249, 286, 297, 301, 312, 316) ranging from 2 to 20 seconds. Mixed with Playwright's proper wait methods like page.wait_for_load_state("networkidle"), this creates unpredictable test duration and flakiness across varying CI environments. Replace fixed delays with explicit element waits or load state checks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/test_module_install.py` around lines 133 - 140, Multiple hardcoded
time.sleep() calls in tests/e2e/test_module_install.py (e.g., around the
"Refresh Modules" click where btn is found via
page.query_selector('button:has-text("Refresh Modules")')) cause flakiness;
replace each fixed sleep (lines noted in the review) with condition-based waits
such as page.wait_for_selector or locator.wait_for for the specific elements
that indicate readiness, or use page.wait_for_load_state("networkidle") /
page.wait_for_response for network-dependent steps, and after actions like
btn.click() wait for the expected UI change (new element, text, or navigation)
rather than sleeping—update every occurrence (the btn click block and the other
11 sleep usages) to use these explicit waits so tests become deterministic.
After splitting import_utils.py into a package, mock.patch decorators must target the actual submodule where each name is looked up, not the package __init__.py. Update all patch paths in test_import_utils.py and test_permissions.py to point to the correct submodules.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
netbox_librenms_plugin/tests/test_import_utils.py (2)
2121-2128: 🧹 Nitpick | 🔵 TrivialPatch location may be incorrect for
Platform.The test patches
dcim.models.Platformbut if_build_sync_infousesfind_matching_platformfromnetbox_librenms_plugin.utils(which importsPlatforminternally), this patch won't have the intended effect. Consider patchingnetbox_librenms_plugin.utils.find_matching_platforminstead.Suggested fix
with ( - patch("dcim.models.Platform") as mock_platform_cls, + patch("netbox_librenms_plugin.utils.find_matching_platform") as mock_find_platform, patch("netbox_librenms_plugin.utils.match_librenms_hardware_to_device_type") as mock_hw_match, ): - mock_platform_cls.objects.get.return_value = platform + mock_find_platform.return_value = {"found": True, "platform": platform, "match_type": "exact"} mock_hw_match.return_value = {"matched": True, "device_type": device_type}Based on learnings: "Patch deferred/inline imports at their source module (e.g.,
netbox_librenms_plugin.import_utils.process_device_filters), not the consuming module".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/tests/test_import_utils.py` around lines 2121 - 2128, The test currently patches dcim.models.Platform but _build_sync_info calls find_matching_platform from netbox_librenms_plugin.utils which imports Platform internally, so the patch has no effect; update the test to patch netbox_librenms_plugin.utils.find_matching_platform (or the Platform symbol at netbox_librenms_plugin.utils where it is used) and have that mock return the desired platform object, keeping the existing mock for match_librenms_hardware_to_device_type and asserting DeviceValidationDetailsView._build_sync_info behaves as expected.
2171-2174: 🧹 Nitpick | 🔵 TrivialSame patch location issue for platform sync test.
This test also patches
dcim.models.Platformbut should patchnetbox_librenms_plugin.utils.find_matching_platformif that's how the platform lookup is performed in_build_sync_info.Suggested fix
- with patch("dcim.models.Platform") as mock_platform_cls: - mock_platform_cls.objects.get.return_value = new_platform + with patch("netbox_librenms_plugin.utils.find_matching_platform") as mock_find_platform: + mock_find_platform.return_value = {"found": True, "platform": new_platform, "match_type": "exact"}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/tests/test_import_utils.py` around lines 2171 - 2174, In the DeviceValidationDetailsView._build_sync_info test, replace the incorrect patch target "dcim.models.Platform" with the actual helper used by the function: patch "netbox_librenms_plugin.utils.find_matching_platform" (or patch the function name exactly as imported in the module under test) so the test stubs the platform lookup correctly; update the mock to return new_platform via the patched find_matching_platform and keep the call to DeviceValidationDetailsView._build_sync_info(libre_device, existing) unchanged.netbox_librenms_plugin/import_utils/device_operations.py (2)
235-237:⚠️ Potential issue | 🟡 MinorNormalize nullable hostname before
.lower()comparisons.Line 236 uses
get("hostname", "")which returns""if the key is missing, but returnsNoneif the key exists with a null value. Line 385 then callshostname.lower(), which raisesAttributeErrorifhostnameisNone.Proposed fix
- hostname = libre_device.get("hostname", "") + hostname = (libre_device.get("hostname") or "").strip()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/import_utils/device_operations.py` around lines 235 - 237, The hostname variable may be None when libre_device contains a null "hostname", causing hostname.lower() to raise; update the assignment of hostname (in device_operations.py where librenms_id and hostname are set) to normalize nulls to an empty string (e.g., use the truthy fallback pattern) so all downstream comparisons and calls like hostname.lower() are safe; ensure any other places that read libre_device["hostname"] follow the same normalization pattern (refer to the hostname and libre_device variables and the block around their assignment).
676-679:⚠️ Potential issue | 🟠 MajorPass the initialized API client into validation.
Line 678 calls
validate_device_for_import(libre_device)withoutapi=api, so chassis-inventory fallback matching and VC detection are skipped despite having a ready client initialized at line 662.Proposed fix
# Validate device if validation not provided if validation is None: - validation = validate_device_for_import(libre_device) + validation = validate_device_for_import(libre_device, api=api)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/import_utils/device_operations.py` around lines 676 - 679, The validation call is missing the initialized API client, so update the call to validate_device_for_import to pass the api variable (e.g., change validate_device_for_import(libre_device) to validate_device_for_import(libre_device, api=api)) so chassis-inventory fallback matching and VC detection (in validate_device_for_import) use the ready client initialized earlier (api variable); ensure the api variable referenced is the same one created near the earlier initialization.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@netbox_librenms_plugin/import_utils/device_operations.py`:
- Around line 420-423: Remove the duplicate inline import of Cluster inside the
block guarded by import_as_vm; the module already imports Cluster at the top, so
delete the line "from virtualization.models import Cluster" within the
import_as_vm conditional (the code surrounding the import_as_vm check and VM
validation logic) to avoid shadowing and Flake8 F811.
---
Duplicate comments:
In `@netbox_librenms_plugin/import_utils/device_operations.py`:
- Around line 235-237: The hostname variable may be None when libre_device
contains a null "hostname", causing hostname.lower() to raise; update the
assignment of hostname (in device_operations.py where librenms_id and hostname
are set) to normalize nulls to an empty string (e.g., use the truthy fallback
pattern) so all downstream comparisons and calls like hostname.lower() are safe;
ensure any other places that read libre_device["hostname"] follow the same
normalization pattern (refer to the hostname and libre_device variables and the
block around their assignment).
- Around line 676-679: The validation call is missing the initialized API
client, so update the call to validate_device_for_import to pass the api
variable (e.g., change validate_device_for_import(libre_device) to
validate_device_for_import(libre_device, api=api)) so chassis-inventory fallback
matching and VC detection (in validate_device_for_import) use the ready client
initialized earlier (api variable); ensure the api variable referenced is the
same one created near the earlier initialization.
In `@netbox_librenms_plugin/tests/test_import_utils.py`:
- Around line 2121-2128: The test currently patches dcim.models.Platform but
_build_sync_info calls find_matching_platform from netbox_librenms_plugin.utils
which imports Platform internally, so the patch has no effect; update the test
to patch netbox_librenms_plugin.utils.find_matching_platform (or the Platform
symbol at netbox_librenms_plugin.utils where it is used) and have that mock
return the desired platform object, keeping the existing mock for
match_librenms_hardware_to_device_type and asserting
DeviceValidationDetailsView._build_sync_info behaves as expected.
- Around line 2171-2174: In the DeviceValidationDetailsView._build_sync_info
test, replace the incorrect patch target "dcim.models.Platform" with the actual
helper used by the function: patch
"netbox_librenms_plugin.utils.find_matching_platform" (or patch the function
name exactly as imported in the module under test) so the test stubs the
platform lookup correctly; update the mock to return new_platform via the
patched find_matching_platform and keep the call to
DeviceValidationDetailsView._build_sync_info(libre_device, existing) unchanged.
ℹ️ Review info
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
netbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_permissions.py
📜 Review details
⏰ 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: test-netbox (3.14)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Reuse thelibrenms_api.pyLibreNMS client instead of making newrequestscalls; it handles multi-server configs viaLibreNMSSettingsmodel and theserversplugin config, plus caching
Always callLibreNMSAPI.get_librenms_idto retrieve the device/VM LibreNMS mapping via thelibrenms_idcustom field instead of touching the field directly
Matching must be exact-only for site, platform, device type, and role; do not add fuzzy matching; use the functionsfind_matching_site,match_librenms_hardware_to_device_type, andfind_matching_platformfromutils.py
Files:
netbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_permissions.py
🧠 Learnings (9)
📓 Common learnings
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-02-15T15:39:20.744Z
Learning: Applies to tests/**/*.py : Patch deferred/inline imports at their source module (e.g., `netbox_librenms_plugin.import_utils.process_device_filters`), not the consuming module
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-02-15T15:39:20.744Z
Learning: Applies to tests/**/*.py : Test coverage mapping: `librenms_api.py` → `test_librenms_api.py`, `import_utils.py`/`import_validation_helpers.py`/`utils.py` → `test_import_utils.py`/`test_import_validation_helpers.py`/`test_utils.py`, `jobs.py`/`views/imports/list.py` → `test_background_jobs.py`
📚 Learning: 2026-02-15T15:39:20.744Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-02-15T15:39:20.744Z
Learning: Applies to tests/**/*.py : Patch deferred/inline imports at their source module (e.g., `netbox_librenms_plugin.import_utils.process_device_filters`), not the consuming module
Applied to files:
netbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_permissions.py
📚 Learning: 2026-02-15T15:38:54.189Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-15T15:38:54.189Z
Learning: Applies to views/imports/**/*.py : REST endpoints for imports live in `views/imports/actions.py` with the list view in `views/imports/list.py`, surface via `urls.py`, and emit HTMX fragments in `templates/netbox_librenms_plugin/htmx/`; keep server responses and HTMX targets in sync
Applied to files:
netbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tests/test_import_utils.py
📚 Learning: 2026-02-15T15:38:54.189Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-15T15:38:54.189Z
Learning: Applies to **/*.py : Always call `LibreNMSAPI.get_librenms_id` to retrieve the device/VM LibreNMS mapping via the `librenms_id` custom field instead of touching the field directly
Applied to files:
netbox_librenms_plugin/import_utils/device_operations.py
📚 Learning: 2026-02-15T15:39:20.744Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-02-15T15:39:20.744Z
Learning: Applies to tests/test_background_jobs.py : Patch `cache` where imported: `netbox_librenms_plugin.views.imports.list.cache`, not `django.core.cache.cache`
Applied to files:
netbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_permissions.py
📚 Learning: 2026-02-15T15:39:20.744Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-02-15T15:39:20.744Z
Learning: Applies to tests/test_import_utils.py : Cache key tests must patch `get_validated_device_cache_key` from `import_utils.py`; never hardcode key formats like `job_123_device_1`
Applied to files:
netbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_permissions.py
📚 Learning: 2026-02-15T15:39:20.744Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-02-15T15:39:20.744Z
Learning: Applies to tests/**/*.py : Test coverage mapping: `librenms_api.py` → `test_librenms_api.py`, `import_utils.py`/`import_validation_helpers.py`/`utils.py` → `test_import_utils.py`/`test_import_validation_helpers.py`/`test_utils.py`, `jobs.py`/`views/imports/list.py` → `test_background_jobs.py`
Applied to files:
netbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_permissions.py
📚 Learning: 2026-02-15T15:39:20.744Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-02-15T15:39:20.744Z
Learning: Applies to tests/**/*.py : Mock NetBox models (Device, Job, User) with `MagicMock()` instead of creating real instances
Applied to files:
netbox_librenms_plugin/tests/test_import_utils.py
📚 Learning: 2026-02-15T15:38:54.189Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-15T15:38:54.189Z
Learning: Applies to **/*.py : Matching must be exact-only for site, platform, device type, and role; do not add fuzzy matching; use the functions `find_matching_site`, `match_librenms_hardware_to_device_type`, and `find_matching_platform` from `utils.py`
Applied to files:
netbox_librenms_plugin/tests/test_import_utils.py
🧬 Code graph analysis (2)
netbox_librenms_plugin/import_utils/device_operations.py (5)
netbox_librenms_plugin/views/mixins.py (1)
librenms_api(217-231)netbox_librenms_plugin/librenms_api.py (2)
LibreNMSAPI(15-1080)get_device_info(310-333)netbox_librenms_plugin/utils.py (3)
find_matching_platform(297-327)find_matching_site(263-294)match_librenms_hardware_to_device_type(195-260)netbox_librenms_plugin/import_utils/cache.py (1)
get_import_device_cache_key(139-158)netbox_librenms_plugin/import_utils/virtual_chassis.py (2)
empty_virtual_chassis_data(15-23)get_virtual_chassis_data(60-80)
netbox_librenms_plugin/tests/test_import_utils.py (2)
netbox_librenms_plugin/import_utils/device_operations.py (1)
validate_device_for_import(117-615)netbox_librenms_plugin/views/imports/actions.py (11)
get(669-689)get(695-719)DeviceConflictActionView(819-1005)post(213-360)post(387-663)post(783-790)post(796-803)post(809-816)post(822-1005)post(1017-1031)_build_sync_info(722-777)
🪛 Flake8 (7.3.0)
netbox_librenms_plugin/import_utils/device_operations.py
[error] 422-422: redefinition of unused 'Cluster' from line 7
(F811)
🔇 Additional comments (15)
netbox_librenms_plugin/import_utils/device_operations.py (5)
1-21: LGTM!Imports are well-organized. The
Clusterimport withnoqa: F401is intentional for test mock.patch targeting.
23-77: LGTM!Well-designed helper with proper handling of edge cases (IP address detection, empty names, domain stripping).
79-115: LGTM!Good fallback matching strategy using chassis inventory fields, with proper exception handling.
740-790: LGTM!Device creation within atomic transaction is properly implemented. Setting
custom_field_dataduring creation is appropriate.
833-907: LGTM!Both helper functions are well-designed with proper caching strategy and error handling.
netbox_librenms_plugin/tests/test_import_utils.py (6)
14-66: LGTM!Cache key generation tests are well-structured with appropriate assertions.
73-142: LGTM!Comprehensive test coverage for device name determination including edge cases.
149-291: LGTM!Patch targets correctly updated to reference the specific submodules (
filters,virtual_chassis) where the dependencies are imported. This aligns with the learning to patch deferred/inline imports at their source module.
298-1104: LGTM!Test patch targets correctly updated to reference
device_operationssubmodule. The pattern follows best practices for patching at the source module where imports occur.
1107-1609: LGTM!Well-organized test class with proper fixture pattern for common patches. Comprehensive coverage of serial number matching scenarios including conflicts, drift detection, and device type mismatch.
1611-2100: LGTM!Comprehensive test coverage for
DeviceConflictActionViewwith all conflict resolution actions properly tested.netbox_librenms_plugin/tests/test_permissions.py (4)
4-198: LGTM!Permission mixin and API permission tests are well-structured with comprehensive coverage.
633-727: LGTM!Patch targets correctly updated to reference the specific submodules (
bulk_import,vm_operations) whererequire_permissionsandLibreNMSAPIare imported. This follows the best practice of patching at the source module.
729-770: LGTM!Patch target correctly updated to
permissions.check_user_permissionsat the source module.
873-896: LGTM!Patch targets correctly updated for VC permission tests.
The module-level import already provides Cluster; the lazy import inside the import_as_vm block was shadowing it unnecessarily.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
netbox_librenms_plugin/import_utils/device_operations.py (1)
508-509: 🧹 Nitpick | 🔵 TrivialRemove redundant
Rackimport.
Rackis already imported at module level (line 5). This inline import shadows it unnecessarily. Consider movingQto module-level imports as well for consistency.Proposed fix
if available_racks is None: - from dcim.models import Rack from django.db.models import QAnd add at top of file with other Django imports:
from django.db.models import Q🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netbox_librenms_plugin/import_utils/device_operations.py` around lines 508 - 509, Remove the redundant inline import "from dcim.models import Rack" (it shadows the module-level Rack) and eliminate the inline "from django.db.models import Q"; instead add "Q" to the module-level imports alongside the existing Django imports so all imports are consistent and non-shadowing; update any references in this file to continue using the already-imported Rack symbol and the newly-promoted Q.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@netbox_librenms_plugin/import_utils/device_operations.py`:
- Around line 793-811: Placeholder syncs for interfaces, cables, and IPs in the
try block (controlled by sync_options and logging via logger for device) are not
implemented and the synced counts remain zero; update the code by (1) replacing
the placeholder logger.info calls inside the if blocks for sync_interfaces,
sync_cables, and sync_ips with calls to the actual sync helper functions (or
clearly stubbed helpers) that perform the real operations and return counts, (2)
update the synced dict to aggregate those returned counts so it reflects actual
results, and (3) if implementation must be deferred, add explicit TODO comments
and create/attach a tracking issue reference (or call an issue-creation helper)
so this missing work is recorded; locate and modify the blocks referencing
sync_options, logger, device, and the synced dict to apply these changes.
---
Duplicate comments:
In `@netbox_librenms_plugin/import_utils/device_operations.py`:
- Around line 508-509: Remove the redundant inline import "from dcim.models
import Rack" (it shadows the module-level Rack) and eliminate the inline "from
django.db.models import Q"; instead add "Q" to the module-level imports
alongside the existing Django imports so all imports are consistent and
non-shadowing; update any references in this file to continue using the
already-imported Rack symbol and the newly-promoted Q.
ℹ️ Review info
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
netbox_librenms_plugin/import_utils/device_operations.py
📜 Review details
⏰ 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: test-netbox (3.14)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Reuse thelibrenms_api.pyLibreNMS client instead of making newrequestscalls; it handles multi-server configs viaLibreNMSSettingsmodel and theserversplugin config, plus caching
Always callLibreNMSAPI.get_librenms_idto retrieve the device/VM LibreNMS mapping via thelibrenms_idcustom field instead of touching the field directly
Matching must be exact-only for site, platform, device type, and role; do not add fuzzy matching; use the functionsfind_matching_site,match_librenms_hardware_to_device_type, andfind_matching_platformfromutils.py
Files:
netbox_librenms_plugin/import_utils/device_operations.py
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-02-15T15:39:20.744Z
Learning: Applies to tests/**/*.py : Patch deferred/inline imports at their source module (e.g., `netbox_librenms_plugin.import_utils.process_device_filters`), not the consuming module
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-15T15:38:54.189Z
Learning: Follow sync pipeline flow: fetch LibreNMS data via `librenms_api.py`, cache it using `CacheMixin`, build comparison tables in `tables/`, and render HTMX fragments in `templates/netbox_librenms_plugin/htmx/`
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-15T15:38:54.189Z
Learning: Applies to views/imports/**/*.py : REST endpoints for imports live in `views/imports/actions.py` with the list view in `views/imports/list.py`, surface via `urls.py`, and emit HTMX fragments in `templates/netbox_librenms_plugin/htmx/`; keep server responses and HTMX targets in sync
📚 Learning: 2026-02-15T15:39:20.744Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/instructions/testing.instructions.md:0-0
Timestamp: 2026-02-15T15:39:20.744Z
Learning: Applies to tests/**/*.py : Patch deferred/inline imports at their source module (e.g., `netbox_librenms_plugin.import_utils.process_device_filters`), not the consuming module
Applied to files:
netbox_librenms_plugin/import_utils/device_operations.py
📚 Learning: 2026-02-15T15:38:54.189Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-15T15:38:54.189Z
Learning: Applies to views/imports/**/*.py : REST endpoints for imports live in `views/imports/actions.py` with the list view in `views/imports/list.py`, surface via `urls.py`, and emit HTMX fragments in `templates/netbox_librenms_plugin/htmx/`; keep server responses and HTMX targets in sync
Applied to files:
netbox_librenms_plugin/import_utils/device_operations.py
📚 Learning: 2026-02-15T15:38:54.189Z
Learnt from: CR
Repo: marcinpsk/netbox-librenms-plugin PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-15T15:38:54.189Z
Learning: Applies to **/*.py : Always call `LibreNMSAPI.get_librenms_id` to retrieve the device/VM LibreNMS mapping via the `librenms_id` custom field instead of touching the field directly
Applied to files:
netbox_librenms_plugin/import_utils/device_operations.py
🧬 Code graph analysis (1)
netbox_librenms_plugin/import_utils/device_operations.py (4)
netbox_librenms_plugin/librenms_api.py (2)
LibreNMSAPI(15-1080)get_device_info(310-333)netbox_librenms_plugin/utils.py (3)
find_matching_platform(297-327)find_matching_site(263-294)match_librenms_hardware_to_device_type(195-260)netbox_librenms_plugin/import_utils/cache.py (1)
get_import_device_cache_key(139-158)netbox_librenms_plugin/import_utils/virtual_chassis.py (2)
empty_virtual_chassis_data(15-23)get_virtual_chassis_data(60-80)
🔇 Additional comments (9)
netbox_librenms_plugin/import_utils/device_operations.py (9)
1-21: LGTM on imports structure.The module-level imports are well-organized with clear grouping. Note that
Rackis imported here at line 5 but also re-imported insidevalidate_device_for_importat line 508—the inner import is redundant and can be removed.
23-77: LGTM!The centralized name determination logic correctly handles the sysName/hostname preference, includes proper IP address detection to avoid stripping domains from IP addresses, and provides sensible fallbacks.
79-115: LGTM!The chassis inventory fallback correctly uses
match_librenms_hardware_to_device_typefromutils.pyper coding guidelines. Exception handling with debug-level logging is appropriate for this optional enhancement path.
235-241: Normalize nullable hostname before.lower()comparisons.Line 236 can leave
hostnameasNonewhen the key exists with a null value (.get("hostname", "")returns the null value, not the default). Line 385 then callshostname.lower(), which raisesAttributeError.Proposed fix
- hostname = libre_device.get("hostname", "") + hostname = (libre_device.get("hostname") or "").strip()
247-252: Consider centralizinglibrenms_idlookup patterns.The direct
custom_field_data__librenms_idfilter queries here and at line 272 work correctly for reverse lookups (finding NetBox objects by LibreNMS ID). However, for consistency and maintainability, consider extracting this pattern into a shared utility—this would make it easier to adapt if the custom field storage mechanism changes.As per coding guidelines: "Always call
LibreNMSAPI.get_librenms_idto retrieve the device/VM LibreNMS mapping via thelibrenms_idcustom field instead of touching the field directly." While this guideline targets reading the field from a NetBox object, centralizing all librenms_id access patterns would improve consistency.
542-566: LGTM on virtual chassis detection integration.The VC detection section correctly respects the
include_vc_detectionflag, properly passesforce_refresh, and has appropriate debug-level logging for troubleshooting. Exception handling captures errors without blocking the validation flow.
675-677: Pass the initialized API client into validation.Line 676 calls
validate_device_for_import(libre_device)withoutapi=api, so chassis-inventory fallback matching and VC detection are skipped despite having a ready client initialized at line 660.Proposed fix
- validation = validate_device_for_import(libre_device) + validation = validate_device_for_import(libre_device, api=api)
831-853: LGTM!Clean wrapper function that properly delegates to the
LibreNMSAPIclient per coding guidelines, with appropriate logging for debugging.
855-906: LGTM on caching pattern.The three-tier cache lookup (pre-fetched dict → Django cache → API fallback) is well-designed and properly consolidates the device fetching pattern. Good use of
get_import_device_cache_keyfrom the cache module for consistent key generation.
Comment on lines
+793
to
+811
| try: | ||
| # Sync interfaces | ||
| if sync_options.get("sync_interfaces", True): | ||
| # This is simplified - would need proper request context | ||
| # For now, just log that it should be done | ||
| logger.info(f"Interface sync should be performed for device {device.name}") | ||
|
|
||
| # Sync cables | ||
| if sync_options.get("sync_cables", True): | ||
| logger.info(f"Cable sync should be performed for device {device.name}") | ||
|
|
||
| # Sync IP addresses | ||
| if sync_options.get("sync_ips", True): | ||
| logger.info(f"IP address sync should be performed for device {device.name}") | ||
|
|
||
| except Exception as e: | ||
| logger.warning(f"Error during post-import sync: {str(e)}") | ||
| # Don't fail the import if sync fails | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Sync operations are placeholders.
The interface, cable, and IP sync sections currently only log messages without actual sync implementation. The synced dict always returns zero counts. This appears intentional based on the comment, but verify this is tracked for follow-up implementation.
Would you like me to open an issue to track implementing the actual sync operations here, or is this handled elsewhere in the codebase?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@netbox_librenms_plugin/import_utils/device_operations.py` around lines 793 -
811, Placeholder syncs for interfaces, cables, and IPs in the try block
(controlled by sync_options and logging via logger for device) are not
implemented and the synced counts remain zero; update the code by (1) replacing
the placeholder logger.info calls inside the if blocks for sync_interfaces,
sync_cables, and sync_ips with calls to the actual sync helper functions (or
clearly stubbed helpers) that perform the real operations and return counts, (2)
update the synced dict to aggregate those returned counts so it reflects actual
results, and (3) if implementation must be deferred, add explicit TODO comments
and create/attach a tracking issue reference (or call an issue-creation helper)
so this missing work is recorded; locate and modify the blocks referencing
sync_options, logger, device, and the synced dict to apply these changes.
marcinpsk
marked this pull request as draft
March 1, 2026 12:31
marcinpsk
marked this pull request as ready for review
March 1, 2026 14:57
marcinpsk
added a commit
that referenced
this pull request
Jun 1, 2026
- utils.set_librenms_device_id: use coerce_librenms_id so float-like IDs (1.9) and bools are rejected instead of silently truncated (#3). - migrate.MoveInterfaceToWinnerView: lock + re-read the donor interface row inside the txn and use its locked name for the collision check, closing a concurrent-rename TOCTOU (the IP-move flow already did this) (#5). - device_operations: populate existing_librenms_link on the primary-IP match path so an already-linked device no longer renders as 'not linked' (#6). - _oob_interface_select.html: add a label for the new-interface name input (#8). - ip_addresses_view: resolve the management IP once on the fresh-fetch path and cache it; flagging no longer makes a live LibreNMS call on cached renders (#12). - sync/ip_addresses: rebuild the API client scoped to the POST server_key so the management-IP lookup hits the same server the cached rows came from (#13). - _dt_mapping_form.html: drop the unneeded CSRF header from the read-only GET (#1). - tests: stale promote_to_host clearing regression (#2); assert same-name interface lookup args (#9); distinct locked-row mocks in OOB transfer (#10). Skipped: #4 (obsolete - auto_create_ipam removed), #7 (hx-include targets the wrapper span which still includes the -cb checkbox), #11 (no VC-with-OOB config in practice).
marcinpsk
added a commit
that referenced
this pull request
Jun 1, 2026
- utils.set_librenms_device_id: use coerce_librenms_id so float-like IDs (1.9) and bools are rejected instead of silently truncated (#3). - migrate.MoveInterfaceToWinnerView: lock + re-read the donor interface row inside the txn and use its locked name for the collision check, closing a concurrent-rename TOCTOU (the IP-move flow already did this) (#5). - device_operations: populate existing_librenms_link on the primary-IP match path so an already-linked device no longer renders as 'not linked' (#6). - _oob_interface_select.html: add a label for the new-interface name input (#8). - ip_addresses_view: resolve the management IP once on the fresh-fetch path and cache it; flagging no longer makes a live LibreNMS call on cached renders (#12). - sync/ip_addresses: rebuild the API client scoped to the POST server_key so the management-IP lookup hits the same server the cached rows came from (#13). - _dt_mapping_form.html: drop the unneeded CSRF header from the read-only GET (#1). - tests: stale promote_to_host clearing regression (#2); assert same-name interface lookup args (#9); distinct locked-row mocks in OOB transfer (#10). Skipped: #4 (obsolete - auto_create_ipam removed), #7 (hx-include targets the wrapper span which still includes the -cb checkbox), #11 (no VC-with-OOB config in practice).
marcinpsk
added a commit
that referenced
this pull request
Jun 1, 2026
- utils.set_librenms_device_id: use coerce_librenms_id so float-like IDs (1.9) and bools are rejected instead of silently truncated (#3). - migrate.MoveInterfaceToWinnerView: lock + re-read the donor interface row inside the txn and use its locked name for the collision check, closing a concurrent-rename TOCTOU (the IP-move flow already did this) (#5). - device_operations: populate existing_librenms_link on the primary-IP match path so an already-linked device no longer renders as 'not linked' (#6). - _oob_interface_select.html: add a label for the new-interface name input (#8). - ip_addresses_view: resolve the management IP once on the fresh-fetch path and cache it; flagging no longer makes a live LibreNMS call on cached renders (#12). - sync/ip_addresses: rebuild the API client scoped to the POST server_key so the management-IP lookup hits the same server the cached rows came from (#13). - _dt_mapping_form.html: drop the unneeded CSRF header from the read-only GET (#1). - tests: stale promote_to_host clearing regression (#2); assert same-name interface lookup args (#9); distinct locked-row mocks in OOB transfer (#10). Skipped: #4 (obsolete - auto_create_ipam removed), #7 (hx-include targets the wrapper span which still includes the -cb checkbox), #11 (no VC-with-OOB config in practice).
marcinpsk
added a commit
that referenced
this pull request
Jun 1, 2026
- utils.set_librenms_device_id: use coerce_librenms_id so float-like IDs (1.9) and bools are rejected instead of silently truncated (#3). - migrate.MoveInterfaceToWinnerView: lock + re-read the donor interface row inside the txn and use its locked name for the collision check, closing a concurrent-rename TOCTOU (the IP-move flow already did this) (#5). - device_operations: populate existing_librenms_link on the primary-IP match path so an already-linked device no longer renders as 'not linked' (#6). - _oob_interface_select.html: add a label for the new-interface name input (#8). - ip_addresses_view: resolve the management IP once on the fresh-fetch path and cache it; flagging no longer makes a live LibreNMS call on cached renders (#12). - sync/ip_addresses: rebuild the API client scoped to the POST server_key so the management-IP lookup hits the same server the cached rows came from (#13). - _dt_mapping_form.html: drop the unneeded CSRF header from the read-only GET (#1). - tests: stale promote_to_host clearing regression (#2); assert same-name interface lookup args (#9); distinct locked-row mocks in OOB transfer (#10). Skipped: #4 (obsolete - auto_create_ipam removed), #7 (hx-include targets the wrapper span which still includes the -cb checkbox), #11 (no VC-with-OOB config in practice).
marcinpsk
added a commit
that referenced
this pull request
Jun 2, 2026
- utils.set_librenms_device_id: use coerce_librenms_id so float-like IDs (1.9) and bools are rejected instead of silently truncated (#3). - migrate.MoveInterfaceToWinnerView: lock + re-read the donor interface row inside the txn and use its locked name for the collision check, closing a concurrent-rename TOCTOU (the IP-move flow already did this) (#5). - device_operations: populate existing_librenms_link on the primary-IP match path so an already-linked device no longer renders as 'not linked' (#6). - _oob_interface_select.html: add a label for the new-interface name input (#8). - ip_addresses_view: resolve the management IP once on the fresh-fetch path and cache it; flagging no longer makes a live LibreNMS call on cached renders (#12). - sync/ip_addresses: rebuild the API client scoped to the POST server_key so the management-IP lookup hits the same server the cached rows came from (#13). - _dt_mapping_form.html: drop the unneeded CSRF header from the read-only GET (#1). - tests: stale promote_to_host clearing regression (#2); assert same-name interface lookup args (#9); distinct locked-row mocks in OOB transfer (#10). Skipped: #4 (obsolete - auto_create_ipam removed), #7 (hx-include targets the wrapper span which still includes the -cb checkbox), #11 (no VC-with-OOB config in practice).
marcinpsk
added a commit
that referenced
this pull request
Jun 24, 2026
Two develop-owned correctness/consistency findings from the max-effort review (#11 declined — see below): - BaseIPAddressTableView._prepare_context: gate the cached branch on isinstance(cached_ip_data, dict) instead of a bare truthy check. A stale/corrupt non-dict cache entry (legacy snapshot shape) otherwise 500s on .get() against a list/str; now it drops to None and renders empty, matching the interfaces/modules cached-path guards. Red→green test. - SyncIPAddressesView.get_management_ip: guard the ip type explicitly (like _resolve_management_ip) rather than letting a non-string ip raise into the broad defensive except. Outcome is unchanged (None for a non-string ip) — this is a consistency hardening so the catch-all isn't used for expected input; contract tests pin the non-string→None and string→stripped behaviour. Declined: the _normalize_librenms_id "regression" (#11) is a false positive — on develop/this branch it's still the lenient int() form; the strict coerce_librenms_id wrapper lives only on the feature stack and is an intentional hardening (LibreNMS device ids are positive integers, so rejecting 0/negative/ float is correct, not a regression). Claude-Session: https://claude.ai/code/session_01RKRVyWizgrukFSHmTF168J
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Briefly describe what this PR does in plain English, and provide as much of the following information as possible.
Motivation / Problem
What issue does this solve?
Link any related issues if applicable.
Scope of Change
Tick all that apply:
How Was This Tested?
Tick all that apply and describe briefly.
Manual Test Steps (if applicable)
Risk Assessment
Explain briefly.
Backwards Compatibility
Other Notes
Anything the maintainer(s) should pay particular attention to?
Summary by CodeRabbit
New Features
Documentation