update apple query for profile verification with include all/any + exclude#47079
Conversation
![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 updates the MySQL query used to compute expected Apple MDM profiles for verification, expanding label-scope handling to support combinations of include any/all with exclude-any.
Changes:
- Updated
getHostMDMAppleProfilesExpectedForVerificationSQL to separate “include-only”, “exclude-only”, and new “include + exclude” cases via additional UNION branches. - Removed Windows handling from
GetHostMDMProfilesExpectedForVerification(and removed corresponding Windows test coverage). - Extended the macOS labeled-profile verification test setup to include include+exclude scenarios.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| server/datastore/mysql/mdm.go | Expands Apple expected-profiles verification query with include/exclude combinations; removes Windows platform support from expected-profiles verification. |
| server/datastore/mysql/mdm_test.go | Adds macOS include+exclude test cases and removes Windows expected-profiles verification tests. |
Comments suppressed due to low confidence (1)
server/datastore/mysql/mdm.go:1397
- The "exclude-any only" branch doesn't implement the same dynamic-label freshness gate used elsewhere (e.g. Android/Windows desired-state queries): a dynamic exclude label created after
hosts.label_updated_atshould disqualify the profile, but this query will currently include it as long as the host isn't inlabel_membership. This can cause profiles to be incorrectly expected/verified before the host has reported label results. Consider joininghosts/labelsand adding a HAVING condition equivalent tocount_profile_labels = count_host_updated_after_labels(seeserver/datastore/mysql/android.goexclude-only HAVING).
macp.team_id = ? AND
NOT EXISTS (
SELECT 1 FROM mdm_configuration_profile_labels
WHERE apple_profile_uuid = macp.profile_uuid AND exclude = 0
)
GROUP BY
profile_uuid, identifier
HAVING
-- considers only the profiles with labels, without any broken label, and with the host not in any label
count_profile_labels > 0 AND
count_profile_labels = count_non_broken_labels AND
count_host_labels = 0
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
WalkthroughThe PR refines Apple MDM expected-profile verification by restructuring the SQL query to handle label combinations more precisely. The platform switch now explicitly rejects unsupported platforms. Single-label-type profiles (include-all-only, exclude-any-only) are refined with NOT EXISTS predicates to enforce mutual exclusivity with conflicting label types. New UNION branches handle mixed configurations (include-all + exclude-any, include-any + exclude-any) using conditional aggregation to enforce include requirements while rejecting profiles with disqualifying exclude labels. Test coverage is extended with 4 new profiles combining include and exclude rules, increasing expected profile count to 15. Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
Main and integration-mdm failures seems unrelated? One failing on TUF and another on an S3 test |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #47079 +/- ##
==========================================
- Coverage 67.05% 67.05% -0.01%
==========================================
Files 2887 2891 +4
Lines 225171 225197 +26
Branches 11636 11636
==========================================
+ Hits 150996 151012 +16
- Misses 60504 60511 +7
- Partials 13671 13674 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@MagnusHJensen I think the integrations-mdm error is legitimate. Looks like maybe the verification Windows code was being used in the test for soemthing time=2026-06-08T13:24:09.582Z level=INFO msg="Completing manage_windows_profiles job" test=TestIntegrationsMDM/TestHostMDMProfilesExcludeLabels time=2026-06-08T13:24:09Z |
|
@JordanMontgomery Just removed the windows part on that test, which should have been removed with the removal of Windows verification. It's never exercised in production code and hasn't been for a long time, so we are fine to remove the windows branch. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/service/integration_mdm_profiles_test.go (1)
6232-6234: ⚡ Quick winUpdate stale Windows wording in this Apple-only flow.
Line 6233 still references “the Windows host,” but this test path is now Apple-only. Keeping this comment accurate will prevent confusion when debugging this scenario.
✏️ Suggested comment cleanup
- // remove membership of [4] for Apple, meaning - // that only D3 will be installed on Apple (as the Windows host is still - // member of an excluded label) + // remove membership of [4] for Apple, meaning + // that only D3 will be installed on Apple.🤖 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 `@server/service/integration_mdm_profiles_test.go` around lines 6232 - 6234, Update the stale comment in integration_mdm_profiles_test.go that reads "the Windows host is still member of an excluded label": this flow is Apple-only, so change that phrase to accurately reflect the scenario (e.g., "the other host is still a member of an excluded label" or "a different host is still a member of an excluded label") near the comment starting "remove membership of [4] for Apple" so the comment no longer references a Windows host.
🤖 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.
Nitpick comments:
In `@server/service/integration_mdm_profiles_test.go`:
- Around line 6232-6234: Update the stale comment in
integration_mdm_profiles_test.go that reads "the Windows host is still member of
an excluded label": this flow is Apple-only, so change that phrase to accurately
reflect the scenario (e.g., "the other host is still a member of an excluded
label" or "a different host is still a member of an excluded label") near the
comment starting "remove membership of [4] for Apple" so the comment no longer
references a Windows host.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 33ac7504-faf2-4dd3-a5db-7ca6c09b0a31
📒 Files selected for processing (1)
server/service/integration_mdm_profiles_test.go
Related issue: Follow up PR for the Include Any/All + Exclude label story, since this was missed.
Checklist for submitter
If some of the following don't apply, delete the relevant line.
Changes file added for user-visible changes in
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Input data is properly validated,
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters.Timeouts are implemented and retries are limited to avoid infinite loops
If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes
Testing
Summary by CodeRabbit
Bug Fixes
Tests