Move targets and secret variables to server/fleet/ - #46196
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.
lucasmrod
changed the title
There was a problem hiding this comment.
Pull request overview
This PR moves the Go API request/response types for “targets” and “secret variables” endpoints out of server/service/ and into server/fleet/, so Go clients (e.g. fleetctl, Orbit, etc.) can depend on server/fleet types without importing server/service code.
Changes:
- Moved targets request/response structs (and related search result structs + JSON marshal logic) to
server/fleet/api_targets.go. - Moved secret variables request/response structs to
server/fleet/api_secret_variables.go. - Updated service endpoints, router registrations, clients, and integration tests to use the new
fleet.*types.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| server/service/targets.go | Switch endpoints to fleet.SearchTargetsRequest/Response and fleet.CountTargetsRequest/Response; remove now-internal service-local types. |
| server/service/secret_variables.go | Switch endpoints to fleet.*SecretVariables* request/response types; remove service-local types. |
| server/service/handler.go | Update route registrations to use fleet.*Request{} types for decoding. |
| server/service/client_targets.go | Update client request/response types for /fleet/targets. |
| server/service/client_secret_variables.go | Update client request/response types for secret variables spec endpoint. |
| server/service/integration_core_test.go | Update tests to use moved fleet.* request/response types. |
| server/service/integration_enterprise_test.go | Update tests to use moved fleet.* request/response types. |
| server/service/integration_mdm_test.go | Update tests to use moved fleet.* request/response types. |
| server/service/integration_mdm_profiles_test.go | Update tests to use moved fleet.* request/response types. |
| server/service/integration_mdm_ddm_test.go | Update tests to use moved fleet.* request/response types. |
| server/fleet/api_targets.go | New home for targets API request/response structs and team search result JSON marshaling logic. |
| server/fleet/api_secret_variables.go | New home for secret variables API request/response structs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
WalkthroughThis PR adds fleet DTOs for targets (search/count) and secret-variables (create/create single/list/delete), implements TeamSearchResult JSON (marshal/unmarshal), and updates server route bindings, endpoint implementations, client calls, and tests to use the new fleet types across search/count and secret-variable flows. Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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.
Actionable comments posted: 1
🤖 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 `@server/fleet/api_targets.go`:
- Around line 36-63: The MarshalJSON implementation for TeamSearchResult
dereferences the embedded Team/TeamConfig unconditionally and can panic for a
zero-value result; update TeamSearchResult.MarshalJSON to guard against nil
before using t.Config/t.Team (check if t.Config!=nil or t.Team!=nil) and assign
a zero-value TeamConfig when nil (instead of dereferencing), ensuring
HostResponsesForHostsCheap(t.Hosts) and other fields are populated only from
safe, non-nil sources.
🪄 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: 0c20c3f6-1081-4cb6-8ebe-a24b44f3a535
📒 Files selected for processing (12)
server/fleet/api_secret_variables.goserver/fleet/api_targets.goserver/service/client_secret_variables.goserver/service/client_targets.goserver/service/handler.goserver/service/integration_core_test.goserver/service/integration_enterprise_test.goserver/service/integration_mdm_ddm_test.goserver/service/integration_mdm_profiles_test.goserver/service/integration_mdm_test.goserver/service/secret_variables.goserver/service/targets.go
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #46196 +/- ##
==========================================
- Coverage 66.91% 66.84% -0.07%
==========================================
Files 2834 2781 -53
Lines 224966 223877 -1089
Branches 11517 10916 -601
==========================================
- Hits 150527 149658 -869
+ Misses 60782 60559 -223
- Partials 13657 13660 +3
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:
|
| MatchQuery string `json:"query"` | ||
| // QueryID is the ID of a saved query to run (used to determine if this is a | ||
| // query that observers can run). | ||
| QueryID *uint `json:"query_id" renameto:"report_id"` |
There was a problem hiding this comment.
Since you are touching all users of SearchTargetsRequest might as well just rename this to ReportID and drop the rename decorator.
There was a problem hiding this comment.
What do you mean? AFAICS we need to support both (query_id and report_id)
| type TargetsData struct { | ||
| Hosts []*HostResponse `json:"hosts"` | ||
| Labels []LabelSearchResult `json:"labels"` | ||
| Teams []TeamSearchResult `json:"teams" renameto:"fleets"` |
|
|
||
| type CountTargetsRequest struct { | ||
| Selected HostTargets `json:"selected"` | ||
| QueryID *uint `json:"query_id" renameto:"report_id"` |
juan-fdz-hawa
left a comment
juan-fdz-hawa
left a comment
There was a problem hiding this comment.
LGTM!
I left one optional suggestion about dropping the rename decorator on a few of the requests. It’s definitely not a dealbreaker, so I'll leave it up to you whether you want to tackle it now or skip it.
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
Resolves #36087 (one of several PRs).
Testing
Summary by CodeRabbit
New Features
Improvements
Tests