Repository navigation
Remove unused windows_updates MySQL table and ingestion - #44128
Conversation
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRemoves the unused Possibly related PRs
🚥 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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@server/datastore/mysql/migrations/tables/20260423153950_DropWindowsUpdatesTable.go`:
- Around line 12-17: hostRefs in server/datastore/mysql/hosts.go still lists
"windows_updates" which was dropped by Up_20260423153950; remove the
"windows_updates" entry from the hostRefs array (the slice/array named hostRefs
used when deleting host-related rows) so the deletion loop no longer issues
DELETE FROM windows_updates and fails; update hostRefs to exclude that string
and run relevant tests to ensure host deletion proceeds without referencing the
dropped table.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d5eae61c-0960-406c-9473-0ce32bcfec0c
📒 Files selected for processing (11)
changes/44127-remove-unused-windows_updatesserver/datastore/mysql/migrations/tables/20260423153950_DropWindowsUpdatesTable.goserver/datastore/mysql/migrations/tables/20260423153950_DropWindowsUpdatesTable_test.goserver/datastore/mysql/windows_updates.goserver/datastore/mysql/windows_updates_test.goserver/fleet/datastore.goserver/fleet/windows_updates.goserver/fleet/windows_updates_tests.goserver/mock/datastore_mock.goserver/service/osquery_utils/queries.goserver/service/osquery_utils/queries_test.go
💤 Files with no reviewable changes (8)
- server/fleet/windows_updates_tests.go
- server/datastore/mysql/windows_updates_test.go
- server/datastore/mysql/windows_updates.go
- server/fleet/datastore.go
- server/service/osquery_utils/queries_test.go
- server/service/osquery_utils/queries.go
- server/mock/datastore_mock.go
- server/fleet/windows_updates.go
There was a problem hiding this comment.
Pull request overview
Removes the unused windows_updates MySQL table and its associated osquery detail query ingestion so Fleet no longer schedules windows_update_history or maintains dead datastore/types.
Changes:
- Removed the
windows_update_historydetail query anddirectIngestWindowsUpdateHistoryingestion. - Deleted
windows_updatesfleet types + MySQL datastore methods and removed the corresponding datastore interface/mock methods. - Added a MySQL migration to drop the
windows_updatestable (and a changelog entry).
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/osquery_utils/queries.go | Removes the Windows update history detail query and ingestion hook. |
| server/service/osquery_utils/queries_test.go | Updates detail-query expectations and removes ingestion test for windows update history. |
| server/fleet/datastore.go | Removes windows_updates methods from the datastore interface. |
| server/mock/datastore_mock.go | Removes mock methods/func fields for windows_updates datastore APIs. |
| server/fleet/windows_updates.go | Deletes unused WindowsUpdate parsing/types. |
| server/fleet/windows_updates_tests.go | Deletes tests for removed WindowsUpdate parsing logic. |
| server/datastore/mysql/windows_updates.go | Deletes MySQL implementation for the removed windows_updates datastore APIs. |
| server/datastore/mysql/windows_updates_test.go | Deletes MySQL tests for the removed windows_updates datastore APIs. |
| server/datastore/mysql/migrations/tables/20260423153950_DropWindowsUpdatesTable.go | Adds migration to drop windows_updates table. |
| server/datastore/mysql/migrations/tables/20260423153950_DropWindowsUpdatesTable_test.go | Adds migration test validating the table is dropped. |
| changes/44127-remove-unused-windows_updates | Adds release note entry for the removal. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
getvictor
left a comment
There was a problem hiding this comment.
Looks good. See CI and CodeRabbit suggestion
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #44128 +/- ##
==========================================
+ Coverage 66.75% 66.78% +0.02%
==========================================
Files 2623 2621 -2
Lines 211148 211106 -42
Branches 9386 9386
==========================================
+ Hits 140959 140987 +28
+ Misses 57357 57307 -50
+ Partials 12832 12812 -20
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/service/osquery_test.go (1)
2495-2496:⚠️ Potential issue | 🟠 MajorUpdate test assertion from
len(expected)+3tolen(expected)+2and remove stalewindows update historyreference.The comment and assertion are outdated.
windows_update_historyis not a detail query—it exists only in seed data and the osquery table implementation, not inosquery_utils.GetDetailQueries. With a Windows host and one campaign query (ID 42), the test should account for exactly two additional queries: the campaign query andfleet_no_policies_wildcard. Update the assertion and comment accordingly:✏️ Proposed fix
- // +1 for the distributed query for campaign ID 42, +1 for windows update history, +1 for the fleet_no_policies_wildcard query. - if expected := expectedDetailQueriesForPlatform(host.Platform); !assert.Equal(t, len(expected)+3, len(queries)) { + // +1 for the distributed query for campaign ID 42, +1 for the fleet_no_policies_wildcard query. + if expected := expectedDetailQueriesForPlatform(host.Platform); !assert.Equal(t, len(expected)+2, len(queries)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/osquery_test.go` around lines 2495 - 2496, Update the test comment and assertion in server/service/osquery_test.go to remove the stale "windows update history" reference and change the expected count from len(expected)+3 to len(expected)+2: adjust the comment that explains why we add extra queries to mention only the campaign query (ID 42) and the fleet_no_policies_wildcard, and change the assertion using expectedDetailQueriesForPlatform(host.Platform) to assert.Equal(t, len(expected)+2, len(queries)); ensure you update any nearby explanatory text to match this corrected expectation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@server/service/osquery_test.go`:
- Around line 2495-2496: Update the test comment and assertion in
server/service/osquery_test.go to remove the stale "windows update history"
reference and change the expected count from len(expected)+3 to len(expected)+2:
adjust the comment that explains why we add extra queries to mention only the
campaign query (ID 42) and the fleet_no_policies_wildcard, and change the
assertion using expectedDetailQueriesForPlatform(host.Platform) to
assert.Equal(t, len(expected)+2, len(queries)); ensure you update any nearby
explanatory text to match this corrected expectation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: efdf06d7-5465-4225-832f-fef04c1cb64c
⛔ Files ignored due to path filters (1)
docs/Contributing/product-groups/orchestration/understanding-host-vitals.mdis excluded by!**/*.md
📒 Files selected for processing (4)
server/datastore/mysql/hosts.goserver/datastore/mysql/hosts_test.goserver/datastore/mysql/schema.sqlserver/service/osquery_test.go
💤 Files with no reviewable changes (2)
- server/datastore/mysql/hosts.go
- server/datastore/mysql/hosts_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@server/datastore/mysql/migrations/tables/20260423161824_DropWindowsUpdatesTable.go`:
- Around line 8-20: The migration functions are misversioned: the file is
20260423161824_DropWindowsUpdatesTable.go and schema seeds version
20260423161824, but the code registers Up_20260423153950/Down_20260423153950;
rename the functions and their registrations from Up_20260423153950 and
Down_20260423153950 to Up_20260423161824 and Down_20260423161824 (and update the
MigrationClient.AddMigration call to use the new symbols) so the migration
version matches the file name and schema seed.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4e92ee9a-1720-48d5-a3b5-b1dad4708297
📒 Files selected for processing (2)
server/datastore/mysql/migrations/tables/20260423161824_DropWindowsUpdatesTable.goserver/datastore/mysql/schema.sql
sgress454
left a comment
There was a problem hiding this comment.
understanding-host-vitals.md removal matches code removal 👍
Related issue: Resolves #44127
changes/,orbit/changes/oree/fleetd-chrome/changes.Testing
Summary by CodeRabbit