-
Notifications
You must be signed in to change notification settings - Fork 9
EDM-4980: fix inventory crash on mount-only application volumes #63
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,11 +9,14 @@ | |
| DOCUMENTATION, | ||
| InventoryModule, | ||
| _build_auth_headers, | ||
| _get_data, | ||
| _get_data_raw, | ||
| _is_pydantic_validation_error, | ||
| _render_hostname_expression, | ||
| _resolve_hostname, | ||
| _validate_device, | ||
| ) | ||
| from plugins.module_utils.exceptions import ValidationException | ||
| from plugins.module_utils.exceptions import FlightctlApiException, ValidationException | ||
|
|
||
|
|
||
| class TestFlightCtlInventoryModule(unittest.TestCase): | ||
|
|
@@ -957,5 +960,172 @@ def test_env_vars_match_module_utils_convention(self): | |
| f"Env var '{expected_env}' should start with FLIGHTCTL_") | ||
|
|
||
|
|
||
| class TestIsPydanticValidationError(unittest.TestCase): | ||
| """Verify _is_pydantic_validation_error detects pydantic errors without importing pydantic.""" | ||
|
|
||
| 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"))) | ||
|
Comment on lines
+966
to
+987
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 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 As per path instructions, “Use type hints for function signatures.” Also applies to: 993-1029, 1035-1106, 1113-1127 🤖 Prompt for AI AgentsSources: Path instructions, Linters/SAST tools |
||
|
|
||
|
|
||
| class TestGetDataRawFallback(unittest.TestCase): | ||
| """Verify _get_data_raw paginates through raw HTTP responses.""" | ||
|
|
||
| def _make_raw_response(self, items, continue_token=None): | ||
| metadata = {} | ||
| if continue_token: | ||
| metadata['continue'] = continue_token | ||
| body = json.dumps({'items': items, 'metadata': metadata}).encode() | ||
| resp = MagicMock() | ||
| resp.data = body | ||
| return resp | ||
|
|
||
| def test_single_page(self): | ||
| items = [{'metadata': {'name': 'dev-1'}}, {'metadata': {'name': 'dev-2'}}] | ||
| list_func = MagicMock(return_value=self._make_raw_response(items)) | ||
|
|
||
| result = _get_data_raw(list_func, limit=100) | ||
| self.assertEqual(len(result), 2) | ||
| self.assertEqual(result[0]['metadata']['name'], 'dev-1') | ||
| list_func.assert_called_once() | ||
|
|
||
| def test_multi_page_pagination(self): | ||
| page1 = self._make_raw_response( | ||
| [{'metadata': {'name': 'dev-1'}}], continue_token='token-abc' | ||
| ) | ||
| page2 = self._make_raw_response( | ||
| [{'metadata': {'name': 'dev-2'}}] | ||
| ) | ||
| list_func = MagicMock(side_effect=[page1, page2]) | ||
|
|
||
| result = _get_data_raw(list_func, limit=1) | ||
| self.assertEqual(len(result), 2) | ||
| self.assertEqual(result[0]['metadata']['name'], 'dev-1') | ||
| self.assertEqual(result[1]['metadata']['name'], 'dev-2') | ||
| self.assertEqual(list_func.call_count, 2) | ||
|
|
||
| def test_api_error_raises_flightctl_exception(self): | ||
| list_func = MagicMock(side_effect=ConnectionError("timeout")) | ||
| with self.assertRaises(FlightctlApiException): | ||
| _get_data_raw(list_func) | ||
|
|
||
|
|
||
| class TestGetDataPydanticFallback(unittest.TestCase): | ||
| """Verify _get_data falls back to raw JSON on pydantic ValidationError.""" | ||
|
|
||
| def _make_typed_response(self, items, continue_token=None): | ||
| resp = MagicMock() | ||
| resp.items = items | ||
| metadata = {} | ||
| if continue_token: | ||
| metadata['continue'] = continue_token | ||
| resp.to_dict.return_value = {'metadata': metadata} | ||
| return resp | ||
|
|
||
| def _make_raw_response(self, items, continue_token=None): | ||
| metadata = {} | ||
| if continue_token: | ||
| metadata['continue'] = continue_token | ||
| body = json.dumps({'items': items, 'metadata': metadata}).encode() | ||
| resp = MagicMock() | ||
| resp.data = body | ||
| return resp | ||
|
|
||
| def _make_pydantic_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: | ||
| return exc | ||
| except ImportError: | ||
| return None | ||
|
|
||
| def test_normal_path_no_fallback_needed(self): | ||
| typed_resp = self._make_typed_response([MagicMock(), MagicMock()]) | ||
| list_func = MagicMock(return_value=typed_resp) | ||
| fallback_func = MagicMock() | ||
|
|
||
| result = _get_data(list_func, fallback_list_func=fallback_func) | ||
| self.assertEqual(len(result), 2) | ||
| fallback_func.assert_not_called() | ||
|
|
||
| def test_pydantic_error_triggers_fallback(self): | ||
| pydantic_exc = self._make_pydantic_error() | ||
| if pydantic_exc is None: | ||
| self.skipTest("pydantic not installed") | ||
|
|
||
| list_func = MagicMock(side_effect=pydantic_exc) | ||
| raw_items = [{'metadata': {'name': 'dev-1'}}] | ||
| fallback_func = MagicMock(return_value=self._make_raw_response(raw_items)) | ||
|
|
||
| result = _get_data(list_func, fallback_list_func=fallback_func) | ||
| self.assertEqual(len(result), 1) | ||
| self.assertEqual(result[0]['metadata']['name'], 'dev-1') | ||
| fallback_func.assert_called_once() | ||
|
|
||
| def test_non_pydantic_error_still_raises(self): | ||
| list_func = MagicMock(side_effect=ConnectionError("refused")) | ||
| fallback_func = MagicMock() | ||
|
|
||
| with self.assertRaises(FlightctlApiException): | ||
| _get_data(list_func, fallback_list_func=fallback_func) | ||
| fallback_func.assert_not_called() | ||
|
|
||
| def test_pydantic_error_without_fallback_raises(self): | ||
| pydantic_exc = self._make_pydantic_error() | ||
| if pydantic_exc is None: | ||
| self.skipTest("pydantic not installed") | ||
|
|
||
| list_func = MagicMock(side_effect=pydantic_exc) | ||
|
|
||
| with self.assertRaises(FlightctlApiException): | ||
| _get_data(list_func, fallback_list_func=None) | ||
|
|
||
|
|
||
| class TestPopulateInventoryFleetsWithRawDicts(unittest.TestCase): | ||
| """Verify _populate_inventory_fleets handles raw dicts (fallback path).""" | ||
|
|
||
| @patch('plugins.inventory.flightctl._fetch_fleet_devices') | ||
| def test_fleet_as_raw_dict(self, mock_fetch): | ||
| mock_fetch.return_value = [] | ||
| inventory = InventoryModule() | ||
| mock_inv = MagicMock() | ||
| mock_groups = MagicMock() | ||
| mock_groups.__contains__ = MagicMock(return_value=False) | ||
| mock_inv.groups = mock_groups | ||
| inventory.inventory = mock_inv | ||
|
|
||
| raw_fleet = {'metadata': {'name': 'fleet-1'}} | ||
| config = MagicMock() | ||
|
|
||
| inventory._populate_inventory_fleets([raw_fleet], config) | ||
|
|
||
| mock_fetch.assert_called_once_with('fleet-1', config, inventory.LIMIT_PER_PAGE) | ||
|
|
||
|
|
||
| if __name__ == '__main__': | ||
| unittest.main() | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Wrap raw response decoding failures in
FlightctlApiException.json.loads(response.data)runs outside thetryblock. A malformed raw response raisesJSONDecodeErrorinstead of the plugin's actionableFlightctlApiException.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
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 624-624: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
Source: Path instructions