Conversation
|
This fix also worked for me. Anything missing in order to merge this? Thanks! |
mmguero
pushed a commit
to mmguero-dev/Device-Type-Library-Import
that referenced
this pull request
Sep 14, 2026
…ommunity#130) * fix(netbox): name the item NetBox rejected in a bulk create pynetbox sets `RequestError.error` to `response.text`, which is always a string. The `isinstance(excep.error, list)` branch in `_create_generic` was therefore unreachable, and every rejected bulk create fell through to the whole-batch message. NetBox rejects the entire POST when one item is invalid, so the log named all submitted components: Error '[{"rf_role":[...]},{},{},{}]' creating Interface. Items: ['Uplink', 'Downlink 2.5G', 'Downlink 1', 'Downlink 2'] when only 'Uplink' was at fault. Decode the response body and map the per-item error array onto the submitted items by position, which is how NetBox returns it. Fall back to the whole-batch message when the payload is not a list, when its length does not match the batch, or when no item carries a message, so a failure is never swallowed. `_extract_error_payload` already did this decoding for the subdevice_role classifier; promote it to `extract_error_payload` and share it instead of adding a second decoder. The previous tests built the error from a MagicMock response and then assigned `err.error = [...]` by hand, fabricating a shape pynetbox never produces, and asserted only that something was logged. Rebuild them on a real `requests.Response` and assert the message content. * feat(report): name the vendor in each change-detection banner A run loops over vendors and prints one report per vendor, so consecutive banners were indistinguishable: three "CHANGE DETECTION REPORT" blocks in a row gave no clue which vendor each covered. Append the vendor to the banner title, on the title line itself: CHANGE DETECTION REPORT: TP-Link MODULE TYPE CHANGE DETECTION: TP-Link RACK TYPE CHANGE DETECTION: TP-Link The rack-type banner had no title line at all, only a bare count block, so it gains one for consistency. The vendor argument defaults to empty and the titles then render exactly as before, which keeps every existing caller and the integration-test banner matching intact. * fix(report): count device-type creation failures in the run report Only the update path recorded an outcome. A device type rejected at creation was logged inline and then vanished: it was missing from the FAILED / PARTIAL UPDATE REPORT and absent from the device_types_failed count, so a run could report fewer failures than it actually had. Record a FAILED outcome for both creation failure paths, the API rejection and the exhausted connection retry, and carry the source file as the hint so the operator can find the definition. A parsed YAML dict carries only the manufacturer slug, so the identity reads "ribbon/SBC 5400" where the update path can use the NetBox manufacturer name. * fix(report): record rack-type and module-type failures in the run report Extends the previous commit to the remaining unreported failure paths. Rack types recorded no outcome at all, so a rejected rack type was logged inline and then missing from the summary: Error creating Rack Type: {"description":["Ensure this field has no more than 200 characters."]} (Context: rack-types/Lenovo/1410HPB.yaml) Record a FAILED outcome for the rack-type create and update paths and for the module-type create path, both the API rejection and the exhausted connection retry, carrying the source file as the hint. Generalise the device-type-only helper into _record_failure(kind, identity, reason, src_file) with an optional counter key, plus _yaml_identity for the parsed dicts that carry only a manufacturer slug. The module-type update path is left alone: _process_single_module_type already records an outcome when _try_update_module_type returns False, and recording there too produced two rows and a double count for one failure. * fix(report): carry the component error into the device-type reason Rack and module failures report NetBox's own message, but a device type whose component changes failed reported only the generic label: ✗ [device_type] TP-Link/EAP670 reason: Component updates failed. The real message was logged inline during the run and then discarded, so the summary could not say what went wrong. Component failures now go through _log_component_error, which logs as before and keeps the message. create_device_types drains the buffer around the component work and passes it to the outcome, which prefers it over the generic label and appends it to a partial-update reason. Up to three messages are joined, the rest counted: ✗ [device_type] TP-Link/EAP670 reason: Failed to create Interface 'LAN': {'rf_role': ['Wireless role may be set only on wireless interfaces.']} (Context: EAP670.yaml) A recognised constraint from the failure resolver still wins, since it already carries a description, blocking objects and a hint. * fix(report): route component transport failures into the outcome reason The RequestError branches for a component update and removal already went through _log_component_error, but their sibling retry-exhausted branches still called handle.log directly. A connection failure was logged and then lost, so the device-type outcome fell back to "Component updates failed." instead of naming the transport error. The create path already routed its retryable branch; this makes the update and removal paths match. * fix(report): route skipped-component reasons into the outcome reason Completes the class the previous commit started. A component change can be dropped during link resolution before any API call, and those reasons were logged and then lost, so the device-type outcome fell back to the generic "Component updates failed." The clearest case is a power outlet whose power_port names a port that does not exist. The outlet is removed from the batch and the run reports only the generic label, when it could name the reference: reason: Could not find Power Port "hardwired" for Power Outlet "Hardwire". Available: ['Input'] (Context: Online-3000.yaml) Routed through _log_component_error: unresolvable power-port and rear-port references, all three front-port mapping resolution failures, and the three bridge failures. Left on handle.log deliberately: the "Removed N" success line, the two "Skipped N ..." summaries whose per-item detail is already routed, the NetBox < 4.5 multiple-mapping notice where the port is still created, and the image upload errors, which belong to a different entity. * refactor(report): derive run-summary failure counts from the outcome registry The summary counts came from the legacy Counter while the itemised report came from the OutcomeRegistry, with nothing keeping the two consistent. They had drifted in three directions: * a module type that failed to be created recorded an outcome but bumped no counter, so it was listed in the report and missing from the counts; * a partially updated module bumped a counter but recorded no outcome, so it was counted in the headline and never itemised; * a failed rack type had no counter and no summary line at all. The registry already holds one row per entity with its kind and outcome, so the counts are derivable from it. RunSummary now carries outcome_counts alongside failure_lines, both taken from the registry at the same instant, and the summary derives every failure and partial line from that aggregate. The headline can no longer disagree with the rows beneath it. Record the two missing module PARTIAL outcomes, then delete the parallel device_types_failed, module_update_failed and module_partial_update keys and the now-unused counter_key argument on _record_failure. Rack types gain their missing line from rows that already existed. The created/updated and component-volume keys stay on the Counter: they are volume tallies rather than per-entity outcomes, and the registry does not hold them. Tests assert the invariant the two tallies never enforced: the headline counts match the itemised rows. * fix(report): scope component errors per entity and count partial device types Two review findings, both from the same seam. The summary read only Outcome.FAILED for device types, so a device type that ended PARTIAL was itemised in the report with no matching headline count. The previous commit added that line for modules and missed it for device types. Add it, and assert the invariant for partials as well as failures. DeviceTypes._component_errors was drained by one of the four paths that produce component errors. New device types and new module types never consumed it, so their component failures reached no outcome, and module reasons carried none of the buffered NetBox detail. The buffer's interface carried an unwritten "clear before, read after" contract that only one caller honoured. Replace take_errors() with the collect_component_errors() context manager, so clearing on entry and collecting on exit are structural rather than remembered, and one entity's failures cannot surface in another's reason. Scope all four paths through it. A new device type or module type whose components partly fail now records Outcome.PARTIAL instead of reporting as cleanly created, and module reasons carry the same NetBox detail device-type reasons already did. * fix(report): stop calling create failures update failures The headline failure lines predate this branch, when Outcome.FAILED could only come from an update. This branch added create-failure recording for device types (core/netbox_api.py:1108, :1121) and module types (:1790, :1801), so both counts now include creates while the text still read "FAILED to update". A Ribbon device type rejected at create time was reported as a failed update, pointing the operator at the wrong operation. Both lines now say "failed to create or update". The two new tests drive real NetBox rejections through create_device_types and _process_single_module_type, then render the real RunSummary through a real LogHandler, so the wording is asserted against the path that actually produces the count. Moves the recording console into tests/helpers.py: both test modules need it now, and one copy keeps them from drifting.
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
4 participants
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.
resolve #117