Refresh inventory detection and extend engine coverage#39
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a database-backed inventory refresh system: model fields and migration, engine-specific inventory detection, scheduler/task wiring, API/settings integration, serializers/views, frontend display and settings, and tests covering behavior and engine fallbacks. ChangesInventory Refresh System
Sequence DiagramsequenceDiagram
participant User
participant Frontend
participant SettingsAPI
participant Scheduler
participant RefreshTask
participant Engine
participant DB
User->>Frontend: Open settings or inventory list
Frontend->>SettingsAPI: GET /system-settings or GET /instances
SettingsAPI->>Scheduler: ensure_inventory_refresh_schedule()
Scheduler->>DB: create/reuse one-shot TaskSchedule (locked via Config)
SettingsAPI-->>Frontend: return settings/options or instances (with inventory fields)
User->>Frontend: Update inventory interval + Save
Frontend->>SettingsAPI: PUT /system-settings {inventory_refresh_interval}
SettingsAPI->>Scheduler: sync_inventory_refresh_schedule(force=True)
Scheduler->>DB: reschedule one-shot TaskSchedule
SettingsAPI-->>Frontend: {inventory_refresh_schedule_synced: true/false}
Note over Scheduler,RefreshTask: At scheduled time
Scheduler->>RefreshTask: fire refresh task
RefreshTask->>DB: list Instances (ordered)
loop per Instance
RefreshTask->>Engine: connect & get_inventory_details()
Engine->>DB: query server properties (hostname/version)
Engine-->>RefreshTask: {hostname, version}
RefreshTask->>DB: update Instance inventory_{status,last_* ,detected_*}
end
RefreshTask->>Scheduler: inventory_refresh_task_callback()
Scheduler->>DB: re-arm next one-shot schedule
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api_core/legacy_tests.py (1)
2319-2345:⚠️ Potential issue | 🟠 MajorRename one of the duplicate
TestPermissionRequestAPIclasses; this updated test is currently shadowed.
api_core/legacy_tests.pydefinesTestPermissionRequestAPItwice (lines 2085 and 4511). In Python, the later class definition overwrites the earlier binding in the module namespace, so tests in the first class—including this updated test at lines 2319-2345—are not discovered by unittest.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api_core/legacy_tests.py` around lines 2319 - 2345, The module defines TestPermissionRequestAPI twice which causes the first definition (containing the updated test at test_test_instance_connection) to be shadowed; locate both class definitions named TestPermissionRequestAPI and rename one to a unique name (e.g., TestPermissionRequestAPI_Legacy or TestPermissionRequestAPI_Old) so the earlier test class is preserved and discovered by unittest; ensure the chosen new class name is updated wherever referenced in the file (if any) and that there are no duplicate test class names remaining.
🧹 Nitpick comments (1)
sql/engines/clickhouse.py (1)
58-68: 💤 Low value
server_version: raisingValueErrorinside the sametryblock that catches bareException(BLE001).The pattern raises a
ValueErroras a control-flow signal and then catches it alongside all other exceptions. Restructuring separates query failure (already handled byresult.error) from parsing failure and avoids the broad catch that static analysis flags.♻️ Suggested refactor
`@property` def server_version(self): sql = "select value from system.build_options where name = 'VERSION_FULL';" - try: - result = self.query(sql=sql) - if result.error or not result.rows or not result.rows[0]: - raise ValueError(result.error or "empty result") - version = result.rows[0][0].split(" ")[1] - return tuple([int(n) for n in version.split(".")[:3]]) - except Exception as exc: - logger.warning("Failed to fetch ClickHouse server version: %s", exc) - return tuple() + result = self.query(sql=sql) + if result.error or not result.rows or not result.rows[0]: + logger.warning("Failed to fetch ClickHouse server version: %s", result.error or "empty result") + return tuple() + try: + version = result.rows[0][0].split(" ")[1] + return tuple(int(n) for n in version.split(".")[:3]) + except (IndexError, ValueError) as exc: + logger.warning("Failed to parse ClickHouse server version: %s", exc) + return tuple()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sql/engines/clickhouse.py` around lines 58 - 68, In server_version, avoid raising ValueError inside the broad try that immediately catches Exception; instead check result.error and empty results immediately after calling self.query and return/handle there, then move only the parsing logic into a narrow try that catches specific parsing errors (e.g., ValueError/IndexError) or let them propagate; reference the server_version method and the result variable returned by self.query to locate where to separate query-failure handling from version-string parsing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api_instances/views.py`:
- Around line 513-514: The API docs listing searchable fields (currently showing
only ID/name/host/user) are out of sync with the view that now performs
Q(inventory_detected_hostname__icontains=search) and
Q(inventory_detected_version__icontains=search); update the
schema/docstring/OpenAPI spec entry that enumerates searchable fields to include
"inventory_detected_hostname" and "inventory_detected_version" (and any matching
human-friendly labels), and update any examples/tests that assert the old list
so docs and tests match the actual search behavior implemented in the view.
In `@common/tests.py`:
- Around line 150-157: The test assumes a default interval longer than "1h"
which may change; make it deterministic by explicitly setting a known initial
interval before the first schedule creation: call
self.sys_config.set("inventory_refresh_interval", "<initial_interval>") (e.g.
"2h"), call inventory.ensure_inventory_refresh_schedule() to obtain first_run
(via
TaskSchedule.objects.get(name=inventory.INVENTORY_REFRESH_SCHEDULE_NAME).run_at),
then change the interval to "1h", call
inventory.ensure_inventory_refresh_schedule(force=True) to obtain second_run,
and finally assert second_run < first_run and mock_schedule.call_count == 2.
In `@frontend/src/features/inventory/pages/InventoryListPage.vue`:
- Line 374: The search placeholder in InventoryListPage.vue is out of sync with
the :search-keys array (which now includes 'inventory_detected_hostname' and
'inventory_detected_version'); update the placeholder text used by the search
input (the component/prop that renders the search box in
InventoryListPage.vue—e.g., the placeholder or search-placeholder prop) to
mention detected hostname and detected version (for example: "Search by name,
host, user, ID, detected hostname or detected version") so the UI matches the
searchable fields.
In `@sql/engines/mysql.py`:
- Around line 158-171: get_inventory_details currently uses a raw cursor
(cursor.execute/cursor.fetchone) so DB errors can propagate; change it to follow
PgSQL/MSSQL behavior by using self.query("SELECT @@hostname") or wrapping the
cursor calls in try/except that catches exceptions, logs a warning, closes the
cursor in finally, and returns default_details on failure. Also protect the
conn.get_server_info() call similarly (return default_details if it raises) so
any exception from hostname lookup or server_info does not bubble up; keep
references to get_inventory_details, self.query, cursor.execute,
cursor.fetchone, conn.get_server_info, and default_details when implementing the
fix.
In `@sql/engines/test_mssql.py`:
- Around line 91-96: The test
test_get_inventory_details_returns_fallback_when_query_fails is not actually
simulating a query error because ResultSet.__init__ ignores an error kwarg;
update the test to construct the ResultSet normally then set its error attribute
explicitly (e.g. rs = ResultSet(rows=[]); rs.error = "boom";
mock_query.return_value = rs) so MssqlEngine.get_inventory_details sees
result.error truthy and exercises the error branch rather than only the
empty-rows path.
---
Outside diff comments:
In `@api_core/legacy_tests.py`:
- Around line 2319-2345: The module defines TestPermissionRequestAPI twice which
causes the first definition (containing the updated test at
test_test_instance_connection) to be shadowed; locate both class definitions
named TestPermissionRequestAPI and rename one to a unique name (e.g.,
TestPermissionRequestAPI_Legacy or TestPermissionRequestAPI_Old) so the earlier
test class is preserved and discovered by unittest; ensure the chosen new class
name is updated wherever referenced in the file (if any) and that there are no
duplicate test class names remaining.
---
Nitpick comments:
In `@sql/engines/clickhouse.py`:
- Around line 58-68: In server_version, avoid raising ValueError inside the
broad try that immediately catches Exception; instead check result.error and
empty results immediately after calling self.query and return/handle there, then
move only the parsing logic into a narrow try that catches specific parsing
errors (e.g., ValueError/IndexError) or let them propagate; reference the
server_version method and the result variable returned by self.query to locate
where to separate query-failure handling from version-string parsing.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8a7c02ba-fcf7-4a36-9b94-903cd5cdc175
📒 Files selected for processing (22)
api_admin/settings.pyapi_core/legacy_tests.pyapi_instances/serializers.pyapi_instances/views.pycommon/tests.pyfrontend/src/app/feature-registry.test.tsfrontend/src/components/ui/data-table/DataTable.vuefrontend/src/features/inventory/pages/InventoryListPage.vuefrontend/src/features/settings/system-settings.tsfrontend/src/lib/api.tssql/engines/__init__.pysql/engines/clickhouse.pysql/engines/mssql.pysql/engines/mysql.pysql/engines/oracle.pysql/engines/pgsql.pysql/engines/test_mssql.pysql/engines/test_mysql.pysql/engines/tests.pysql/inventory.pysql/migrations/0016_instance_inventory_detected_hostname_and_more.pysql/models.py
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
api_core/legacy_tests.py (1)
1536-1536: ⚡ Quick winUse
timezone.now()instead ofdatetime.now()for theDateTimeField.
DateTimeFieldwithUSE_TZ = Trueexpects only aware datetimes; passing a naive one fromdatetime.now()raises aRuntimeWarning.django.utils.timezone.now()is the idiomatic replacement — it returns an aware datetime whenUSE_TZ = Trueand a naive one whenFalse, making it safe in all configurations.🔧 Proposed fix
+from django.utils import timezone ... - self.ins.inventory_last_success_at = datetime.now() + self.ins.inventory_last_success_at = timezone.now()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api_core/legacy_tests.py` at line 1536, Replace the naive datetime.now() with Django's timezone-aware helper when setting the DateTimeField: change the assignment to use django.utils.timezone.now() for self.ins.inventory_last_success_at (and add an import for timezone at top if missing) so the value is timezone-aware when USE_TZ=True and avoids RuntimeWarning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@api_core/legacy_tests.py`:
- Line 1536: Replace the naive datetime.now() with Django's timezone-aware
helper when setting the DateTimeField: change the assignment to use
django.utils.timezone.now() for self.ins.inventory_last_success_at (and add an
import for timezone at top if missing) so the value is timezone-aware when
USE_TZ=True and avoids RuntimeWarning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9b41f4e4-51c7-4b9a-9362-6d66578235ae
📒 Files selected for processing (8)
api_core/legacy_tests.pyapi_instances/views.pycommon/tests.pyfrontend/src/features/inventory/pages/InventoryListPage.vuesql/engines/clickhouse.pysql/engines/mysql.pysql/engines/test_mssql.pysql/engines/test_mysql.py
✅ Files skipped from review due to trivial changes (1)
- sql/engines/test_mssql.py
🚧 Files skipped from review as they are similar to previous changes (2)
- sql/engines/test_mysql.py
- frontend/src/features/inventory/pages/InventoryListPage.vue
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
Testing
Summary by CodeRabbit
New Features
Bug Fixes