Remove apple profile and decl from bulk set pending - #46321
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #46321 +/- ##
==========================================
- Coverage 66.90% 66.76% -0.14%
==========================================
Files 2797 2797
Lines 223494 223484 -10
Branches 11297 11297
==========================================
- Hits 149519 149206 -313
- Misses 60432 60735 +303
Partials 13543 13543
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:
|
|
Vuln test unrelated failure |
a3e1589 to
960ff51
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
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.
There was a problem hiding this comment.
Pull request overview
This PR removes Apple configuration profile + DDM declaration side effects from BulkSetPendingMDMHostProfiles, shifting expectations to the reconciliation crons/batched reconcilers, and updates integration/datastore tests accordingly.
Changes:
- Updates MDM integration tests to explicitly run the profile schedule/reconcilers instead of relying on synchronous “bulk set pending” behavior.
- Switches Apple DDM reconciliation tests to
ReconcileAppleDeclarationsBatched. - Removes large datastore tests that asserted legacy
BulkSetPendingMDMHostProfilesbehavior, and adjusts remaining tests for the new flow.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/integration_mdm_test.go | Tweaks profile assertion to improve failure output via require.Len. |
| server/service/integration_mdm_profiles_test.go | Runs profile schedule in more places; adjusts expectations for pending operations and Fleet-managed profiles. |
| server/service/integration_mdm_ddm_test.go | Uses batched Apple declaration reconciler and removes now-invalid immediate host profile assertions. |
| server/datastore/mysql/mysql.go | Adds a TODO comment on a test hook field. |
| server/datastore/mysql/microsoft_mdm_test.go | Updates Windows LocURI protection test to run the Windows reconciler instead of calling BulkSetPendingMDMHostProfiles. |
| server/datastore/mysql/microsoft_mdm_eager_test.go | Marks Windows eager-hook helpers as TODO for deletion. |
| server/datastore/mysql/mdm.go | Stops bulk “pending” work for Apple (Android-only), leaves Windows eager hook for tests, and removes Apple declaration bulk-set logic. |
| server/datastore/mysql/mdm_test.go | Removes legacy bulk-set-pending tests; narrows remaining coverage to Android-only behavior. |
| server/datastore/mysql/apple_mdm.go | Marks bulkSetPendingMDMAppleHostProfilesDB as slated for deletion and suppresses unused lint. |
| server/datastore/mysql/apple_mdm_test.go | Updates several tests to use batched reconcilers; adds ABM asset setup in some test paths. |
Comments suppressed due to low confidence (1)
server/datastore/mysql/mdm.go:894
- The comment above
BulkSetPendingMDMHostProfilesstill describes Apple profiles/declarations being reconciled eagerly in this method, but the implementation now only updates Android (and optionally triggers a Windows test hook). Updating this comment avoids misleading future readers about what this function actually does.
// TODO(MHJ): Document the oddity this now only handles Android
func (ds *Datastore) BulkSetPendingMDMHostProfiles(
ctx context.Context,
hostIDs, teamIDs []uint,
profileUUIDs, hostUUIDs []string,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR reduces the shared bulk pending-host-profile helper to Android-only and marks the Apple helper for deletion, deprecates Windows eager-hook test helpers, and updates datastore and service tests to call batched reconcilers (ReconcileAppleDeclarationsBatched, ReconcileAppleProfilesBatched, ReconcileWindowsProfiles). Tests are wired with MDMAppleCommander, mocked KV where required, include ABM asset setup, and integration tests now await the profile-schedule trigger before asserting host profile state. Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
![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 `@server/datastore/mysql/apple_mdm_test.go`:
- Around line 6554-6558: The test contains a dead setup: a local
mock.AdvancedKVStore assigned to mockKV and its MGetFunc configured but never
used because ReconcileAppleDeclarationsBatched(ctx, ds, commander, ds.logger)
does not accept or use mockKV; remove the unused mockKV creation and MGetFunc
assignment from the declaration reconcile test blocks (leave the profiles tests
that call ReconcileAppleProfilesBatched unchanged) so the test only sets up
mocks actually passed to or used by ReconcileAppleDeclarationsBatched.
In `@server/datastore/mysql/microsoft_mdm_eager_test.go`:
- Line 26: Do not delete the deprecated Windows eager-test helpers yet: restore
or keep bulkSetPendingMDMWindowsHostProfilesForTests,
EnableTestWindowsEagerHook, and the eager-hook plumbing (testWindowsEagerHook
and its invocation) so existing tests still compile; add a clear TODO comment
referencing the remaining test call-site in mdm_test.go that must be migrated
before removal, and ensure service.ReconcileWindowsProfiles usages remain
unchanged—only remove these helpers after
bulkSetPendingMDMWindowsHostProfilesForTests call sites are migrated and tests
updated.
🪄 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: bc63896a-dae1-465d-b328-439442d2702b
📒 Files selected for processing (10)
server/datastore/mysql/apple_mdm.goserver/datastore/mysql/apple_mdm_test.goserver/datastore/mysql/mdm.goserver/datastore/mysql/mdm_test.goserver/datastore/mysql/microsoft_mdm_eager_test.goserver/datastore/mysql/microsoft_mdm_test.goserver/datastore/mysql/mysql.goserver/service/integration_mdm_ddm_test.goserver/service/integration_mdm_profiles_test.goserver/service/integration_mdm_test.go
| }) | ||
| } | ||
|
|
||
| // TODO(MHJ): Document the oddity this now only handles Android |
There was a problem hiding this comment.
Might be worth dropping a note in #g-power-to-pc about this I saw them making some other Android profile reconciliation changes
Follow up work as discovery made on the Apple reconciler changes.
One more follow up PR will come with a clean up of all old and unused code.
Summary by CodeRabbit
Tests
Chores