Fix fleetctl generate-gitops failing to include VPP fleet assignments#42429
Fix fleetctl generate-gitops failing to include VPP fleet assignments#42429
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.
Tip: disable this comment in your organization's Code Review settings.
There was a problem hiding this comment.
Pull request overview
Updates fleetctl generate-gitops to correctly include VPP (Volume Purchasing Program) token-to-fleet/team assignments by fetching and translating VPP token state from the server API.
Changes:
- Added a Fleet client method to fetch VPP tokens (
GET /api/latest/fleet/vpp_tokens). - Updated
generateMDMto derivevolume_purchasing_programGitOps output from VPP tokens instead of app config. - Expanded
generate-gitopsunit tests to cover VPP token-to-config translation and updated an existing MDM-disabled test to avoid VPP calls.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| server/service/client_mdm.go | Adds Client.GetVPPTokens() for retrieving VPP token state from the Fleet API. |
| cmd/fleetctl/fleetctl/generate_gitops.go | Uses GetVPPTokens() to generate mdm.volume_purchasing_program from token assignments. |
| cmd/fleetctl/fleetctl/generate_gitops_test.go | Adds VPP translation tests and adjusts existing tests to account for the new VPP fetch. |
| changes/33106-fix-generate-gitops-vpp | Changelog entry for the fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #42429 +/- ##
==========================================
- Coverage 66.52% 66.51% -0.01%
==========================================
Files 2530 2530
Lines 203059 203083 +24
Branches 9141 9141
==========================================
+ Hits 135077 135086 +9
- Misses 55775 55786 +11
- Partials 12207 12211 +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:
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThis change adds VPP token fetching functionality to the Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cmd/fleetctl/fleetctl/generate_gitops_test.go (1)
1979-2090: Add one failure-path case forGetVPPTokenserrors.Current table cases validate shape/mapping well, but don’t assert
generateMDMbehavior when the client returns an error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/fleetctl/fleetctl/generate_gitops_test.go` around lines 1979 - 2090, Add a failure-path test to TestGenerateMDMVPPTokens that asserts generateMDM returns the error when GetVPPTokens fails: extend the table with a case like "GetVPPTokens error", modify vppMockClientWrapper to include an err field (e.g. vppErr error) and update its GetVPPTokens to return (w.vppTokens, w.vppErr), and in the test set wrapper.vppErr = errors.New("some error") then call cmd.generateMDM(&appConfig.MDM) and require.Error(t, err) and assert the returned mdmRaw is nil or empty as appropriate; reference TestGenerateMDMVPPTokens, generateMDM, and vppMockClientWrapper.GetVPPTokens to locate where to add the case and change the wrapper.cmd/fleetctl/fleetctl/generate_gitops.go (1)
1121-1130: Make VPP output ordering deterministic for stable GitOps diffs.Line 1122+ preserves incoming token/team order. If backend order changes, generated YAML can churn without real config changes.
♻️ Suggested deterministic ordering
+ "sort" + "strings" @@ - var vppConfig []fleet.MDMAppleVolumePurchasingProgramInfo + vppConfig := make([]fleet.MDMAppleVolumePurchasingProgramInfo, 0, len(vppTokens)) for _, token := range vppTokens { + if token == nil { + continue + } teamNames := make([]string, 0, len(token.Teams)) for _, team := range token.Teams { teamNames = append(teamNames, team.Name) } + sort.Strings(teamNames) vppConfig = append(vppConfig, fleet.MDMAppleVolumePurchasingProgramInfo{ Location: token.Location, Teams: teamNames, }) } + sort.Slice(vppConfig, func(i, j int) bool { + return strings.ToLower(vppConfig[i].Location) < strings.ToLower(vppConfig[j].Location) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/fleetctl/fleetctl/generate_gitops.go` around lines 1121 - 1130, The current loop building vppConfig from vppTokens preserves upstream ordering which causes non-deterministic YAML diffs; to fix, iterate over a deterministically sorted copy of vppTokens (e.g., sort by token.Location or another stable key) and for each token build teamNames and sort them (use sort.Strings on the slice of Team.Name) before creating fleet.MDMAppleVolumePurchasingProgramInfo, and (optionally) sort the final vppConfig slice by Location as a final pass so the resulting output is stable regardless of backend ordering.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/fleetctl/fleetctl/generate_gitops_test.go`:
- Around line 1979-2090: Add a failure-path test to TestGenerateMDMVPPTokens
that asserts generateMDM returns the error when GetVPPTokens fails: extend the
table with a case like "GetVPPTokens error", modify vppMockClientWrapper to
include an err field (e.g. vppErr error) and update its GetVPPTokens to return
(w.vppTokens, w.vppErr), and in the test set wrapper.vppErr = errors.New("some
error") then call cmd.generateMDM(&appConfig.MDM) and require.Error(t, err) and
assert the returned mdmRaw is nil or empty as appropriate; reference
TestGenerateMDMVPPTokens, generateMDM, and vppMockClientWrapper.GetVPPTokens to
locate where to add the case and change the wrapper.
In `@cmd/fleetctl/fleetctl/generate_gitops.go`:
- Around line 1121-1130: The current loop building vppConfig from vppTokens
preserves upstream ordering which causes non-deterministic YAML diffs; to fix,
iterate over a deterministically sorted copy of vppTokens (e.g., sort by
token.Location or another stable key) and for each token build teamNames and
sort them (use sort.Strings on the slice of Team.Name) before creating
fleet.MDMAppleVolumePurchasingProgramInfo, and (optionally) sort the final
vppConfig slice by Location as a final pass so the resulting output is stable
regardless of backend ordering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4bec6317-8723-431c-87f3-4ba834eb7536
📒 Files selected for processing (4)
changes/33106-fix-generate-gitops-vppcmd/fleetctl/fleetctl/generate_gitops.gocmd/fleetctl/fleetctl/generate_gitops_test.goserver/service/client_mdm.go
|
Re: CodeRabbit:
|
|
@sgress454 I noticed this was the oldest bug not in a sprint so figured I'd take crack at it based on your comment. |
For #33106
Reproduced bug and validated fix locally. After fix:
Passed
fleetctl gitopsvalidation.Summary by CodeRabbit
Bug Fixes
fleetctl generate-gitopscommand that previously failed to include VPP (Volume Purchasing Program) fleet assignments. The tool now properly retrieves VPP tokens and includes them with their associated team assignments in the generated configuration files.Tests