Custom host vitals: schema + CRUD (backend)#48706
Conversation
|
@coderabbitai full review |
✅ Action performedFull review finished. |
WalkthroughThis PR adds a "custom host vitals" feature: globally-defined, per-host key/value fields. It introduces authorization rules, new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Service
participant Authz
participant Datastore
Client->>Service: CreateCustomHostVital(name)
Service->>Authz: authorize write
Service->>Service: ValidateCustomHostVitalName(name)
Service->>Datastore: CreateCustomHostVital(name)
Datastore-->>Service: CustomHostVital
Service->>Datastore: NewActivity(CreatedCustomHostVital)
Service-->>Client: *CustomHostVital
Related issues: Suggested labels: backend, enhancement, needs review Suggested reviewers: fleetdm backend maintainers familiar with authz policy and MySQL datastore layers 🐰 A vital new field, host by host, 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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)
server/service/hosts_test.go (1)
94-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate mock stub assignments.
ConditionalAccessBypassedAtFuncandGetHostCustomHostVitalsFuncare each assigned twice in this block (lines 94-96 duplicated at 103-105, and 97-99 duplicated at 106-108). Harmless but likely a rebase artifact.🧹 Proposed cleanup
ds.ConditionalAccessBypassedAtFunc = func(ctx context.Context, hostID uint) (*time.Time, error) { return nil, nil } ds.GetHostCustomHostVitalsFunc = func(ctx context.Context, hostID uint) ([]fleet.HostCustomHostVital, error) { return nil, nil } ds.IsHostDiskEncryptionKeyArchivedFunc = func(ctx context.Context, hostID uint) (bool, error) { return false, nil } - ds.ConditionalAccessBypassedAtFunc = func(ctx context.Context, hostID uint) (*time.Time, error) { - return nil, nil - } - ds.GetHostCustomHostVitalsFunc = func(ctx context.Context, hostID uint) ([]fleet.HostCustomHostVital, error) { - return nil, nil - }🤖 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/service/hosts_test.go` around lines 94 - 108, Remove the duplicated mock stub assignments in the test setup so each function is only configured once; in the host test block, keep a single assignment for ConditionalAccessBypassedAtFunc and a single assignment for GetHostCustomHostVitalsFunc, and delete the repeated copies introduced after IsHostDiskEncryptionKeyArchivedFunc.server/service/custom_host_vitals_test.go (1)
96-130: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for team-scoped success and vital-not-found paths.
Two gaps in
TestSetHostCustomHostVitalValueAuth:
- The comment claims "team maintainers/admins (of the host's team) can set it," but
HostLiteFuncalways returns a host with noTeamID, so no team-scoped success case is actually exercised — only failing cases are covered.- No case exercises
customHostVitalByIDreturningNotFound(e.g.,GetCustomHostVitalsFuncreturning an empty slice), even though that's an explicit guard inSetHostCustomHostVitalValue.🤖 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/service/custom_host_vitals_test.go` around lines 96 - 130, `TestSetHostCustomHostVitalValueAuth` only covers global roles and negative team observer behavior, so it never verifies the team-scoped allow path or the not-found guard. Update the setup around `HostLiteFunc` in `SetHostCustomHostVitalValue` tests to return a host with a matching `TeamID`, then add a case for a team maintainer/admin on that team that succeeds. Also add a separate case where `GetCustomHostVitalsFunc` returns no vitals so `customHostVitalByID` in `SetHostCustomHostVitalValue` hits the `NotFound` path and the test asserts that error.
🤖 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.
Nitpick comments:
In `@server/service/custom_host_vitals_test.go`:
- Around line 96-130: `TestSetHostCustomHostVitalValueAuth` only covers global
roles and negative team observer behavior, so it never verifies the team-scoped
allow path or the not-found guard. Update the setup around `HostLiteFunc` in
`SetHostCustomHostVitalValue` tests to return a host with a matching `TeamID`,
then add a case for a team maintainer/admin on that team that succeeds. Also add
a separate case where `GetCustomHostVitalsFunc` returns no vitals so
`customHostVitalByID` in `SetHostCustomHostVitalValue` hits the `NotFound` path
and the test asserts that error.
In `@server/service/hosts_test.go`:
- Around line 94-108: Remove the duplicated mock stub assignments in the test
setup so each function is only configured once; in the host test block, keep a
single assignment for ConditionalAccessBypassedAtFunc and a single assignment
for GetHostCustomHostVitalsFunc, and delete the repeated copies introduced after
IsHostDiskEncryptionKeyArchivedFunc.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 49b5872e-efb0-4a81-a57f-43668759b083
⛔ Files ignored due to path filters (1)
docs/Contributing/reference/audit-logs.mdis excluded by!**/*.md
📒 Files selected for processing (24)
server/authz/policy.regoserver/authz/policy_test.goserver/datastore/mysql/custom_host_vitals.goserver/datastore/mysql/custom_host_vitals_test.goserver/datastore/mysql/migrations/tables/20260702175822_CustomHostVitals.goserver/datastore/mysql/migrations/tables/20260702175822_CustomHostVitals_test.goserver/datastore/mysql/schema.sqlserver/fleet/activities.goserver/fleet/api_custom_host_vitals.goserver/fleet/custom_host_vitals.goserver/fleet/datastore.goserver/fleet/fleet_vars.goserver/fleet/hosts.goserver/fleet/service.goserver/mock/datastore_mock.goserver/mock/service/service_mock.goserver/service/custom_host_vitals.goserver/service/custom_host_vitals_test.goserver/service/devices_endpoint_test.goserver/service/handler.goserver/service/hosts.goserver/service/hosts_test.goserver/service/integration_custom_host_vitals_test.goserver/service/testing_utils_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 44954-custom-host-vitals #48706 +/- ##
===========================================================
Coverage ? 68.00%
===========================================================
Files ? 3682
Lines ? 233874
Branches ? 12385
===========================================================
Hits ? 159049
Misses ? 60496
Partials ? 14329
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![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.
…tVitalPrefix to its domain file
2 participants
Related issue: Resolves #48559
Adds the schema (global definitions + per-host values) and the CRUD API: create/list/rename/delete definitions, set a per-host value, the host-details embed, and the related activities.
Checklist for submitter
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Will be added in the feature branch directly.
Testing
Added/updated automated tests
QA'd all new/changed functionality manually
Summary by CodeRabbit
New Features
Bug Fixes