Extract Apple MDM initialization out of runServeCmd#46517
Conversation
Move the Apple MDM storage setup, push service construction, and APNs/SCEP/ABM asset reconciliation out of runServeCmd into testable functions in a new cmd/fleet/mdm_apple.go. The shared asset-existence check that was an inline closure is now a package function reused across all call sites. Adds unit tests for the dev-mode push gate, the asset-existence helper branches, and the no-op and missing-private-key paths of both reconcilers. Behavior is preserved; the full cmd/fleet suite passes unchanged. Part of fleetdm#33370.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis PR refactors Apple MDM initialization logic by extracting inline code from serve.go into a new module (mdm_apple.go) with focused helper functions. New helpers construct MySQL-backed NanoMDM storages, initialize an APNs push service (or no-op in dev mode), and reconcile APNs/SCEP and ABM assets in the datastore. The reconciliation functions load configured certs/keys and insert them when missing, while handling duplicate-key errors gracefully. The serve.go file is refactored to call these helpers and to replace all 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 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: 1
🤖 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 `@cmd/fleet/mdm_apple.go`:
- Around line 62-66: The TLS config passed into buford.NewPushProviderFactory
(see pushProviderFactory / buford.NewPushProviderFactory and the tls.Config used
when constructing fleethttp.NewClient) must explicitly set MinVersion to
tls.VersionTLS12 to enforce TLS 1.2+ for APNs; update the tls.Config literal to
include MinVersion: tls.VersionTLS12 while keeping the existing Certificates
field so the client uses TLS 1.2 or higher.
🪄 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: a3f121e5-d769-480b-911f-0ef520ba1334
📒 Files selected for processing (3)
cmd/fleet/mdm_apple.gocmd/fleet/mdm_apple_test.gocmd/fleet/serve.go
|
Hey @getvictor @JordanMontgomery - Would you mind taking a review, please? This PR extracts the Apple MDM related logic out of serve.go. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #46517 +/- ##
==========================================
- Coverage 66.92% 66.79% -0.13%
==========================================
Files 2798 2801 +3
Lines 223420 223565 +145
Branches 11467 11467
==========================================
- Hits 149519 149338 -181
- Misses 60357 60668 +311
- Partials 13544 13559 +15
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:
|
Add MinVersion: tls.VersionTLS12 to the APNs push client (pre-existing gap surfaced when moving the code; Apple APNs requires TLS 1.2+). Adds a unit test for the APNs/SCEP reconciler path where assets are already stored, asserting the insert is skipped. Per review feedback on fleetdm#46517.
1 participant
Extracts the Apple MDM initialization out of
runServeCmdinto testable functions in a newcmd/fleet/mdm_apple.go. Continues the chain of extractions on this issue (#44929, #45343, #45583, #46166, #46421) toward theserve.go>60% coverage target discussed on #33370.Five functions come out of the inline block:
initAppleMDMStorages— constructs the MDM, DEP, and SCEP storages.initAppleMDMPushService— picks the no-op pusher underFLEET_DEV_MDM_APPLE_DISABLE_PUSH=1, otherwise the real APNs pusher.checkMDMAssetsExist— promotes the inlinecheckMDMAssetsclosure to a package function. It was already used at several call sites; they now all share this one.reconcileAppleMDMAPNsAndSCEPAssets/reconcileAppleMDMABMAssets— the APNs/SCEP and ABM asset reconciliation blocks.Behavior is preserved —
runServeCmdcalls these in the same order with the same arguments, and the fullcmd/fleetsuite passes unchanged against MySQL + Redis. Each function returns early afterinitFatalso it's also safe when the caller'sinitFataldoesn't terminate (the case in tests).On test scope: the new unit tests cover the dev-mode push gate, all four branches of
checkMDMAssetsExist, and the no-op and missing-private-key paths of both reconcilers. The storage construction and the actual asset-insert paths need a real datastore, so those stay covered by the existing integration tests rather than new unit tests — I didn't want to stand up a full datastore mock for paths that are already exercised end-to-end.Related issue: Refs #33370
Checklist for submitter
Summary by CodeRabbit
New Features
Tests