42508 Rename abm to ab in API - #46657
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #46657 +/- ##
==========================================
+ Coverage 66.89% 66.91% +0.02%
==========================================
Files 2815 2834 +19
Lines 224397 225018 +621
Branches 11356 11622 +266
==========================================
+ Hits 150116 150580 +464
- Misses 60650 60782 +132
- Partials 13631 13656 +25
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:
|
# Conflicts: # frontend/interfaces/team.ts
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@claude review once |
There was a problem hiding this comment.
Pull request overview
Renames Apple Business Manager (ABM) terminology and surface area to Apple Business (AB) across the Fleet API, GitOps YAML, frontend, and fleetctl, while preserving backwards compatibility via deprecated path aliases and JSON key duplication. It also extends the JSON key duplicator with an inline option to support multi-level renames without breaking older clients that expect old/new nested keys to co-exist under the deprecated container.
Changes:
- Introduce canonical AB API routes (
/ab_tokens,/mdm/apple/ab_public_key) and add deprecated-path aliasing for legacy ABM routes. - Add
renameto:",inline"support to preserve prior response shapes when multiple nested renames exist across versions. - Update GitOps key migrations/validation, frontend API usage/types, and
fleetctlcommands (mdm-ab) with deprecatedmdm-apple-bmaliases.
Reviewed changes
Copilot reviewed 49 out of 50 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/integration_mdm_test.go | Updates integration test to use /ab_public_key and adds deprecated public-key path coverage. |
| server/service/handler.go | Switches canonical routes from ABM to AB (/ab_tokens, /ab_public_key). |
| server/service/handler_deprecated_paths.go | Adds deprecated path aliases mapping ABM routes to AB routes. |
| server/service/client.go | Adjusts GitOps client-side defaulting to use apple_business. |
| server/service/client_mdm.go | Updates fleetctl client calls to canonical AB endpoints. |
| server/service/apple_mdm.go | Adds renameto (with inline) to ABM token response fields so responses include both old/new names. |
| server/service/appconfig.go | Updates validation error paths from apple_business_manager to apple_business. |
| server/platform/endpointer/json_key_rewriter.go | Documents/handles Inline behavior (explicitly ignored for pure renames). |
| server/platform/endpointer/json_key_duplicator.go | Implements Inline container duplication to support multi-level rename compatibility. |
| server/platform/endpointer/json_key_duplicator_test.go | Adds coverage for inline multi-level container rename behavior. |
| server/platform/endpointer/extract_alias_rules_test.go | Adds tests for parsing renameto:",inline" option. |
| server/platform/endpointer/endpoint_utils.go | Parses renameto options and records Inline in alias rules. |
| server/fleet/errors.go | Updates deprecation guidance message to point to mdm.apple_business. |
| server/fleet/apple_mdm.go | Adds renameto:"ab_token_id" for AB token id while keeping legacy key. |
| server/fleet/app.go | Renames MDM config key via renameto:"apple_business,inline" and updates comments. |
| pkg/spec/gitops_validate.go | Ensures GitOps validator registers renameto aliases without options like ,inline. |
| pkg/spec/gitops_validate_test.go | Adds test ensuring ,inline isn’t treated as part of the alias key. |
| pkg/spec/gitops_test.go | Adds GitOps tests covering new key acceptance, old key support, and conflict error. |
| pkg/spec/gitops_deprecations.go | Adds parent key mapping apple_business_manager → apple_business plus nested fleet mappings. |
| frontend/utilities/endpoints.ts | Switches frontend AB endpoints to /ab_tokens and /ab_public_key. |
| frontend/services/entities/mdm_apple_bm.ts | Updates frontend AB service types/requests to use AB endpoints and ab_tokens. |
| frontend/pages/admin/IntegrationsPage/cards/MdmSettings/VppPage/components/VppTable/VppTableConfig.tsx | Updates imports/types (but currently introduces a type regression—see comments). |
| frontend/pages/admin/IntegrationsPage/cards/MdmSettings/AppleBusinessManagerPage/components/EditTeamsAbmModal/EditTeamsAbmModal.tsx | Updates modal to use new AB token shape (*_fleet). |
| frontend/pages/admin/IntegrationsPage/cards/MdmSettings/AppleBusinessManagerPage/components/AppleBusinessManagerTable/AppleBusinessManagerTableConfig.tsx | Updates AB table config to new AB token shape and fleet display name. |
| frontend/pages/admin/IntegrationsPage/cards/MdmSettings/AppleBusinessManagerPage/components/AppleBusinessManagerTable/AppleBusinessManagerTable.tsx | Renames props/types to AB token model. |
| frontend/pages/admin/IntegrationsPage/cards/MdmSettings/AppleBusinessManagerPage/AppleBusinessManagerPage.tsx | Migrates page query/props from abm_tokens to ab_tokens. |
| frontend/interfaces/team.ts | Adds getFleetDisplayName helper for fleet-shaped token associations. |
| frontend/interfaces/mdm.ts | Introduces ITokenFleet and renames ABM token interface to IMdmAbToken. |
| frontend/components/App/App.tsx | Updates global AB token query to use new response shape (ab_tokens). |
| ee/server/service/mdm.go | Updates deprecation error message to point to /ab_tokens. |
| cmd/fleetctl/fleetctl/testdata/macosSetupExpectedAppConfigSet.yml | Updates expected config output to include apple_business. |
| cmd/fleetctl/fleetctl/testdata/macosSetupExpectedAppConfigEmpty.yml | Updates expected config output to include apple_business. |
| cmd/fleetctl/fleetctl/testdata/generateGitops/test_dir_premium/default.yml | Updates generated GitOps YAML to use apple_business. |
| cmd/fleetctl/fleetctl/testdata/generateGitops/expectedOrgSettings.yaml | Updates expected GitOps output to use apple_business. |
| cmd/fleetctl/fleetctl/testdata/generateGitops/expectedOrgSettings-insecure.yaml | Updates expected GitOps output to use apple_business. |
| cmd/fleetctl/fleetctl/testdata/expectedGetConfigIncludeServerConfigYaml.yml | Updates expected get-config YAML to include apple_business. |
| cmd/fleetctl/fleetctl/testdata/expectedGetConfigIncludeServerConfigJson.json | Updates expected get-config JSON to include apple_business. |
| cmd/fleetctl/fleetctl/testdata/expectedGetConfigAppConfigYaml.yml | Updates expected app config YAML to include apple_business. |
| cmd/fleetctl/fleetctl/testdata/expectedGetConfigAppConfigTeamMaintainerYaml.yml | Updates expected team-maintainer YAML to include apple_business. |
| cmd/fleetctl/fleetctl/testdata/expectedGetConfigAppConfigTeamMaintainerJson.json | Updates expected team-maintainer JSON to include apple_business. |
| cmd/fleetctl/fleetctl/testdata/expectedGetConfigAppConfigJson.json | Updates expected app config JSON to include apple_business. |
| cmd/fleetctl/fleetctl/templates/new/default.template.yml | Updates starter template to use apple_business. |
| cmd/fleetctl/fleetctl/gitops.go | Updates GitOps processing to treat apple_business as the new canonical key. |
| cmd/fleetctl/fleetctl/gitops_test.go | Updates GitOps tests for new error messages/key names. |
| cmd/fleetctl/fleetctl/get.go | Adds fleetctl get mdm-ab and deprecates mdm-apple-bm (aliasing to new implementation). |
| cmd/fleetctl/fleetctl/get_test.go | Updates tests to match revised wording and /ab_tokens guidance. |
| cmd/fleetctl/fleetctl/generate.go | Adds fleetctl generate mdm-ab and deprecates mdm-apple-bm. |
| cmd/fleetctl/fleetctl/generate_gitops.go | Ensures canonical field name extraction strips ,inline options. |
| cmd/fleetctl/fleetctl/generate_gitops_test.go | Updates expected keys for generated GitOps output. |
| changes/42508-rename-abm-to-ab | Adds changelog entry describing the ABM→AB rename and backwards-compat behavior. |
Comments suppressed due to low confidence (1)
server/service/integration_mdm_test.go:12002
- This test exercises the deprecated
/abm_tokenspath but does not currently validate that the new canonical/ab_tokensendpoint works. Since this PR changes the canonical route registration, it’s worth asserting the new path returns the same token and using it for the delete call.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -191,9 +191,10 @@ type MDM struct { | |||
| // Deprecated: use AppleBussinessManager instead | |||
|
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)
WalkthroughThis PR renames Apple Business Manager (ABM) to Apple Business (AB) across server APIs, DTOs, GitOps keys, fleetctl commands, and frontend consumers. It adds canonical endpoints (/ab_tokens, /mdm/apple/ab_public_key), the mdm.apple_business GitOps key, and fleetctl mdm-ab get/generate subcommands; legacy abm_* paths, keys, and CLI names remain supported via deprecated aliases that emit conditional deprecation notices. renameto tag parsing now strips comma options (e.g., ",inline") and response duplication was made inline-aware so responses can expose both old and new JSON keys without cross-contamination. 🚥 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
cmd/fleetctl/fleetctl/templates/new/default.template.yml (1)
63-70:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep the starter template comments on the canonical AB name.
This block now uses
apple_business, but the surrounding “Read more” comments still point readers to Apple Business Manager wording. That leaves the generated template teaching the deprecated name right next to the canonical key.Suggested comment update
- # Read more: - # • https://fleetdm.com/docs/configuration/yaml-files#apple-business-manager - # • https://fleetdm.com/guides/apple-mdm-setup#apple-business-manager-abm + # Read more: + # • https://fleetdm.com/docs/configuration/yaml-files#apple-business + # • https://fleetdm.com/guides/apple-mdm-setup#apple-business-ab🤖 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 `@cmd/fleetctl/fleetctl/templates/new/default.template.yml` around lines 63 - 70, The template's explanatory comments reference the Apple Business Manager wording while the config block uses the canonical key apple_business, which can confuse users; update the surrounding "Read more" and explanatory comment lines to clearly state that the canonical config key is apple_business (not the deprecated name), remove or rephrase any wording that suggests the old/deprecated key, and ensure the comment directs readers to the same documentation URLs while explicitly noting the canonical key apple_business so the template and comments match.frontend/pages/admin/IntegrationsPage/cards/MdmSettings/AppleBusinessManagerPage/components/EditTeamsAbmModal/EditTeamsAbmModal.tsx (1)
148-190:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFinish the UI copy rename in these tooltips.
These strings still say “from ABM”, so the canonical Apple Business flow still surfaces the deprecated product name in the modal.
🤖 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 `@frontend/pages/admin/IntegrationsPage/cards/MdmSettings/AppleBusinessManagerPage/components/EditTeamsAbmModal/EditTeamsAbmModal.tsx` around lines 148 - 190, In EditTeamsAbmModal, update the tooltip copy for each Dropdown (the macOS, iOS, and iPadOS Dropdown components) so the phrase "from ABM" is replaced with the new canonical name ("from Apple Business" or "from Apple Business Manager") in the tooltip JSX passed to the tooltip prop; search for the Dropdown usages inside the EditTeamsAbmModal component (labels "macOS fleet", "iOS fleet", "iPadOS fleet") and change the string fragments that read "from ABM" to the updated product name so all three tooltips are consistent.frontend/components/App/App.tsx (1)
125-143:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClear the cached AB expiry state when there are no usable tokens.
This query only updates
setABMExpiryfor non-emptyab_tokensor a400response. If a refetch later returns[]or a different error after a prior successful load, the old expiry data stays inAppContextand the renewal banner can remain visible for the rest of the session. Please resetsetABMExpiryto the same empty/default state used by the context in those branches.🤖 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 `@frontend/components/App/App.tsx` around lines 125 - 143, The onSuccess/onError handlers only update setABMExpiry when ab_tokens is non-empty or on a 400 error, leaving stale expiry data in context if a later fetch returns [] or another error; update the onSuccess branch to handle an empty ab_tokens array by calling setABMExpiry with the context's empty/default state (e.g., earliestExpiry: null and needsAbmTermsRenewal: false) and update onError to reset to that same default for non-400 errors as well (use the same default object consistently instead of leaving old values), referencing the existing onSuccess, onError, setABMExpiry, ab_tokens and GUARANTEED_PAST_DATE symbols to locate where to apply the change.
🧹 Nitpick comments (4)
pkg/spec/gitops_test.go (1)
3792-3823: ⚡ Quick winCover the deprecated child-key path too.
This regression test only proves that
apple_business_managerparses when the nested fields are already using the new*_fleetnames. The new logic inDeprecatedGitOpsKeyMappingsis the parent-first migration of the actual deprecated child keys (macos_team,ios_team,ipados_team), so a break there would still pass this suite.Suggested test expansion
t.Run("old_key_still_accepted", func(t *testing.T) { t.Parallel() dir := t.TempDir() yaml := baseConfig(` apple_business_manager: - organization_name: Test Org - macos_fleet: "Workstations" - ios_fleet: "Phones" - ipados_fleet: "Tablets"`) + macos_team: "Workstations" + ios_team: "Phones" + ipados_team: "Tablets"`) yamlPath := filepath.Join(dir, "gitops.yml") require.NoError(t, os.WriteFile(yamlPath, []byte(yaml), 0o644)) - _, err := GitOpsFromFile(yamlPath, dir, nil, nopLogf) + gitops, err := GitOpsFromFile(yamlPath, dir, nil, nopLogf) require.NoError(t, err) + mdm := gitops.OrgSettings["mdm"].(map[string]any) + appleBusiness := mdm["apple_business"].([]any) + entry := appleBusiness[0].(map[string]any) + require.Equal(t, "Workstations", entry["macos_fleet"]) + require.Equal(t, "Phones", entry["ios_fleet"]) + require.Equal(t, "Tablets", entry["ipados_fleet"]) })🤖 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 `@pkg/spec/gitops_test.go` around lines 3792 - 3823, Add tests that exercise the deprecated child-key migration (macos_team, ios_team, ipados_team) in addition to the existing parent-key tests: create a new subtest (or extend "old_key_still_accepted") which writes a gitops.yml where apple_business_manager entries use macos_team/ios_team/ipados_team (not *_fleet) and assert GitOpsFromFile(yamlPath, dir, nil, nopLogf) returns no error; also add a conflicting-subtest that writes a yaml containing both the new parent-style keys (e.g., macos_fleet/ios_fleet/ipados_fleet or apple_business) and the deprecated child keys in the same resource and assert GitOpsFromFile returns an error containing "cannot specify both" and the expected key path (org_settings.mdm.apple_business) so DeprecatedGitOpsKeyMappings migration logic is covered. Reference the existing test functions "old_key_still_accepted" and "both_keys_conflict" and the called function "GitOpsFromFile" to locate where to add these cases.cmd/fleetctl/fleetctl/generate_gitops_test.go (1)
2143-2149: ⚡ Quick winAssert that the deprecated alias is absent from generated GitOps output.
This now verifies the canonical
apple_businesskey is present, but it still won't catch a regression wheregenerateMDMemits bothapple_businessandapple_business_manager. Sincegenerate-gitopsis supposed to write the canonical key only, add a negative assertion for the deprecated alias here too.Suggested test hardening
for _, key := range []string{ "apple_business", "apple_server_url", "end_user_authentication", "end_user_license_agreement", "volume_purchasing_program", } { require.Contains(t, mdmRaw, key) require.Empty(t, mdmRaw[key]) } + _, hasDeprecatedAlias := mdmRaw["apple_business_manager"] + require.False(t, hasDeprecatedAlias)🤖 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 `@cmd/fleetctl/fleetctl/generate_gitops_test.go` around lines 2143 - 2149, The test currently asserts presence of the canonical apple_business key but not absence of the deprecated alias; update the test in generate_gitops_test.go (the loop that checks keys such as "apple_business" and related MDM entries) to also assert that the deprecated "apple_business_manager" key is not present in the generated GitOps output produced by generateMDM/generate-gitops; add a negative assertion (e.g., ensure output string or map does not contain "apple_business_manager") immediately alongside the positive assertion for "apple_business" so the test fails if both keys are emitted.server/platform/endpointer/json_key_duplicator_test.go (1)
192-229: ⚡ Quick winAssert the
ios_*branch in this regression test too.This case seeds both
macos_teamandios_team, but it only verifies themacos_*rename path. A break inios_team → ios_fleetwould still pass here, which weakens the coverage for the exact multi-container payload this PR is protecting.🤖 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/platform/endpointer/json_key_duplicator_test.go` around lines 192 - 229, The test case "MultiLevelRenamedContainers" only asserts the macos_* path; add symmetric assertions for the ios_* branch inside the validate func: after checking macos_team/mac os_fleet, assert ios_team still has team_id and not fleet_id, assert ios_fleet has fleet_id and not team_id, and in the new ab_tokens entry verify it contains ios_fleet (with fleet_id) and does not contain ios_team; update variable references (e.g., iosTeam, iosFleet, ab, newTok) accordingly to mirror the macos checks so the ios_team → ios_fleet rename is covered.cmd/fleetctl/fleetctl/get_test.go (1)
2542-2615: ⚡ Quick winAdd a direct test for the new
get mdm-abcommand.This block only validates the deprecated
mdm_apple_bmpath. Since this PR’s user-facing contract is the new canonicalmdm-abcommand, it’s worth adding an equivalent happy-path assertion for that entrypoint so the rename itself is covered, not just the backward-compatible wrapper.🤖 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 `@cmd/fleetctl/fleetctl/get_test.go` around lines 2542 - 2615, TestGetAppleBM only exercises the deprecated "mdm_apple_bm" command; add equivalent assertions exercising the new "mdm-ab" entrypoint so the rename is covered. For the same subtests that set up the test server and mock datastore (the "premium license, single token", "premium license, no token", and "premium license, multiple tokens" cases), invoke runAppForTest or runAppNoChecks with []string{"get", "mdm-ab"} and assert the same expected outputs (e.g., check for "Apple ID:", "Organization name:", "MDM server URL:", "Renew date:", "Default team:" for the single-token happy path; "No Apple Business (AB) server token found." for no-token; and the deprecation/error for multiple tokens). Use the existing TestGetAppleBM test, ds.ListABMTokensFunc mock and the runAppForTest/runAppNoChecks helpers to implement these additional assertions.
🤖 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/fleetctl/fleetctl/generate.go`:
- Around line 117-124: The default output path for the Apple Business public key
is still using the deprecated "bm" term; update the flag default in
generateMDMABFlags (the &cli.StringFlag with Name "public-key") to use the
AB-named path (e.g. "fleet-apple-mdm-ab-public-key.crt") instead of
bmPublicKeyCertPath, and if bmPublicKeyCertPath is a shared constant rename or
replace it with a new constant (e.g. mdmABPublicKeyCertPath) and update all
references to avoid leaving the deprecated "bm" name in artifacts.
In `@cmd/fleetctl/fleetctl/get.go`:
- Around line 1689-1699: The CLI still prints "Default team"/"No team": update
the user-visible strings in the get MDN output by changing the fallback value
for defaultTeam (currently set from bm.DefaultTeam) from "No team" to "No fleet"
and change the key label passed to printKeyValueTable from "Default team:" to
"Default fleet:" (look for variables defaultTeam, bm.DefaultTeam and the
printKeyValueTable call to locate the code).
In
`@frontend/pages/admin/IntegrationsPage/cards/MdmSettings/VppPage/components/VppTable/VppTableConfig.tsx`:
- Line 4: generateTableData currently accepts AB tokens (IMdmAbToken[]) which
breaks the VPP table contract; change its signature and any related type
annotations back to IMdmVppToken[] so generateTableData(tokenList:
IMdmVppToken[]) (and any exported type aliases/import usage) matches the VPP
pipeline. Update callers that mistakenly pass IMdmAbToken to convert or map to
IMdmVppToken before calling, and restore any internal assumptions that rely on
IMdmVppToken fields so the VPP table shape is enforced.
In `@server/service/integration_mdm_test.go`:
- Around line 11992-11996: The test only checks the deprecated endpoint returns
a key but not that it matches the canonical endpoint; call the canonical handler
(GET "/api/latest/fleet/mdm/apple/ab_public_key") into a separate variable
(e.g., canonicalResp of type generateABMKeyPairResponse) and assert
deprecatedResp.PublicKey equals canonicalResp.PublicKey and deprecatedResp.Err
equals canonicalResp.Err so the test ensures the deprecated route is an alias of
the canonical route.
---
Outside diff comments:
In `@cmd/fleetctl/fleetctl/templates/new/default.template.yml`:
- Around line 63-70: The template's explanatory comments reference the Apple
Business Manager wording while the config block uses the canonical key
apple_business, which can confuse users; update the surrounding "Read more" and
explanatory comment lines to clearly state that the canonical config key is
apple_business (not the deprecated name), remove or rephrase any wording that
suggests the old/deprecated key, and ensure the comment directs readers to the
same documentation URLs while explicitly noting the canonical key apple_business
so the template and comments match.
In `@frontend/components/App/App.tsx`:
- Around line 125-143: The onSuccess/onError handlers only update setABMExpiry
when ab_tokens is non-empty or on a 400 error, leaving stale expiry data in
context if a later fetch returns [] or another error; update the onSuccess
branch to handle an empty ab_tokens array by calling setABMExpiry with the
context's empty/default state (e.g., earliestExpiry: null and
needsAbmTermsRenewal: false) and update onError to reset to that same default
for non-400 errors as well (use the same default object consistently instead of
leaving old values), referencing the existing onSuccess, onError, setABMExpiry,
ab_tokens and GUARANTEED_PAST_DATE symbols to locate where to apply the change.
In
`@frontend/pages/admin/IntegrationsPage/cards/MdmSettings/AppleBusinessManagerPage/components/EditTeamsAbmModal/EditTeamsAbmModal.tsx`:
- Around line 148-190: In EditTeamsAbmModal, update the tooltip copy for each
Dropdown (the macOS, iOS, and iPadOS Dropdown components) so the phrase "from
ABM" is replaced with the new canonical name ("from Apple Business" or "from
Apple Business Manager") in the tooltip JSX passed to the tooltip prop; search
for the Dropdown usages inside the EditTeamsAbmModal component (labels "macOS
fleet", "iOS fleet", "iPadOS fleet") and change the string fragments that read
"from ABM" to the updated product name so all three tooltips are consistent.
---
Nitpick comments:
In `@cmd/fleetctl/fleetctl/generate_gitops_test.go`:
- Around line 2143-2149: The test currently asserts presence of the canonical
apple_business key but not absence of the deprecated alias; update the test in
generate_gitops_test.go (the loop that checks keys such as "apple_business" and
related MDM entries) to also assert that the deprecated "apple_business_manager"
key is not present in the generated GitOps output produced by
generateMDM/generate-gitops; add a negative assertion (e.g., ensure output
string or map does not contain "apple_business_manager") immediately alongside
the positive assertion for "apple_business" so the test fails if both keys are
emitted.
In `@cmd/fleetctl/fleetctl/get_test.go`:
- Around line 2542-2615: TestGetAppleBM only exercises the deprecated
"mdm_apple_bm" command; add equivalent assertions exercising the new "mdm-ab"
entrypoint so the rename is covered. For the same subtests that set up the test
server and mock datastore (the "premium license, single token", "premium
license, no token", and "premium license, multiple tokens" cases), invoke
runAppForTest or runAppNoChecks with []string{"get", "mdm-ab"} and assert the
same expected outputs (e.g., check for "Apple ID:", "Organization name:", "MDM
server URL:", "Renew date:", "Default team:" for the single-token happy path;
"No Apple Business (AB) server token found." for no-token; and the
deprecation/error for multiple tokens). Use the existing TestGetAppleBM test,
ds.ListABMTokensFunc mock and the runAppForTest/runAppNoChecks helpers to
implement these additional assertions.
In `@pkg/spec/gitops_test.go`:
- Around line 3792-3823: Add tests that exercise the deprecated child-key
migration (macos_team, ios_team, ipados_team) in addition to the existing
parent-key tests: create a new subtest (or extend "old_key_still_accepted")
which writes a gitops.yml where apple_business_manager entries use
macos_team/ios_team/ipados_team (not *_fleet) and assert
GitOpsFromFile(yamlPath, dir, nil, nopLogf) returns no error; also add a
conflicting-subtest that writes a yaml containing both the new parent-style keys
(e.g., macos_fleet/ios_fleet/ipados_fleet or apple_business) and the deprecated
child keys in the same resource and assert GitOpsFromFile returns an error
containing "cannot specify both" and the expected key path
(org_settings.mdm.apple_business) so DeprecatedGitOpsKeyMappings migration logic
is covered. Reference the existing test functions "old_key_still_accepted" and
"both_keys_conflict" and the called function "GitOpsFromFile" to locate where to
add these cases.
In `@server/platform/endpointer/json_key_duplicator_test.go`:
- Around line 192-229: The test case "MultiLevelRenamedContainers" only asserts
the macos_* path; add symmetric assertions for the ios_* branch inside the
validate func: after checking macos_team/mac os_fleet, assert ios_team still has
team_id and not fleet_id, assert ios_fleet has fleet_id and not team_id, and in
the new ab_tokens entry verify it contains ios_fleet (with fleet_id) and does
not contain ios_team; update variable references (e.g., iosTeam, iosFleet, ab,
newTok) accordingly to mirror the macos checks so the ios_team → ios_fleet
rename is covered.
🪄 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: 535b3b57-0037-492e-9a82-306f2e3ad963
📒 Files selected for processing (50)
changes/42508-rename-abm-to-abcmd/fleetctl/fleetctl/generate.gocmd/fleetctl/fleetctl/generate_gitops.gocmd/fleetctl/fleetctl/generate_gitops_test.gocmd/fleetctl/fleetctl/get.gocmd/fleetctl/fleetctl/get_test.gocmd/fleetctl/fleetctl/gitops.gocmd/fleetctl/fleetctl/gitops_test.gocmd/fleetctl/fleetctl/templates/new/default.template.ymlcmd/fleetctl/fleetctl/testdata/expectedGetConfigAppConfigJson.jsoncmd/fleetctl/fleetctl/testdata/expectedGetConfigAppConfigTeamMaintainerJson.jsoncmd/fleetctl/fleetctl/testdata/expectedGetConfigAppConfigTeamMaintainerYaml.ymlcmd/fleetctl/fleetctl/testdata/expectedGetConfigAppConfigYaml.ymlcmd/fleetctl/fleetctl/testdata/expectedGetConfigIncludeServerConfigJson.jsoncmd/fleetctl/fleetctl/testdata/expectedGetConfigIncludeServerConfigYaml.ymlcmd/fleetctl/fleetctl/testdata/generateGitops/expectedOrgSettings-insecure.yamlcmd/fleetctl/fleetctl/testdata/generateGitops/expectedOrgSettings.yamlcmd/fleetctl/fleetctl/testdata/generateGitops/test_dir_premium/default.ymlcmd/fleetctl/fleetctl/testdata/macosSetupExpectedAppConfigEmpty.ymlcmd/fleetctl/fleetctl/testdata/macosSetupExpectedAppConfigSet.ymlee/server/service/mdm.gofrontend/components/App/App.tsxfrontend/interfaces/mdm.tsfrontend/interfaces/team.tsfrontend/pages/admin/IntegrationsPage/cards/MdmSettings/AppleBusinessManagerPage/AppleBusinessManagerPage.tsxfrontend/pages/admin/IntegrationsPage/cards/MdmSettings/AppleBusinessManagerPage/components/AppleBusinessManagerTable/AppleBusinessManagerTable.tsxfrontend/pages/admin/IntegrationsPage/cards/MdmSettings/AppleBusinessManagerPage/components/AppleBusinessManagerTable/AppleBusinessManagerTableConfig.tsxfrontend/pages/admin/IntegrationsPage/cards/MdmSettings/AppleBusinessManagerPage/components/EditTeamsAbmModal/EditTeamsAbmModal.tsxfrontend/pages/admin/IntegrationsPage/cards/MdmSettings/VppPage/components/VppTable/VppTableConfig.tsxfrontend/services/entities/mdm_apple_bm.tsfrontend/utilities/endpoints.tspkg/spec/gitops_deprecations.gopkg/spec/gitops_test.gopkg/spec/gitops_validate.gopkg/spec/gitops_validate_test.goserver/fleet/app.goserver/fleet/apple_mdm.goserver/fleet/errors.goserver/platform/endpointer/endpoint_utils.goserver/platform/endpointer/extract_alias_rules_test.goserver/platform/endpointer/json_key_duplicator.goserver/platform/endpointer/json_key_duplicator_test.goserver/platform/endpointer/json_key_rewriter.goserver/service/appconfig.goserver/service/apple_mdm.goserver/service/client.goserver/service/client_mdm.goserver/service/handler.goserver/service/handler_deprecated_paths.goserver/service/integration_mdm_test.go
There was a problem hiding this comment.
This PR is too large and touches too many sensitive surfaces (MDM API endpoints, the JSON key rewriter/duplicator infrastructure, and an design decision on the renameto mechanism) for me to approve without human review — the bug hunting pass found no new defects in the PR itself.
Extended reasoning...
Overview
This PR renames abm/apple_business_manager to ab/apple_business across the entire stack: 50 files touching the public API (/ab_tokens, /mdm/apple/ab_public_key), fleetctl CLI commands (mdm-ab/mdm_ab), GitOps YAML schema (mdm.apple_business), frontend types/services/UI, and the shared server/platform/endpointer infrastructure (json_key_duplicator.go, json_key_rewriter.go, endpoint_utils.go). Deprecated path aliases preserve backwards compatibility for every renamed endpoint.
Security risks
No direct injection/auth-bypass risks. The endpoints being renamed are MDM-related and require Premium licensing + admin auth (already in place). The main risk class is behavioral regression: if the new Inline rule on renameto interacts unexpectedly with existing response shapes, downstream API consumers (other Fleet UIs, customer scripts) could see malformed payloads. The added tests for MultiLevelRenamedContainers and InlineContainerWithLeafChildren cover the intended shapes, but the broader contract is wide.
Level of scrutiny
High. This is not a sandbox or config tweak — it changes the shape of authenticated production API responses, introduces a new ,inline option on the renameto struct tag that affects how every renamed container in the codebase serializes, and modifies the duplicator's recursion semantics. The PR description itself flags this as "a slight twist" on existing logic, which is exactly the kind of design decision that warrants a human signoff from a CODEOWNER familiar with the endpointer/MDM systems.
Other factors
- Bug-hunting pass surfaced only one finding, which is pre-existing (the
renametoround-trip conflict on null/scalar siblings) and explicitly out of scope. - Codecov reports 64% patch coverage with 52 lines uncovered, several in the new duplicator branches.
- The new test
TestAppleBusinessKeyRename/both_keys_conflictconfirms the conflict behavior is intentional for this rename. - No prior review from me on this PR.
# Conflicts: # cmd/fleetctl/fleetctl/gitops.go
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.
MagnusHJensen
left a comment
There was a problem hiding this comment.
Should this also update the api_endpoints.yml, both for the visual name, but also if the routes change, without backwards compatible routes.
| ue.GET("/api/_version_/fleet/mdm/apple/ab_public_key", generateABMKeyPairEndpoint, nil) | ||
| ue.POST("/api/_version_/fleet/ab_tokens", uploadABMTokenEndpoint, uploadABMTokenRequest{}) | ||
| ue.DELETE("/api/_version_/fleet/ab_tokens/{id:[0-9]+}", deleteABMTokenEndpoint, deleteABMTokenRequest{}) | ||
| ue.GET("/api/_version_/fleet/ab_tokens", listABMTokensEndpoint, nil) | ||
| ue.GET("/api/_version_/fleet/ab_tokens/count", countABMTokensEndpoint, nil) | ||
| ue.PATCH("/api/_version_/fleet/ab_tokens/{id:[0-9]+}/fleets", updateABMTokenTeamsEndpoint, updateABMTokenTeamsRequest{}) | ||
| ue.PATCH("/api/_version_/fleet/ab_tokens/{id:[0-9]+}/renew", renewABMTokenEndpoint, renewABMTokenRequest{}) |
There was a problem hiding this comment.
Just wanted to understand, is this not a breaking change renaming the URL's without keeping the old names around?
There was a problem hiding this comment.
That's what this file does. Remaps deprecated paths to their non-deprecated counterparts and issues deprecation warnings when applicable
https://github.com/fleetdm/fleet/pull/46657/changes#diff-bd1190c0c0405070190c6b0d5195aa3e3cac26ecf1859faecbf056aec96445b8
There was a problem hiding this comment.
Great, I missed that. I still think we should at least update the visual name under the api_endpoints.yaml.
There was a problem hiding this comment.
Good call. Updated and added endpoints to api_endpoints.yml(which I totally forgot aobut as it didn't exist when I first looked at this ticket)
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
Related issue: Resolves #42508
Renames abm/apple_business_manager to ab/apple_business in API and fleetctl. Uses existing renameto logic with a slight twist: added "inline" option to handle cases particularly where a single object tree has renames in multiple versions so that we don't break backwards compatibiility since the default behavior when you have multi-level renames is a new/old split at the top level
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
Added/updated automated tests
Where appropriate, automated tests simulate multiple hosts and test for host isolation (updates to one hosts's records do not affect another)
QA'd all new/changed functionality manually
Summary by CodeRabbit
New Features
Deprecations