Fix fleetctl get fleets to use source of truth (DB) for software - #46480
Conversation
…e DB tables for get teams/fleets
![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 fixes fleetctl get fleets / fleetctl get teams YAML/JSON export so the software section (notably per-item setup_experience) is derived from the authoritative software + setup experience endpoints, instead of the potentially stale team config. This addresses issue #44970 where exported YAML could contain setup_experience: null for apps that are actually configured for setup experience.
Changes:
- Update
fleetctl get fleets/teamsto fetch per-team software (packages, Fleet-maintained apps, VPP apps) and setup experience membership from source-of-truth endpoints when running against a Premium server. - Add a regression test ensuring
setup_experience: trueis emitted for an app that is in setup experience. - Update existing tests to stub the newly-used software endpoints where those tests don’t care about software output.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/fleetctl/fleetctl/get.go | Build team software spec from software titles + setup experience endpoints (Premium-only) when printing YAML/JSON. |
| cmd/fleetctl/fleetctl/get_test.go | Add software regression test and add mocks for software endpoints in unrelated tests. |
| cmd/fleetctl/fleetctl/apply_test.go | Stub software endpoints in tests impacted by the new get fleets behavior. |
| cmd/fleetctl/fleetctl/apply_deprecated_test.go | Stub software endpoints in tests impacted by the new get fleets behavior. |
| changes/44970-get-fleets-setup-experience | Add release note for the user-visible behavior change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
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 (3)
✅ Files skipped from review due to trivial changes (1)
WalkthroughThis PR fixes a bug where 🚥 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 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #46480 +/- ##
==========================================
+ Coverage 66.81% 66.85% +0.04%
==========================================
Files 2804 2804
Lines 223574 223642 +68
Branches 11481 11400 -81
==========================================
+ Hits 149379 149520 +141
+ Misses 60639 60534 -105
- Partials 13556 13588 +32
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:
|
sgress454
left a comment
sgress454
left a comment
There was a problem hiding this comment.
Why do we need to check the license when teams are a premium-only feature anyway?
You are right. No need. (It fails earlier in the list teams request.) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
cmd/fleetctl/fleetctl/get.go (2)
323-330:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winPotential key mismatch for App Store apps may cause
setup_experienceto be incorrectly omitted.On line 328, the setup experience map is keyed by
app.FullyQualifiedName()from the setup experience endpoint response. However, on line 397, the lookup usesapp.VPPAppID.String()from the software title detail response. If these methods return different string formats, App Store apps will fail to match andInstallDuringSetupwill remain unset—which is exactly the bug this PR intends to fix.Run the following to verify the key formats match:
#!/bin/bash # Find the definition of FullyQualifiedName and VPPAppID.String() to compare their output formats echo "=== FullyQualifiedName implementation ===" ast-grep --pattern $'func ($_ $_) FullyQualifiedName() string { $$$ }' echo "" echo "=== VPPAppID type and String method ===" rg -nP --type=go -A10 'type VPPAppID' echo "" echo "=== VPPAppID String method ===" ast-grep --pattern $'func ($_ VPPAppID) String() string { $$$ }'Also applies to: 394-411
🤖 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.go` around lines 323 - 330, The setupAppsByName map is populated using app.FullyQualifiedName() in the setup loop but later looked up with app.VPPAppID.String(), causing mismatches; update the map population in the loop that iterates setupSoftware so AppStore apps are keyed using the same identifier used at lookup (use app.VPPAppID.String() when app.VPPAppID is present, otherwise fall back to FullyQualifiedName()), so the lookup at the later check (where app.VPPAppID.String() is used) will correctly find entries; adjust the logic around setupAppsByName and references to FullyQualifiedName/VPPAppID.String() accordingly.
306-313:⚠️ Potential issue | 🟠 Major | ⚖️ Poor tradeoffGracefully handle premium-gated software endpoints in
getTeamSoftwareSpec
getTeamSoftwareSpecis called for every team and currently fails the wholefleetctl get fleetsrun if either software endpoint errors. On non-premium deployments, the server explicitly returnsfleet.ErrMissingLicense(402) from:
server/service/software_titles.go(ListSoftwareTitleswhenfleet_id/team_idis provided)server/service/setup_experience.go(ListSetupExperienceSoftware)Catch
ErrMissingLicensearoundListSoftwareTitles/GetSetupExperienceSoftwareand treat it as “no software section” (nil/empty) instead of returning an error.🤖 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.go` around lines 306 - 313, getTeamSoftwareSpec currently returns an error when ListSoftwareTitles fails, which breaks `fleetctl get fleets` on non-premium deployments; update getTeamSoftwareSpec to catch `fleet.ErrMissingLicense` from `client.ListSoftwareTitles` (and likewise for calls to `client.GetSetupExperienceSoftware` where used) and treat that case as “no software” by returning nil (or an empty spec) instead of an error; specifically, wrap the error check after calling `client.ListSoftwareTitles` to if errors.Is(err, fleet.ErrMissingLicense) { return nil, nil } and apply the same pattern around `client.GetSetupExperienceSoftware` calls so premium-gated endpoints degrade to an empty/no software section.
🤖 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 `@cmd/fleetctl/fleetctl/get.go`:
- Around line 323-330: The setupAppsByName map is populated using
app.FullyQualifiedName() in the setup loop but later looked up with
app.VPPAppID.String(), causing mismatches; update the map population in the loop
that iterates setupSoftware so AppStore apps are keyed using the same identifier
used at lookup (use app.VPPAppID.String() when app.VPPAppID is present,
otherwise fall back to FullyQualifiedName()), so the lookup at the later check
(where app.VPPAppID.String() is used) will correctly find entries; adjust the
logic around setupAppsByName and references to
FullyQualifiedName/VPPAppID.String() accordingly.
- Around line 306-313: getTeamSoftwareSpec currently returns an error when
ListSoftwareTitles fails, which breaks `fleetctl get fleets` on non-premium
deployments; update getTeamSoftwareSpec to catch `fleet.ErrMissingLicense` from
`client.ListSoftwareTitles` (and likewise for calls to
`client.GetSetupExperienceSoftware` where used) and treat that case as “no
software” by returning nil (or an empty spec) instead of an error; specifically,
wrap the error check after calling `client.ListSoftwareTitles` to if
errors.Is(err, fleet.ErrMissingLicense) { return nil, nil } and apply the same
pattern around `client.GetSetupExperienceSoftware` calls so premium-gated
endpoints degrade to an empty/no software section.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b830bac0-a558-4c3a-9e05-103ba78dfabe
📒 Files selected for processing (1)
cmd/fleetctl/fleetctl/get.go
) Resolves #44970 (2/2). The issue is that in `fleetctl apply` the `setup_experience` field in software items was being converted to `macos_setup` (which we don't want because that rename should only happen in MDM). I tried to make the change as simple as possible and isolated to `software` (I checked and it seems there are no renames under `software` spec). Supporting some context aware renaming (or prevention of renaming) requires a bigger refactor on the rewriter functionality. - [X] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. ## Testing - [X] Added/updated automated tests - [X] QA'd all new/changed functionality manually
sgress454
left a comment
sgress454
left a comment
There was a problem hiding this comment.
I wonder if there's some way we could be leveraging the generate_gitops code for this (assuming it's doing it right). Anything to do with fleetctl get feels close to throwaway code anyway, and we don't want to have to keep maintaining both (for example if we added a new key to the software package spec).
Resolves #44970 (1/2).
changes/,orbit/changes/oree/fleetd-chrome/changes.Summary by CodeRabbit
fleetctl get fleets/get teamsnow display software and setup experience from authoritative software endpoints.