Fixes from Konstantin's code review - #46701
Merged
Merged
Conversation
Member
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Member
Author
|
/agentic_review |
Code Review by Qodo
1.
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors MDM enrollment status handling and Android wipe validation to reduce duplicated logic and replace hard-coded status strings with shared constants, aligning Fleet Free and Premium implementations.
Changes:
- Centralize Android-specific wipe validations into a shared helper (
fleet.ValidateAndroidWipeRequest) used by both Free and Premium WipeHost paths. - Introduce
fleet.MDMEnrollmentStatus*string constants and update comparisons to use them instead of raw string literals. - Update Apple MDM and MySQL datastore logic to use the shared
MDMEnrollmentStatusPendingconstant.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/scripts.go | Uses shared Android wipe validation helper for Fleet Free wipe flow. |
| server/service/hosts.go | Replaces Android BYOD enrollment status string comparison with a constant. |
| server/service/apple_mdm.go | Replaces "Pending" enrollment status comparison with a constant. |
| server/fleet/hosts.go | Adds shared enrollment status constants and the shared Android wipe validation helper. |
| server/datastore/mysql/apple_mdm.go | Replaces "Pending" comparison with a constant when tracking pending hosts. |
| ee/server/service/hosts.go | Reuses shared Android wipe validation helper and replaces enrollment status string comparisons with constants. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
|
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 (1)
WalkthroughThis PR adds exported MDM enrollment-status constants and a ValidateAndroidWipeRequest(ctx, ds, host) helper. It updates HostMDM.EnrollmentStatus() to return the new constants, replaces literal status string comparisons across Apple flows and DEP assignment deletion, and refactors Android wipe prechecks to call the new validation helper (wrapping validation errors where used). Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 4❌ Failed checks (2 warnings, 2 inconclusive)
✅ Passed checks (1 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ee/server/service/hosts.go (1)
67-73: ⚡ Quick winFinish the status-constant migration in this file.
This switches
LockHosttofleet.MDMEnrollmentStatus*, butWipeHoststill compares against the raw"On (personal)"string on Line 285. That leaves two sources of truth for the same enrollment state in the same file.♻️ Suggested follow-up
- if host.MDM.EnrollmentStatus != nil && *host.MDM.EnrollmentStatus == "On (personal)" { + if host.MDM.EnrollmentStatus != nil && *host.MDM.EnrollmentStatus == fleet.MDMEnrollmentStatusPersonal {🤖 Prompt for 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. In `@ee/server/service/hosts.go` around lines 67 - 73, WipeHost still compares host enrollment using the raw string "On (personal)" while LockHost was migrated to use fleet.MDMEnrollmentStatus* constants; update WipeHost to use the same constants instead of raw strings—replace any comparisons against "On (personal)" with checks against fleet.MDMEnrollmentStatusPersonal and any checks for the manual state with fleet.MDMEnrollmentStatusManual, using the same host.MDM.EnrollmentStatus nil-safe pattern as in LockHost to ensure consistent single source of truth.
🤖 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.go`:
- Around line 2243-2245: The code builds pendingHostIDs using the constant
fleet.MDMEnrollmentStatusPending but later checks byStatus["Pending"], causing a
mismatch; update the lookup to use the same constant value (use
fleet.MDMEnrollmentStatusPending or dereferenced host.EnrollmentStatus value)
when gating/logging and when accessing byStatus so the pending bucket is read
consistently (update the occurrences around byStatus lookups and the gate/log
that currently uses the literal "Pending" — ensure both the construction of
byStatus keys and the lookup use fleet.MDMEnrollmentStatusPending or the actual
EnrollmentStatus value).
---
Nitpick comments:
In `@ee/server/service/hosts.go`:
- Around line 67-73: WipeHost still compares host enrollment using the raw
string "On (personal)" while LockHost was migrated to use
fleet.MDMEnrollmentStatus* constants; update WipeHost to use the same constants
instead of raw strings—replace any comparisons against "On (personal)" with
checks against fleet.MDMEnrollmentStatusPersonal and any checks for the manual
state with fleet.MDMEnrollmentStatusManual, using the same
host.MDM.EnrollmentStatus nil-safe pattern as in LockHost to ensure consistent
single source of truth.
🪄 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: 8b815ff9-21e1-45d4-915e-f5c36e6a4f67
📒 Files selected for processing (6)
ee/server/service/hosts.goserver/datastore/mysql/apple_mdm.goserver/fleet/hosts.goserver/service/apple_mdm.goserver/service/hosts.goserver/service/scripts.go
![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.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #46701 +/- ##
==========================================
+ Coverage 66.91% 66.98% +0.06%
==========================================
Files 2836 2826 -10
Lines 225064 224540 -524
Branches 11619 11401 -218
==========================================
- Hits 150610 150407 -203
+ Misses 60789 60469 -320
+ Partials 13665 13664 -1
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:
|
ksykulev
reviewed
Jun 3, 2026
ksykulev
left a comment
ksykulev
left a comment
Contributor
There was a problem hiding this comment.
ee/server/service/hosts.go:285
if host.MDM.EnrollmentStatus != nil && *host.MDM.EnrollmentStatus == "On (personal)" {
is the only one missing that I detected with grep
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
ksykulev
approved these changes
Jun 4, 2026
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
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.
Related issue: Resolves #41683
Summary by CodeRabbit
Refactoring
Bug Fixes