Improve custom host vital missing-value error message - #49640
Conversation
…d its variable Include the custom host vital's name alongside its $FLEET_HOST_VITAL_<id> token so the delivery failure detail is actionable for admins.
There was a problem hiding this comment.
Pull request overview
Improves the admin-facing delivery failure detail when a referenced custom host vital has no per-host value, by including both the vital’s human name and its $FLEET_HOST_VITAL_<id> token. This makes it easier to identify which vital needs a value set on the host.
Changes:
- Extend
MissingCustomHostVitalValueErrorto carry vital names alongside IDs and render them in the error message. - Populate missing vital names during datastore expansion (
ExpandCustomHostVitals). - Update/extend tests to assert the new error payload and message content.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| server/fleet/custom_host_vitals.go | Adds MissingNames to the error type and updates the error string to include name (token). |
| server/fleet/custom_host_vitals_test.go | Updates tests to validate the new error message format (single + multi). |
| server/datastore/mysql/custom_host_vitals.go | Collects missing vital names during expansion and returns them in the error. |
| server/datastore/mysql/software_installers_test.go | Extends integration test assertions to include MissingNames in the returned error. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for i, id := range e.MissingIDs { | ||
| var name string | ||
| if i < len(e.MissingNames) { | ||
| name = e.MissingNames[i] | ||
| } | ||
| tokens = append(tokens, fmt.Sprintf("%s ($%s%d)", name, CustomHostVitalPrefix, id)) | ||
| } |
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughCustom host vital expansion now records names corresponding to missing vital IDs. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #49640 +/- ##
==========================================
+ Coverage 67.85% 67.89% +0.04%
==========================================
Files 3889 3889
Lines 247605 248324 +719
Branches 13022 13022
==========================================
+ Hits 168004 168602 +598
- Misses 64450 64523 +73
- Partials 15151 15199 +48
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:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
server/fleet/custom_host_vitals.go:96
- If MissingNames is empty/shorter than MissingIDs (e.g. callers that only set MissingIDs), the formatted token becomes " ($FLEET_HOST_VITAL_)" with a leading space and no name. Consider falling back to just the token when the name is missing/blank to keep the error message readable and resilient to partial struct initialization.
for i, id := range e.MissingIDs {
var name string
if i < len(e.MissingNames) {
name = e.MissingNames[i]
}
tokens = append(tokens, fmt.Sprintf("%s ($%s%d)", name, CustomHostVitalPrefix, id))
sharon-fdm
left a comment
sharon-fdm
left a comment
There was a problem hiding this comment.
Claude-generated review -- minor items only, LGTM overall.
Also noting: missing change file (changes/ directory). This is a user-visible error message improvement, so it should have one for the changelog.
| tokens = append(tokens, fmt.Sprintf("\"$%s%d\"", CustomHostVitalPrefix, id)) | ||
| for i, id := range e.MissingIDs { | ||
| var name string | ||
| if i < len(e.MissingNames) { |
There was a problem hiding this comment.
Minor: when the guard fails (or the name in the map was empty due to a deleted vital), name is "", producing a message like " ($FLEET_HOST_VITAL_42)" with a leading space and no name. Consider a fallback:
| if i < len(e.MissingNames) { | |
| if i < len(e.MissingNames) && e.MissingNames[i] != "" { | |
| name = e.MissingNames[i] | |
| } else { | |
| name = fmt.Sprintf("$%s%d", CustomHostVitalPrefix, id) | |
| } |
There was a problem hiding this comment.
I think this would be too defensive and it's not a real-world scenario for a vital to have an empty name (it's required for the insertion at the DB layer).
Changes file was already included as part of the feature (#44954), this is just a follow-up which will land with the entire feature. |
Related issue: Resolves #44954
When a custom host vital referenced in a script or profile has no value set for a host, the delivery failure detail didn't name the vital, making it hard for admins to tell which one needed a value. The message now includes both the vital's name and its
$FLEET_HOST_VITAL_<id>token.Testing
Added/updated automated tests
QA'd all new/changed functionality manually
Summary by CodeRabbit