API endpoints for Linux setup experience - #32493
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #32493 +/- ##
==========================================
+ Coverage 64.02% 64.03% +0.01%
==========================================
Files 1987 1986 -1
Lines 195630 195830 +200
Branches 6549 6542 -7
==========================================
+ Hits 125251 125400 +149
- Misses 60576 60598 +22
- Partials 9803 9832 +29
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 review |
✅ Actions performedReview triggered.
|
WalkthroughAdds Linux support to setup_experience: platform-scoped software configuration (macOS/Linux), orbit init endpoint, device status endpoint, platform-aware datastore changes, activity logging, and control-flow updates to operate on Host objects. Includes osquery policy gating during setup, platform_like persistence, and extensive integration and unit tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Admin
participant API as Fleet API
participant Svc as Service
participant DS as Datastore
participant Act as Activities
Admin->>API: PUT /fleet/setup_experience/{platform}/software (title IDs, team)
API->>Svc: SetSetupExperienceSoftware(platform, teamID, titles)
Svc->>DS: SetSetupExperienceSoftwareTitles(platform, teamID, titles)
DS-->>Svc: ok
Svc->>Act: Record EditedSetupExperienceSoftware (platform, team)
Act-->>Svc: ok
Svc-->>API: 200
API-->>Admin: Success
sequenceDiagram
autonumber
participant Orbit as Orbit (host)
participant API as Fleet API
participant Svc as Service (EE)
participant DS as Datastore
Note over Orbit,API: Initialization (non-darwin also)
Orbit->>API: POST /fleet/orbit/setup_experience/init
API->>Svc: SetupExperienceInit(ctx)
Svc->>DS: EnqueueSetupExperienceItems(host.platform_like, hostUUID, teamID)
DS-->>Svc: enabled?
Svc-->>API: { enabled }
API-->>Orbit: { enabled }
Note over Orbit,API: Status polling
Orbit->>API: POST /fleet/device/{token}/setup_experience/status
API->>Svc: GetDeviceSetupExperienceStatus(ctx)
Svc->>DS: ListSetupExperienceResultsByHostUUID(hostUUID)
DS-->>Svc: results (software-only)
Svc->>Svc: SetupExperienceNextStep(host)
Svc-->>API: { setup_experience_results }
Note over Orbit,Svc: Result reporting (scripts/software)
Orbit->>API: Report script/software result
API->>Svc: SaveHost...Result(host, result)
Svc->>Svc: SetupExperienceNextStep(host)
Svc-->>API: ok
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Suggested labels
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
ee/server/service/orbit.go (1)
178-223: Guard against nil pointers and VPP cases when marking cancellations.r.IsForSoftware() can include entries without SoftwareInstallerID or HostSoftwareInstallsExecutionID (e.g., VPP). The current code dereferences without checks and can panic.
- if r.IsForSoftware() { + if r.IsForSoftware() && r.SoftwareInstallerID != nil && r.HostSoftwareInstallsExecutionID != nil { softwarePackage := "" installerMeta, err := svc.ds.GetSoftwareInstallerMetadataByID(ctx, *r.SoftwareInstallerID) if err != nil && !fleet.IsNotFound(err) { return ctxerr.Wrap(ctx, err, "getting software installer metadata for cancelled setup experience software install") } if installerMeta != nil { softwarePackage = installerMeta.Name } activity := fleet.ActivityTypeInstalledSoftware{ HostID: hostID, HostDisplayName: hostDisplayName, SoftwareTitle: r.Name, SoftwarePackage: softwarePackage, InstallUUID: *r.HostSoftwareInstallsExecutionID, Status: "failed", SelfService: false, FromSetupExperience: true, } err = svc.NewActivity(ctx, nil, activity) if err != nil { return ctxerr.Wrap(ctx, err, "creating activity for cancelled setup experience software install") } + } else if r.IsForSoftware() { + // Likely a VPP entry or missing IDs. Avoid panic; optionally add a TODO to emit a VPP-specific activity. + level.Warn(svc.logger).Log("msg", "skipping activity for cancelled setup experience item with missing installer IDs", "host_uuid", hostUUID, "name", r.Name) }server/service/orbit.go (1)
865-874: Avoid nil panic and use consistent host UUID for setup-experience script results
- Use HostUUIDForSetupExperience for Linux parity (enqueue/update must match).
- Guard the enterprise override callback to avoid calling a nil function in OSS builds.
Apply:
- if updated, err := maybeUpdateSetupExperienceStatus(ctx, svc.ds, fleet.SetupExperienceScriptResult{ - HostUUID: host.UUID, + hostUUID := fleet.HostUUIDForSetupExperience(host) + if updated, err := maybeUpdateSetupExperienceStatus(ctx, svc.ds, fleet.SetupExperienceScriptResult{ + HostUUID: hostUUID, ExecutionID: result.ExecutionID, ExitCode: result.ExitCode, }, true); err != nil { return ctxerr.Wrap(ctx, err, "update setup experience status") } else if updated { - level.Debug(svc.logger).Log("msg", "setup experience script result updated", "host_uuid", host.UUID, "execution_id", result.ExecutionID) + level.Debug(svc.logger).Log("msg", "setup experience script result updated", "host_uuid", hostUUID, "execution_id", result.ExecutionID) fromSetupExperience = true - _, err := svc.EnterpriseOverrides.SetupExperienceNextStep(ctx, host) - if err != nil { - return ctxerr.Wrap(ctx, err, "getting next step for host setup experience") - } + if next := svc.EnterpriseOverrides.SetupExperienceNextStep; next != nil { + if _, err := next(ctx, host); err != nil { + return ctxerr.Wrap(ctx, err, "getting next step for host setup experience") + } + } }Also applies to: 874-878
server/fleet/service.go (1)
1227-1247: Implement missing Setup Experience software APIs and mocks
- Define
SetSetupExperienceSoftwareandListSetupExperienceSoftwareon the OSSService(server/fleet/service.go)- Add corresponding methods to your mocks (e.g. in server/mock)
- Wire these methods into EE via
EnterpriseOverridesalongsideSetupExperienceInitandGetDeviceSetupExperienceStatusee/server/service/setup_experience.go (2)
226-244: Don't set NanoCommandUUID when enqueueing VPP app failsNanoCommandUUID is assigned before checking err. If installSoftwareFromVPP fails, the record may incorrectly show a command UUID. Set the UUID only on success.
Apply:
- cmdUUID, err := svc.installSoftwareFromVPP(ctx, host, vppApp, true, fleet.HostSoftwareInstallOptions{ + cmdUUID, err := svc.installSoftwareFromVPP(ctx, host, vppApp, true, fleet.HostSoftwareInstallOptions{ SelfService: false, ForSetupExperience: true, }) - - app.NanoCommandUUID = &cmdUUID - app.Status = fleet.SetupExperienceStatusRunning - - if err != nil { + if err == nil { + app.NanoCommandUUID = &cmdUUID + app.Status = fleet.SetupExperienceStatusRunning + } else { // if we get an error (e.g. no available licenses) while attempting to enqueue the // install, then we should immediately go to an error state so setup experience // isn't blocked. level.Warn(svc.logger).Log("msg", "got an error when attempting to enqueue VPP app install", "err", err, "adam_id", app.VPPAppAdamID) app.Status = fleet.SetupExperienceStatusFailure app.Error = ptr.String(err.Error()) - } + }
150-156: Guard against nil hostSetupExperienceNextStep dereferences host.ID later. Add a nil check to avoid a panic if callers pass nil.
func (svc *Service) SetupExperienceNextStep(ctx context.Context, host *fleet.Host) (bool, error) { + if host == nil { + return false, fleet.NewInvalidArgumentError("host", "host is required") + } hostUUID := fleet.HostUUIDForSetupExperience(host)server/datastore/mysql/setup_experience.go (1)
21-60: Linux distro checks: normalize host platform_like to avoid case/variant mismatchesThe query compares ? against 'debian'/'rhel'. Normalize hostPlatformLike to lowercase before binding to avoid mismatches (e.g., 'Debian' or 'rhel8').
- fleetPlatform := fleet.PlatformFromHost(hostPlatformLike) - res, err := tx.ExecContext(ctx, stmtSoftwareInstallers, hostUUID, teamID, fleetPlatform, hostPlatformLike, hostPlatformLike) + fleetPlatform := fleet.PlatformFromHost(hostPlatformLike) + distro := strings.ToLower(hostPlatformLike) + res, err := tx.ExecContext(ctx, stmtSoftwareInstallers, hostUUID, teamID, fleetPlatform, distro, distro)Also applies to: 103-106
♻️ Duplicate comments (1)
server/service/handler.go (1)
386-392: Validate {platform} and document deprecation window.Ensure the request decoder rejects unknown platforms (allow only "macos" and "linux"). Keep the legacy path but mark as deprecated in docs/UI.
🧹 Nitpick comments (32)
server/service/integration_core_test.go (1)
10310-10314: Optional: normalize common aliases for platform to reduce test footgunsIf a test accidentally passes “macos” or “win”, Host.Platform will be set to an unexpected value. Consider normalizing known aliases before assignment (keeps existing callers working and avoids subtle flakiness).
func createOrbitEnrolledHost(t *testing.T, platform, suffix string, ds fleet.Datastore) *fleet.Host { name := t.Name() + suffix + // Normalize common aliases used in tests. + switch strings.ToLower(platform) { + case "macos": + platform = "darwin" + case "win": + platform = "windows" + } h, err := ds.NewHost(context.Background(), &fleet.Host{ @@ - Platform: platform, + Platform: platform, })docs/Contributing/reference/audit-logs.md (1)
2005-2013: Resolve markdownlint warnings (MD001, MD010) in new section.
- MD001: Change “#### Example” to “### Example”.
- MD010: Replace hard tabs with spaces inside the JSON example.
Apply this diff:
-#### Example +### Example ```json { - "platform": "darwin", - "team_id": 1, - "team_name": "Workstations" + "platform": "darwin", + "team_id": 1, + "team_name": "Workstations" }</blockquote></details> <details> <summary>server/fleet/orbit.go (1)</summary><blockquote> `204-208`: **Tighten doc comment and remove stray blank line.** Keep the doc comment contiguous and concise to satisfy go doc style; no blank line between the comment and type. Apply this diff: ```diff -// SetupExperienceInitResult is the payload returned when the orbit client manually initiates -// setup experience for non-darwin platforms. - -type SetupExperienceInitResult struct { +// SetupExperienceInitResult is the payload returned when the Orbit client manually +// initiates the setup experience for non-darwin platforms. +type SetupExperienceInitResult struct { Enabled bool `json:"enabled"` }server/datastore/mysql/software_installers_test.go (1)
1284-1284: Prefer platform constant over string literal.Use fleet.MacOSPlatform (or the host’s actual platform) instead of "darwin" to avoid drift.
Apply this diff:
- _, err = ds.EnqueueSetupExperienceItems(ctx, "darwin", host1.UUID, *host1.TeamID) + _, err = ds.EnqueueSetupExperienceItems(ctx, fleet.MacOSPlatform, host1.UUID, *host1.TeamID)server/service/apple_mdm.go (1)
3497-3504: Platform-aware enqueue looks right—please verify accepted values and consider gating non-macOS.
- Nice update passing
info.Platform. Confirm datastore expectsdarwin/ios/ipados(notmacos). If not, normalize before calling.- Optional: skip the DB call for iOS/iPadOS (likely no setup-experience items today) to avoid a no-op round-trip.
- hasSetupExpItems, err = svc.ds.EnqueueSetupExperienceItems(r.Context, info.Platform, r.ID, info.TeamID) + // Optional: only enqueue for macOS; avoid no-op for iOS/iPadOS. + if info.Platform == "darwin" { + hasSetupExpItems, err = svc.ds.EnqueueSetupExperienceItems(r.Context, info.Platform, r.ID, info.TeamID) + }server/service/integration_mdm_test.go (3)
15904-15907: Use require.NoError for error assertionsPrefer require.NoError(t, err) over require.Nil(t, err) to properly assert error values.
Apply this diff:
-require.Nil(t, respPutSetupExperience.Error()) +require.NoError(t, respPutSetupExperience.Error())Consider making the same change for the earlier assertion on Line 15899.
15901-15903: Escape team_name in JSON payload to avoid malformed JSONIf team1.Name contains quotes/backslashes, the fmt.Sprintf with "%s" can generate invalid JSON. Use %q or pre-marshal the payload.
Apply this diff:
- s.lastActivityOfTypeMatches(fleet.ActivityEditedSetupExperienceSoftware{}.ActivityName(), - fmt.Sprintf(`{"platform": "darwin", "team_id": %d, "team_name": "%s"}`, team1.ID, team1.Name), 0) + s.lastActivityOfTypeMatches(fleet.ActivityEditedSetupExperienceSoftware{}.ActivityName(), + fmt.Sprintf(`{"platform":"darwin","team_id":%d,"team_name":%q}`, team1.ID, team1.Name), 0)And similarly for the later assertion:
- s.lastActivityOfTypeMatches(fleet.ActivityEditedSetupExperienceSoftware{}.ActivityName(), - fmt.Sprintf(`{"platform": "darwin", "team_id": %d, "team_name": "%s"}`, team1.ID, team1.Name), 0) + s.lastActivityOfTypeMatches(fleet.ActivityEditedSetupExperienceSoftware{}.ActivityName(), + fmt.Sprintf(`{"platform":"darwin","team_id":%d,"team_name":%q}`, team1.ID, team1.Name), 0)Also applies to: 15908-15910
15908-15910: Optionally assert both activities exist (latest and previous)To prove both PUTs emitted an activity, also check offset 1 for the earlier event.
Apply this diff:
s.lastActivityOfTypeMatches(fleet.ActivityEditedSetupExperienceSoftware{}.ActivityName(), fmt.Sprintf(`{"platform":"darwin","team_id":%d,"team_name":%q}`, team1.ID, team1.Name), 0) +s.lastActivityOfTypeMatches(fleet.ActivityEditedSetupExperienceSoftware{}.ActivityName(), + fmt.Sprintf(`{"platform":"darwin","team_id":%d,"team_name":%q}`, team1.ID, team1.Name), 1)server/fleet/setup_experience.go (2)
188-191: Use platform constant for consistency.Avoid the raw "darwin" literal; use the same MacOSPlatform constant used elsewhere.
-func IsSetupExperienceSupported(hostPlatform string) bool { - return hostPlatform == "darwin" || IsLinux(hostPlatform) -} +func IsSetupExperienceSupported(hostPlatform string) bool { + return hostPlatform == string(MacOSPlatform) || IsLinux(hostPlatform) +}
193-198: Decide: omit vs empty array in device payload.With
omitempty,softwaremay be omitted or null; many clients prefer an empty[]for stable contracts. If that’s desired, ensure handlers initialize tomake([]*SetupExperienceStatusResult, 0)when empty.server/datastore/mysql/hosts_test.go (1)
7556-7559: EnqueueSetupExperienceItems now takes platform — verify platform-like semantics.If this parameter expects a platform-like value (e.g., "linux" not "ubuntu"), passing host.Platform may be incorrect for Linux hosts. Here "darwin" is fine. Consider deriving/using a platform-like helper to avoid mismatch and optionally add a Linux case to cover the new path.
server/service/osquery_test.go (2)
1790-1791: Good addition: OsqueryHostID set for Linux hostSetting OsqueryHostID ensures HostUUIDForSetupExperience can prefer it on Linux. Consider adding a focused unit test that asserts HostUUIDForSetupExperience(host) returns OsqueryHostID when Platform is linux (and falls back to UUID otherwise).
1853-1855: Mock returns no setup-experience results — add a non-empty path testRight now the mock returns nil results. Add a test exercising a non-empty ListSetupExperienceResultsByHostUUID flow to lock in behavior (e.g., pending item present) and ensure no panics on result processing.
ee/server/service/setup_experience_test.go (1)
78-81: *API change to pass fleet.Host looks good; expand coverage
- Passing a host object is an improvement. Please:
- Add a Linux variant (Platform "linux") to verify the same sequencing (installer before script) for Linux setup experience.
- Assert the script execution payload matches the expected SetupExperienceScriptID and ScriptContentID for the queued script.
- Re-introduce a negative case (unknown host/UUID) to ensure a stable error path is still returned.
Also applies to: 99-102, 114-117, 136-139, 151-154, 176-179, 193-196, 210-213
server/service/endpoint_utils.go (1)
146-150: Add formatted bad request helper — minor polishHelper is fine. Optional: add a short doc comment for consistency with badRequest and to aid discovery.
server/service/devices.go (2)
800-809: Drop defensive type assertion; rely on decoder (also remove fmt).The request decoder ensures type-safety. The manual type-check adds noise and pulls in fmt just for error formatting. Simplify and align with other endpoints.
Apply:
@@ -import ( +import ( "context" "crypto/x509" "database/sql" "encoding/json" "errors" - "fmt" "io" "net/http" "net/url" "strconv" "time" @@ -func getDeviceSetupExperienceStatusEndpoint(ctx context.Context, request interface{}, svc fleet.Service) (fleet.Errorer, error) { - if _, ok := request.(*getDeviceSetupExperienceStatusRequest); !ok { - return nil, fmt.Errorf("internal error: invalid request type: %T", request) - } +func getDeviceSetupExperienceStatusEndpoint(ctx context.Context, request interface{}, svc fleet.Service) (fleet.Errorer, error) { + _ = request.(*getDeviceSetupExperienceStatusRequest) // decoder guarantees type results, err := svc.GetDeviceSetupExperienceStatus(ctx) if err != nil { - return &getDeviceSetupExperienceStatusResponse{Err: err}, nil + return getDeviceSetupExperienceStatusResponse{Err: err}, nil } - return &getDeviceSetupExperienceStatusResponse{Results: results}, nil + return getDeviceSetupExperienceStatusResponse{Results: results}, nil }Also applies to: 9-9
811-817: Implement device-scoped logic; don’t skipauth in final version.When you wire the real implementation, read the host from hostctx and enforce device-auth like other device endpoints; then compute and return the payload. Avoid SkipAuthorization here.
server/service/handler.go (2)
868-871: Expose a GET alias for an idempotent status read.This endpoint is read-only; consider adding GET alongside POST for consistency with other device “status” reads.
Apply:
de.WithCustomMiddleware( errorLimiter.Limit("setup_experience_status", desktopQuota, logger), ).POST("/api/_version_/fleet/device/{token}/setup_experience/status", getDeviceSetupExperienceStatusEndpoint, getDeviceSetupExperienceStatusRequest{}) +de.WithCustomMiddleware( + errorLimiter.Limit("setup_experience_status", desktopQuota, logger), +).GET("/api/_version_/fleet/device/{token}/setup_experience/status", getDeviceSetupExperienceStatusEndpoint, getDeviceSetupExperienceStatusRequest{})
925-925: Rate-limit Orbit init endpoint.Protect the new Orbit init with the existing error limiter bucket to avoid abuse bursts.
Apply:
-oe.POST("/api/fleet/orbit/setup_experience/init", orbitSetupExperienceInitEndpoint, orbitSetupExperienceInitRequest{}) +oe.WithCustomMiddleware( + errorLimiter.Limit("orbit_setup_experience_init", desktopQuota, logger), +).POST("/api/fleet/orbit/setup_experience/init", orbitSetupExperienceInitEndpoint, orbitSetupExperienceInitRequest{})server/fleet/datastore.go (2)
2070-2072: Document and validate allowed platform values ("linux" | "macos").These new methods accept a free-form platform string. Please document the accepted values and ensure implementations validate early to avoid silent no-ops.
Apply this doc-only diff:
- SetSetupExperienceSoftwareTitles(ctx context.Context, platform string, teamID uint, titleIDs []uint) error + // platform must be "linux" or "macos". + SetSetupExperienceSoftwareTitles(ctx context.Context, platform string, teamID uint, titleIDs []uint) error - ListSetupExperienceSoftwareTitles(ctx context.Context, platform string, teamID uint, opts ListOptions) ([]SoftwareTitleListResult, int, *PaginationMetadata, error) + // platform must be "linux" or "macos". + ListSetupExperienceSoftwareTitles(ctx context.Context, platform string, teamID uint, opts ListOptions) ([]SoftwareTitleListResult, int, *PaginationMetadata, error)
2101-2107: Prefer typed constants for platform_like values.Hardcoding "darwin"/"debian"/"rhel" in comments invites drift. Introduce a typed enum or constants (e.g., fleet.PlatformLikeDarwin, PlatformLikeDebian, PlatformLikeRHEL) and reference them across DS and service layers.
server/service/integration_mdm_setup_experience_test.go (3)
2039-2051: Fix misleading comment ("vim" → "emacs").This block records the result for the emacs install, not vim.
- // Record a result for vim. + // Record a result for emacs.
2060-2061: Remove duplicate assertion.Back-to-back require.Len on the same slice is redundant.
- require.Len(t, getDeviceStatusResponse.Results.Software, 1) - require.Len(t, getDeviceStatusResponse.Results.Software, 1) + require.Len(t, getDeviceStatusResponse.Results.Software, 1)
1901-1912: Fix misleading comment ("vim" → "ruby").This section updates Ruby’s result.
- // Record a result for vim. + // Record a result for ruby.ee/server/service/orbit.go (1)
291-316: Handle unknown PlatformLike gracefully in init.If PlatformLike is empty/unknown, EnqueueSetupExperienceItems may silently noop. Consider logging at info/warn and/or defaulting linux hosts without platform_like to include only generic tgz items.
server/datastore/mysql/setup_experience_test.go (2)
134-157: Add Linux enqueue coverageGreat Darwin coverage. Please add Linux cases to assert:
- Enqueue returns true when Linux-eligible installers exist.
- host_mdm_apple_awaiting_configuration is not set for Linux.
- setup_experience_status_results rows use the expected host UUID mapping.
I can draft a focused test (mirroring team1/team2) that calls EnqueueSetupExperienceItems(ctx, "linux", ...) and asserts no awaiting_configuration rows while verifying pending rows were inserted. Want me to propose it?
Also applies to: 230-245
379-404: Platform-param tests: include Linux List/Set pathsYou validated “darwin” and negative “ios”. Consider Linux-specific ListSetupExperienceSoftwareTitles/SetSetupExperienceSoftwareTitles tests (e.g., ensure VPP paths are excluded and only Linux installers are returned/toggled).
Happy to add a minimal Linux test that uploads a .deb/.rpm/.tar.gz and exercises Set→List→Unset.
Also applies to: 539-674
server/datastore/mysql/setup_experience.go (4)
21-60: Simplify installer platform predicate for clarityThe inner OR checks si.platform='darwin' while the outer condition already enforces si.platform=?; this is only true for darwin. Consider splitting the SELECT by platform or using CASE for readability. Behavior is correct but hard to audit.
141-149: Awaiting configuration set for macOS onlyThis writes to host_mdm_apple_awaiting_configuration only for darwin. If Linux setup items are enqueued, confirm there's an equivalent UX signal for Orbit/frontend. If not, consider a Linux-specific signal/table.
588-652: Two-step SELECT+UPDATE per status: consider single UPDATE with WHERE for fewer round-tripsCurrent flow reads id then updates. If no other fields are needed, a single UPDATE ... WHERE host_uuid = ? AND =? is simpler and avoids an extra query.
610-631: Add composite indexes for setup_experience_status_results
Existing migrations only define single-column indexes (idx_setup_experience_scripts_host_uuid, idx_setup_experience_scripts_hsi_id, idx_setup_experience_scripts_nano_command_uuid and idx_setup_experience_scripts_script_execution_id). Add composite indexes on:
- (host_uuid, host_software_installs_execution_id)
- (host_uuid, nano_command_uuid)
- (host_uuid, script_execution_id)
to ensure your UPDATE/SELECT queries use index lookups for both predicates.server/mock/datastore_mock.go (1)
1320-1320: Consider strong typing for platform arguments.Using raw string for platform/hostPlatform invites subtle drift (e.g., "linux" vs "Linux"). Consider a shared typed alias and constants (e.g., fleet.Platform) across the interfaces. Non-blocking for this mock file.
Also applies to: 1322-1322, 1336-1336
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
server/service/testdata/software-installers/test.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (29)
changes/32040-linux-setup-experience-backend(1 hunks)docs/Contributing/reference/audit-logs.md(1 hunks)ee/server/service/devices.go(1 hunks)ee/server/service/orbit.go(5 hunks)ee/server/service/setup_experience.go(2 hunks)ee/server/service/setup_experience_test.go(8 hunks)server/datastore/mysql/common_mysql/testing_utils/testing_utils.go(1 hunks)server/datastore/mysql/hosts.go(3 hunks)server/datastore/mysql/hosts_test.go(2 hunks)server/datastore/mysql/setup_experience.go(9 hunks)server/datastore/mysql/setup_experience_test.go(10 hunks)server/datastore/mysql/software_installers_test.go(1 hunks)server/fleet/activities.go(2 hunks)server/fleet/datastore.go(2 hunks)server/fleet/orbit.go(1 hunks)server/fleet/service.go(3 hunks)server/fleet/setup_experience.go(1 hunks)server/mock/datastore_mock.go(4 hunks)server/service/apple_mdm.go(1 hunks)server/service/devices.go(2 hunks)server/service/endpoint_utils.go(7 hunks)server/service/handler.go(3 hunks)server/service/integration_core_test.go(2 hunks)server/service/integration_mdm_setup_experience_test.go(11 hunks)server/service/integration_mdm_test.go(1 hunks)server/service/orbit.go(3 hunks)server/service/osquery.go(2 hunks)server/service/osquery_test.go(2 hunks)server/service/setup_experience.go(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
When reviewing SQL queries that are added or modified, ensure that appropriate filtering criteria are applied—especially when a query is intended to return data for a specific entity (e.g., a single host). Check for missing WHERE clauses or incorrect filtering that could lead to incorrect or non-deterministic results (e.g., returning the first row instead of the correct one). Flag any queries that may return unintended results due to lack of precise scoping.
Files:
server/datastore/mysql/common_mysql/testing_utils/testing_utils.goserver/service/integration_mdm_setup_experience_test.goserver/fleet/setup_experience.goserver/service/handler.goserver/fleet/orbit.goserver/service/integration_core_test.goee/server/service/devices.goserver/datastore/mysql/hosts.goserver/fleet/activities.goserver/service/devices.goserver/service/osquery.goserver/datastore/mysql/software_installers_test.goserver/mock/datastore_mock.goserver/service/integration_mdm_test.goserver/service/setup_experience.goserver/fleet/service.goee/server/service/orbit.goee/server/service/setup_experience_test.goserver/datastore/mysql/setup_experience_test.goserver/service/endpoint_utils.goserver/service/orbit.goserver/service/apple_mdm.goserver/datastore/mysql/hosts_test.goserver/fleet/datastore.goserver/service/osquery_test.goee/server/service/setup_experience.goserver/datastore/mysql/setup_experience.go
🪛 markdownlint-cli2 (0.17.2)
docs/Contributing/reference/audit-logs.md
2005-2005: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
2009-2009: Hard tabs
Column: 1
(MD010, no-hard-tabs)
2010-2010: Hard tabs
Column: 1
(MD010, no-hard-tabs)
2011-2011: Hard tabs
Column: 1
(MD010, no-hard-tabs)
🔇 Additional comments (24)
server/service/integration_core_test.go (1)
10300-10304: LGTM: helper signature uses platform consistentlyRenaming the parameter to platform reads clearer and matches the Host.Platform assignment below. No functional issues spotted.
server/datastore/mysql/common_mysql/testing_utils/testing_utils.go (1)
26-26: LGTM: formatting-only var init kept behavior identical.No functional change; initialization still occurs at package init and respects FLEET_MYSQL_TEST_PORT.
server/fleet/activities.go (2)
224-226: Add to ActivityDetailsList looks correct.The new activity is discoverable by the doc generator and APIs that enumerate activities.
2766-2786: Confirm canonical platform and team nullability handling
- Platform is already normalized via host.FleetPlatform()/PlatformFromHost and covered by the server/fleet/hosts_test.go tests.
- TeamID/TeamName use zero/empty rather than pointer-null like other activities—verify downstream UI/API treat
0/""as “no team” or refactor to pointers for consistency.server/datastore/mysql/hosts.go (1)
92-106: Verify null platform_like in secondary host INSERTs
Secondary INSERTs in server/datastore/mysql/hosts.go (lines 2262, 2369) and in server/datastore/mysql/android.go (line 32) omitplatform_like, so it will be null until detail ingestion. Confirm this is intended.docs/Contributing/reference/audit-logs.md (1)
2001-2004: Confirm “No team” semantics match backend.You document team_id as 0 and team_name as empty string for “No team.” Please confirm the emitted activity really uses 0/"" for this event (other activities sometimes use null or -1). If the backend differs, update the doc to match.
server/datastore/mysql/hosts_test.go (1)
7300-7301: Explicit platform on host initialization — good change.Setting Platform ensures the subsequent enqueue call has a concrete value.
server/service/endpoint_utils.go (2)
7-7: fmt import is appropriateUsed by badRequestf; no issues.
99-101: Signature reformatting onlyMulti-line wrapping of function signatures changes no behavior. OK to keep.
Also applies to: 108-123, 125-140, 152-176, 178-196, 198-222
server/service/osquery.go (2)
839-858: Platform-aware check looks good.Clean split (macOS uses awaiting-config, Linux uses result scan). After the change above, this will correctly gate for all item types.
890-897: Good placement for gating policies during setup.Runs after interval check, logs and skips policies when setup is active.
server/service/integration_mdm_setup_experience_test.go (1)
1609-2064: Strong, end-to-end Linux flow coverage.Great coverage: platform-scoped config, per-distro enqueueing, orbit init, status transitions, and policy gating all verified.
ee/server/service/orbit.go (1)
291-316: LGTM: orbit-scoped init endpoint.Auth skip is appropriate; hostctx usage and teamID derivation are correct. Returning a simple Enabled flag keeps the client contract clean.
server/fleet/service.go (1)
38-41: No outdatedSetupExperienceNextStepreferences remain: I found no call sites still passing a string UUID and no implementations using the old signature—this change is fully propagated.server/service/setup_experience.go (2)
24-27: Platform validation + internal mapping looks solidAllowing only "", "macos", or "linux" and mapping to "darwin" internally keeps the external API stable while aligning with datastore expectations. Usage at both PUT/GET endpoints is correct.
Also applies to: 36-38, 73-75, 275-287
18-22: Remove this check —{platform}is correctly bound on both PUT (line 389) and GET (line 391) routes inserver/service/handler.go.ee/server/service/setup_experience.go (1)
17-31: Platform normalization is already handled Confirmed that all call sites invoke SetSetupExperienceSoftware with the result of transformPlatformForSetupExperience (mapping “macos”→“darwin”), so no further changes are needed.server/datastore/mysql/setup_experience.go (4)
158-162: Platform-scoped setters look correctValidation + per-platform unsets/sets for installers and VPP teams align with new API. Good guardrails to prevent cross-platform assignment.
Also applies to: 246-253, 288-306
316-328: ListTitles platform filter passes through correctlyPassing Platform to ListSoftwareTitles ensures correct scoping. Good.
351-381: Host UUID scoping in SELECT is preciseQuery is appropriately scoped by host_uuid; safe against cross-host leakage.
424-452: GetSetupExperienceScript safe without ORDER BYUnique constraint
idx_setup_experience_scripts_global_or_team_idonglobal_or_team_idguarantees at most one row, so adding ORDER BY is unnecessary.server/mock/datastore_mock.go (3)
8194-8199: LGTM: forwards platform and follows mock pattern.Invoked flag set under lock; parameters and return match the function type; delegation is correct.
8201-8206: LGTM: platform-aware list wrapper is correct.Properly sets the invoked flag and forwards ctx, platform, teamID, and opts.
8250-8255: LGTM: enqueue wrapper propagates hostPlatform.Locking/flag pattern and argument order match the declared function type.
|
Left one comment about auth failure |
## PR 1/2 for #32037 - Implements update for the Linux setup experience from the IT admin's point of view. Updates for the end-user ("My device" page) to follow - Works in concert with the new endpoints implemented in #32493 - Splits Controls > Setup experience > Install software into 3 tabbed sections, one for each of macOS, Windows (placeholder state for now, to be implemented in following iteration), and Linux. - Dynamically calls new GET and PUT endpoints and routes data accordingly depending on which platform software for install is being updated for. - Update the software selection modal to display software package versions, including the package type (deb, rpm, or tar) for Linux software packges. - New activity feed item - Update relevant tests  _Note that the lower-right-hand image in this GIF is outdated and will be updated with new content once this entire feature is integrated_ ~- [ ] Changes file added for user-visible changes in `changes/`~ will include in PR 2/2 - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually - [x] Verified that any relevant UI is disabled when GitOps mode is enabled --------- Co-authored-by: Jacob Shandling <jacob@fleetdm.com>
## PR 2/2 for #32037 - Implements update for the Linux setup experience from the end-user's point of view (the "My device" page). - Works in concert with the new endpoints implemented in #32493 - My device page calls a new endpoint to get in-progress setup experience software installations. If there are any, the page is replaced with a "Setting up your device" page - The UI polls this endpoint until all such installations are either successful or failed (including canceled) - Setting up your device page includes a table displaying the name and status of each software installation - Once all installations are finished (succeed/fail), renders the regular My device page - Add a handler for the new API call for relevant tests  ## Testing Can use [this branch with fake data](https://github.com/fleetdm/fleet/tree/32037-end-user-fake-data) to help test this PR - [x] Changes file added for user-visible changes in `changes/` - [x] Added/updated automated tests - additional tests coming in follow-up - [x] QA'd all new/changed functionality manually --------- Co-authored-by: Jacob Shandling <jacob@fleetdm.com>
4 participants
For #32040.
Backend changes to unblock the development of the orbit and frontend changes.
New GET and PUT APIs for setting/getting software for Linux Setup Experience:
New setup_experience/init API called by orbit to trigger the Linux setup experience on the device:
Get status API to call on "My device":
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
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
New Fleet configuration settings
fleetctl generate-gitopsSummary by CodeRabbit
New Features
Documentation