create new appCfg entry on AB fleet updates if not found - #49559
Merged
Conversation
Contributor
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
This PR fixes GitOps export correctness for Apple Business Manager (ABM) default fleet assignments by ensuring the app config’s apple_business mapping is created/maintained when ABM defaults are updated via the UI, and keeps app config entries in sync on token deletion.
Changes:
- Update ABM token team update flow to create a new
apple_businessapp-config entry when none exists. - Update ABM token deletion flow to remove the corresponding
apple_businessapp-config entry. - Add a unit test covering the “entry not present” update scenario.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| ee/server/service/mdm.go | Syncs ABM token updates/deletes with app config apple_business entries. |
| ee/server/service/mdm_test.go | Adds coverage for creating a missing app-config entry on ABM team updates. |
| changes/48653-empty-ab-when-only-updated-via-ui | Excluded from review by policy (not accessed). |
Files excluded by content exclusion policy (1)
- changes/48653-empty-ab-when-only-updated-via-ui
💡 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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughABM token deletion now retrieves token metadata, removes the matching Apple Business assignment, saves the app configuration, and disables Apple Business Manager when the last token is deleted. Team updates now create missing assignment entries and persist their fleet names. Tests cover insertion of new assignments, and a changelog entry documents the GitOps export fix for UI-configured default fleets. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ee/server/service/mdm.go (1)
1785-1823: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFix "No team" strings leaking into GitOps output and properly set
optjsonflags.Two issues exist in the current implementation:
- When appending a new entry in the
!foundpath, the"No team"strings (fallback values for unassigned teams) are not cleared. This bypasses the"No team"cleanup logic running in thefoundbranch, which will cause GitOps to incorrectly exportmacos_team: "No team"instead of an empty string.- If
appCfg.MDM.AppleBusinessManagerwas never configured, appending to its.Valuewithout explicitly toggling.Set = trueand.Valid = truewill causeoptjsonto omit the field during JSON serialization, failing to persist the new assignment.Consolidate the cleanup logic before the condition to ensure newly appended entries are properly formatted and the
optjsonslice is marked as set.🛠️ Proposed fix
- var found bool - for i, appCfgToken := range appCfg.MDM.AppleBusinessManager.Value { - if appCfgToken.OrganizationName == token.OrganizationName { - - // Clear no team names, so they are presented nicer in gitops. - appCfgToken.BYODTeam = token.BYODTeam.Name - if token.BYODTeam.Name == fleet.TeamNameNoTeam { - appCfgToken.BYODTeam = "" - } - appCfgToken.MacOSTeam = token.MacOSTeam.Name - if token.MacOSTeam.Name == fleet.TeamNameNoTeam { - appCfgToken.MacOSTeam = "" - } - appCfgToken.IOSTeam = token.IOSTeam.Name - if token.IOSTeam.Name == fleet.TeamNameNoTeam { - appCfgToken.IOSTeam = "" - } - appCfgToken.IpadOSTeam = token.IPadOSTeam.Name - if token.IPadOSTeam.Name == fleet.TeamNameNoTeam { - appCfgToken.IpadOSTeam = "" - } - - // update the app config with the new team names - appCfg.MDM.AppleBusinessManager.Value[i] = appCfgToken - found = true - break - } - } - - if !found { - // create a new entry if app config doesn't have one. - appCfg.MDM.AppleBusinessManager.Value = append(appCfg.MDM.AppleBusinessManager.Value, fleet.MDMAppleABMAssignmentInfo{ - OrganizationName: token.OrganizationName, - BYODTeam: token.BYODTeam.Name, - MacOSTeam: token.MacOSTeam.Name, - IOSTeam: token.IOSTeam.Name, - IpadOSTeam: token.IPadOSTeam.Name, - }) - } + appCfgToken := fleet.MDMAppleABMAssignmentInfo{ + OrganizationName: token.OrganizationName, + BYODTeam: token.BYODTeam.Name, + MacOSTeam: token.MacOSTeam.Name, + IOSTeam: token.IOSTeam.Name, + IpadOSTeam: token.IPadOSTeam.Name, + } + + // Clear no team names, so they are presented nicer in gitops. + if appCfgToken.BYODTeam == fleet.TeamNameNoTeam { + appCfgToken.BYODTeam = "" + } + if appCfgToken.MacOSTeam == fleet.TeamNameNoTeam { + appCfgToken.MacOSTeam = "" + } + if appCfgToken.IOSTeam == fleet.TeamNameNoTeam { + appCfgToken.IOSTeam = "" + } + if appCfgToken.IpadOSTeam == fleet.TeamNameNoTeam { + appCfgToken.IpadOSTeam = "" + } + + var found bool + for i, existing := range appCfg.MDM.AppleBusinessManager.Value { + if existing.OrganizationName == token.OrganizationName { + // update the app config with the new team names + appCfg.MDM.AppleBusinessManager.Value[i] = appCfgToken + found = true + break + } + } + + if !found { + // create a new entry if app config doesn't have one. + appCfg.MDM.AppleBusinessManager.Value = append(appCfg.MDM.AppleBusinessManager.Value, appCfgToken) + } + + appCfg.MDM.AppleBusinessManager.Set = true + appCfg.MDM.AppleBusinessManager.Valid = true🤖 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/mdm.go` around lines 1785 - 1823, Consolidate the team-name cleanup before the found/not-found branch so BYODTeam, MacOSTeam, IOSTeam, and IpadOSTeam store empty strings for fleet.TeamNameNoTeam in both update and append paths. In the !found path, append the cleaned values and explicitly set appCfg.MDM.AppleBusinessManager.Set and Valid to true so optjson serializes the new assignment.
🧹 Nitpick comments (2)
ee/server/service/mdm.go (1)
1658-1663: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSet
optjsonwrapper flags explicitly.When mutating the underlying
.Valueof anoptjson.Slice, it is a good practice to explicitly flag.Setand.Validas true to ensure the slice safely marshals to JSON on save, especially if the slice becomes empty.🛠️ Proposed refactor
for i, t := range appCfg.MDM.AppleBusinessManager.Value { if t.OrganizationName == token.OrganizationName { appCfg.MDM.AppleBusinessManager.Value = append(appCfg.MDM.AppleBusinessManager.Value[:i], appCfg.MDM.AppleBusinessManager.Value[i+1:]...) + appCfg.MDM.AppleBusinessManager.Set = true + appCfg.MDM.AppleBusinessManager.Valid = true break } }🤖 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/mdm.go` around lines 1658 - 1663, After removing the matching entry in the Apple Business Manager slice within the MDM configuration update flow, explicitly set the enclosing optjson.Slice’s Set and Valid flags to true so the mutated value, including an empty slice, marshals correctly when saved.ee/server/service/mdm_test.go (1)
582-605: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest missing assignment creation with
nilteam parameters.To complement the test for valid team IDs, consider adding a subtest verifying that a missing app config entry initialized with
nilteam IDs successfully scrubs"No team"strings and leaves the fields empty. This prevents future regressions of the issue flagged inmdm.go.🧪 Proposed test case addition
assert.Equal(t, validTeamName, appCfgToken.MacOSTeam) assert.Equal(t, validTeamName, appCfgToken.IOSTeam) assert.Equal(t, validTeamName, appCfgToken.IpadOSTeam) }) + + t.Run("updates app config with new entry if not present (nil teams)", func(t *testing.T) { + ds.SaveAppConfigFuncInvoked = false + appCfg.MDM.AppleBusinessManager = optjson.SetSlice([]fleet.MDMAppleABMAssignmentInfo{}) + + token, err := svc.UpdateABMTokenTeams(ctx, tokenID, nil, nil, nil, nil) + require.NoError(t, err) + + assert.Nil(t, token.BYODDefaultTeamID) + assert.Nil(t, token.MacOSDefaultTeamID) + assert.Nil(t, token.IOSDefaultTeamID) + assert.Nil(t, token.IPadOSDefaultTeamID) + require.True(t, ds.SaveAppConfigFuncInvoked) + + var appCfgToken fleet.MDMAppleABMAssignmentInfo + for _, tok := range updatedAppCfg.MDM.AppleBusinessManager.Value { + if tok.OrganizationName == orgName { + appCfgToken = tok + break + } + } + assert.Empty(t, appCfgToken.BYODTeam) + assert.Empty(t, appCfgToken.MacOSTeam) + assert.Empty(t, appCfgToken.IOSTeam) + assert.Empty(t, appCfgToken.IpadOSTeam) + }) }🤖 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/mdm_test.go` around lines 582 - 605, The UpdateABMTokenTeams tests should also cover creating a missing AppleBusinessManager assignment when all team parameters are nil. Add a subtest alongside “updates app config with new entry if not present” that invokes UpdateABMTokenTeams with nil team IDs, verifies success, and confirms the newly created assignment has empty team fields rather than “No team” strings.
🤖 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.
Outside diff comments:
In `@ee/server/service/mdm.go`:
- Around line 1785-1823: Consolidate the team-name cleanup before the
found/not-found branch so BYODTeam, MacOSTeam, IOSTeam, and IpadOSTeam store
empty strings for fleet.TeamNameNoTeam in both update and append paths. In the
!found path, append the cleaned values and explicitly set
appCfg.MDM.AppleBusinessManager.Set and Valid to true so optjson serializes the
new assignment.
---
Nitpick comments:
In `@ee/server/service/mdm_test.go`:
- Around line 582-605: The UpdateABMTokenTeams tests should also cover creating
a missing AppleBusinessManager assignment when all team parameters are nil. Add
a subtest alongside “updates app config with new entry if not present” that
invokes UpdateABMTokenTeams with nil team IDs, verifies success, and confirms
the newly created assignment has empty team fields rather than “No team”
strings.
In `@ee/server/service/mdm.go`:
- Around line 1658-1663: After removing the matching entry in the Apple Business
Manager slice within the MDM configuration update flow, explicitly set the
enclosing optjson.Slice’s Set and Valid flags to true so the mutated value,
including an empty slice, marshals correctly when saved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e342e5a1-dc05-4db1-aad6-bdbd9828f212
📒 Files selected for processing (3)
changes/48653-empty-ab-when-only-updated-via-uiee/server/service/mdm.goee/server/service/mdm_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #49559 +/- ##
==========================================
- Coverage 67.82% 67.82% -0.01%
==========================================
Files 3890 3890
Lines 247624 247661 +37
Branches 12981 13138 +157
==========================================
+ Hits 167951 167964 +13
- Misses 64516 64529 +13
- Partials 15157 15168 +11
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:
|
1 task
JordanMontgomery
approved these changes
Jul 21, 2026
12 tasks
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 #48653
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
generate-gitopsfrom exporting an emptyapple_businesssection when default fleets are configured only via the UI.