Extract Apple APNs/SCEP pair validation onto MDMConfig#46166
Conversation
Move the both-or-neither check for Apple APNs and SCEP out of runServeCmd and onto a ValidateAppleAPNSAndSCEPPair(initFatal) method on MDMConfig, matching the pattern used by ConditionalAccessConfig.Validate and the validators added in fleetdm#45583. Behavior is preserved; error messages and gating are unchanged. Part of fleetdm#33370.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Hello @getvictor - This is another installment for the coverage improvement in Thanks! 🙏 🙇 |
|
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)
WalkthroughThis PR extracts Apple MDM APNs/SCEP configuration validation from inline checks in the APNs/SCEP reconciliation flow into a centralized method on 🚥 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #46166 +/- ##
=======================================
Coverage 66.82% 66.83%
=======================================
Files 2755 2755
Lines 220203 220205 +2
Branches 11045 11045
=======================================
+ Hits 147160 147165 +5
+ Misses 59749 59742 -7
- Partials 13294 13298 +4
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:
|
MagnusHJensen
left a comment
MagnusHJensen
left a comment
There was a problem hiding this comment.
Thanks @raju249 for this change!
I reviewed this one, since most US folks is out for the holiday today.
Extracts the OTEL trace, metric, and log provider setup out of `runServeCmd` and into `initOTELProviders` in a new `cmd/fleet/otel.go`. Same pattern as the prior extractions on this issue (#44929, #45343, #45583, #46166). Side effects (`otel.SetTracerProvider`, `otel.SetMeterProvider`) are preserved inside the extracted function, so runtime behavior is identical. Three unit tests in `cmd/fleet/otel_test.go`: - OTEL disabled (the common production path) returns `(nil, nil, nil)` and never calls `initFatal`. - OTEL enabled without log export returns non-nil trace and meter providers; logger provider stays nil. - Log export enabled returns all three providers non-nil. One honest note on coverage: the four `initFatal` sites inside the function are paranoid wrapping for OTEL SDK constructors that don't dial at construction time, so the error paths are hard to drive in tests without mocking the SDK. The tests above exercise the success paths and the disabled gate, which is the bulk of the realistic flow. This continues the path toward `serve.go` >60% coverage per the discussion on #33370 — `serve.go` is now ~100 lines shorter and the OTEL phase is testable as a unit. Remaining slices per the broader plan: MDM Apple init, datastore init, Redis init. **Related issue:** Refs #33370 # Checklist for submitter - [x] Added/updated automated tests - Changes file: not applicable — internal refactor with no user-visible behavior change <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Refactor** * Centralized OpenTelemetry provider initialization into a single setup path, simplifying startup and shutdown behavior and making observability configuration clearer. * **Tests** * Added unit tests covering disabled/enabled telemetry paths and optional log export, plus cleanup logic to ensure providers are shut down correctly. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/fleetdm/fleet/pull/46421?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
](https://app.coderabbit.ai/change-stack/fleetdm/fleet/pull/46421?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack){kind=link}
2 participants
Extracts the Apple APNs/SCEP both-or-neither check out of
runServeCmdand puts it onMDMConfigasValidateAppleAPNSAndSCEPPair(initFatal). Same pattern asConditionalAccessConfig.Validate,AndroidAgentConfig.Validate, and the validators added in #45583.The call site (inside the existing
if len(toInsert) > 0gate) goes from six lines of inline conditionalinitFatalcalls to one method call. Behavior, error messages, and gating are unchanged.Tests live in
server/config/config_test.go: one smoke case plus two error branches (APNs-only and SCEP-only). Skipped the "neither set" case on purpose — the outerif config.MDM.IsAppleAPNsSet() || config.MDM.IsAppleSCEPSet()gate inrunServeCmdguarantees at least one is set before the validator is ever reached.This is the last pure config validation left in
runServeCmdper the broader-plan note on #45583. RemaininginitFatalsites are runtime failure paths (datastore init, Redis init, MDM init wiring) which need the injection from #45343 — those would be the next slice.Related issue: Refs #33370
Checklist for submitter
Summary by CodeRabbit