EDM-4980: fix inventory crash on mount-only application volumes - #63
Conversation
The flightctl-client SDK's pydantic model for ApplicationVolume declares image as required, but RHEM returns mount-only volumes without image. Add a fallback that catches pydantic ValidationError and retries with raw JSON endpoints, bypassing pydantic deserialization. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
WalkthroughFleet retrieval now supports model objects and dictionaries. Typed SDK validation failures can use raw JSON pagination with continuation tokens. Device and fleet call sites provide fallback endpoints, with tests covering errors, pagination, and dictionary fleets. ChangesFlight Control retrieval
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant InventoryPlugin
participant FlightctlSDK
participant RawFlightctlEndpoint
InventoryPlugin->>FlightctlSDK: request typed device or fleet data
FlightctlSDK-->>InventoryPlugin: typed data or Pydantic validation error
InventoryPlugin->>RawFlightctlEndpoint: request raw JSON fallback
RawFlightctlEndpoint-->>InventoryPlugin: page items and continuation token
InventoryPlugin->>RawFlightctlEndpoint: request subsequent page
RawFlightctlEndpoint-->>InventoryPlugin: remaining items
Possibly related PRs
Suggested labels: Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors)
✅ Passed checks (9 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
These changes are now in a dedicated PR #63 on branch bugfix/EDM-4980-pydantic-fallback. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/inventory/flightctl.py`:
- Around line 623-629: Expand the existing try block in the Flight Control API
retrieval flow to include json.loads(response.data) and the subsequent records,
metadata, and continue_token processing, so malformed JSON or response-shape
errors are re-raised as FlightctlApiException. Add a test covering invalid JSON
response data and assert the plugin exception is raised.
In `@tests/unit/plugins/inventory/test_flightctl.py`:
- Around line 966-987: Add -> None return annotations to the new test methods,
including test_real_pydantic_validation_error,
test_generic_exception_returns_false, and
test_non_pydantic_validation_error_returns_false. Annotate parameters and return
values for the related fixture helper functions in the referenced sections,
following the repository’s existing type-hint conventions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 75e1c04f-1465-4e21-a7cd-1488c265e30c
📒 Files selected for processing (2)
plugins/inventory/flightctl.pytests/unit/plugins/inventory/test_flightctl.py
| except Exception as e: | ||
| raise FlightctlApiException(f"Error retrieving data from Flight Control API: {e}") from e | ||
| data = json.loads(response.data) | ||
| records = data.get('items', []) | ||
| all_records.extend(records) | ||
| metadata = data.get('metadata', {}) | ||
| continue_token = metadata.get('continue', None) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Wrap raw response decoding failures in FlightctlApiException.
json.loads(response.data) runs outside the try block. A malformed raw response raises JSONDecodeError instead of the plugin's actionable FlightctlApiException.
Include response decoding and response-shape processing in the protected block. Add a test with invalid JSON data.
Proposed fix
try:
response = list_func(
var_continue=continue_token,
label_selector=label_list,
field_selector=field_list,
limit=limit,
_headers=headers,
_request_timeout=request_timeout,
)
- except Exception as e:
- raise FlightctlApiException(f"Error retrieving data from Flight Control API: {e}") from e
- data = json.loads(response.data)
- records = data.get('items', [])
- all_records.extend(records)
- metadata = data.get('metadata', {})
- continue_token = metadata.get('continue', None)
+ data = json.loads(response.data)
+ records = data.get('items', [])
+ all_records.extend(records)
+ metadata = data.get('metadata', {})
+ continue_token = metadata.get('continue')
+ except Exception as exc:
+ raise FlightctlApiException(
+ f"Error retrieving data from Flight Control API: {exc}"
+ ) from exc📝 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.
| except Exception as e: | |
| raise FlightctlApiException(f"Error retrieving data from Flight Control API: {e}") from e | |
| data = json.loads(response.data) | |
| records = data.get('items', []) | |
| all_records.extend(records) | |
| metadata = data.get('metadata', {}) | |
| continue_token = metadata.get('continue', None) | |
| try: | |
| response = list_func( | |
| var_continue=continue_token, | |
| label_selector=label_list, | |
| field_selector=field_list, | |
| limit=limit, | |
| _headers=headers, | |
| _request_timeout=request_timeout, | |
| ) | |
| data = json.loads(response.data) | |
| records = data.get('items', []) | |
| all_records.extend(records) | |
| metadata = data.get('metadata', {}) | |
| continue_token = metadata.get('continue') | |
| except Exception as exc: | |
| raise FlightctlApiException( | |
| f"Error retrieving data from Flight Control API: {exc}" | |
| ) from exc |
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 624-624: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugins/inventory/flightctl.py` around lines 623 - 629, Expand the existing
try block in the Flight Control API retrieval flow to include
json.loads(response.data) and the subsequent records, metadata, and
continue_token processing, so malformed JSON or response-shape errors are
re-raised as FlightctlApiException. Add a test covering invalid JSON response
data and assert the plugin exception is raised.
Source: Path instructions
| def test_real_pydantic_validation_error(self): | ||
| try: | ||
| from pydantic import ValidationError, BaseModel | ||
|
|
||
| class StrictModel(BaseModel): | ||
| value: int | ||
|
|
||
| try: | ||
| StrictModel(value="not-an-int") # type: ignore[arg-type] | ||
| except ValidationError as exc: | ||
| self.assertTrue(_is_pydantic_validation_error(exc)) | ||
| except ImportError: | ||
| self.skipTest("pydantic not installed") | ||
|
|
||
| def test_generic_exception_returns_false(self): | ||
| self.assertFalse(_is_pydantic_validation_error(ValueError("nope"))) | ||
|
|
||
| def test_non_pydantic_validation_error_returns_false(self): | ||
| class ValidationError(Exception): | ||
| __module__ = "myapp.errors" | ||
|
|
||
| self.assertFalse(_is_pydantic_validation_error(ValidationError("nope"))) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add type hints to the new test methods and fixture helpers.
The new methods omit return annotations. The fixture helpers also omit parameter annotations. Add -> None to test methods and annotate helper inputs and return values.
As per path instructions, “Use type hints for function signatures.”
Also applies to: 993-1029, 1035-1106, 1113-1127
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/plugins/inventory/test_flightctl.py` around lines 966 - 987, Add
-> None return annotations to the new test methods, including
test_real_pydantic_validation_error, test_generic_exception_returns_false, and
test_non_pydantic_validation_error_returns_false. Annotate parameters and return
values for the related fixture helper functions in the referenced sections,
following the repository’s existing type-hint conventions.
Sources: Path instructions, Linters/SAST tools
|
Same SDK issue likely affects module_utils/api_module.py This fix is scoped to the inventory plugin, but the root cause (SDK's ApplicationVolume requiring image) also hits api_module.py's call_api() — the shared chokepoint behind flightctl_resource_info, device/fleet modules, approvals, etc. get()/list() only catch ApiException, not pydantic's ValidationError, so e.g. flightctl_resource_info: kind=Device on a mount-only-volume device would still crash with a raw traceback. Not blocking for this PR, but worth a follow-up: extract _is_pydantic_validation_error() into a shared location and apply the same raw-JSON fallback to call_api(). |
2 participants
Summary
The
flightctl-clientSDK's pydantic model forApplicationVolumedeclaresimageas required and non-nullable, but the RHEM API legitimately returnsmount-only volumes without an
imagefield. This causes aValidationErrorthat crashes the entire inventory sync — even if only one device out of many
has a mount-only volume.
This PR adds a fallback mechanism: when
_get_data()catches a pydanticValidationError, it retries using the SDK's*_without_preload_contentmethods that return raw JSON, bypassing pydantic deserialization entirely.
Changes
_is_pydantic_validation_error()helper to detect pydantic errorswithout importing pydantic
_get_data_raw()function for paginated raw JSON fetchingfallback_list_funcparameter to_get_data()with automaticfallback on
ValidationError_populate_inventory_fleets()to handle raw dicts from fallbackTest Plan
python -m pytest tests/unit/ -v)application volume and run
ansible-inventory -i inventory.yml --listSDK path (no warning emitted)
🤖 Generated with Claude Code
Summary
plugins/inventory/andtests/unit/plugins/inventory/.ApplicationVolumepydantic validation becauseimageis missing._get_data()to accept afallback_list_funcparameter._populate_inventory_fleets()to process raw dictionaries.API and compatibility
_get_data()signature gains an optional parameter.