Improve error message when referencing a bad label for a configuration profile (#44839)
Closes #39739 ## Local reproduction Reproduced the bug locally by running `fleetctl gitops --dry-run` against a local Fleet server with a GitOps config that references a nonexistent label on a configuration profile. **Setup:** 1. Started a local Fleet server (`fleet serve` against Docker MySQL/Redis on `https://localhost:8080`, fresh database). 2. Created a minimal `.mobileconfig` profile (`test-profile.mobileconfig`). 3. Created a `default.yml` that references it under `controls.macos_settings.custom_settings` with `labels_include_all: ["this-label-does-not-exist"]`. **Reproduction:** ```bash fleetctl gitops -f /tmp/repro-39739/default.yml --dry-run ``` **Result (before fix):** ``` [!] Unknown label 'this-label-does-not-exist' is referenced by MDM Profile '/tmp/repro-39739/profiles/test-profile.mobileconfig' Error: Please create the missing labels, or update your settings to not refer to these labels. ``` Two problems visible: - Says "MDM Profile" — internal jargon, not the user-facing term - Shows the full absolute path — noisy and unhelpful --- ## Code changes **Summary:** Two lines changed in `cmd/fleetctl/fleetctl/gitops.go` inside the `getLabelUsage()` function. Both fix how configuration profile label errors are displayed to the user during `fleetctl gitops` runs. ### `cmd/fleetctl/fleetctl/gitops.go` **Line 851 — "multiple label keys" error message:** Changed `"MDM profile"` → `"configuration profile"` and wrapped `setting.Path` in `filepath.Base()` so the error shows just the filename instead of the full absolute path. ```diff - err := fmt.Errorf("MDM profile '%s' has multiple label keys; ...", setting.Path) + err := fmt.Errorf("configuration profile '%s' has multiple label keys; ...", filepath.Base(setting.Path)) ``` **Line 869 — label usage tracking entry:** Changed the type string from `"MDM Profile"` → `"configuration profile"` and the identifier from the full `setting.Path` to `filepath.Base(setting.Path)`. This feeds into the error message on line 458: `[!] Unknown label '<name>' is referenced by <type> '<identifier>'` ```diff - updateLabelUsage(labels, setting.Path, "MDM Profile", result) + updateLabelUsage(labels, filepath.Base(setting.Path), "configuration profile", result) ``` **After fix:** ``` [!] Unknown label 'this-label-does-not-exist' is referenced by configuration profile 'test-profile.mobileconfig' ``` --- ## Testing ### Manual testing 1. Started a local Fleet server (fresh DB, `fleet serve` on `https://localhost:8080`). 2. Created a minimal `.mobileconfig` profile and a `default.yml` GitOps config that references it with `labels_include_all: ["this-label-does-not-exist"]`. 3. Built `fleetctl` from the **unfixed** code (`git stash`) and ran `fleetctl gitops -f default.yml --dry-run`. Confirmed the old error message: ``` [!] Unknown label 'this-label-does-not-exist' is referenced by MDM Profile '/tmp/repro-39739/profiles/test-profile.mobileconfig' ``` 4. Built `fleetctl` from the **fixed** code and ran the same command. Confirmed the new error message: ``` [!] Unknown label 'this-label-does-not-exist' is referenced by configuration profile 'test-profile.mobileconfig' ``` ### Unit tests added New file: `cmd/fleetctl/fleetctl/gitops_label_usage_test.go` — two tests that exercise `getLabelUsage()` directly (no Redis/MySQL needed): - **`TestGetLabelUsageProfilePathShortened`**: Creates a `GitOps` config with a macOS profile using a full absolute path and a nonexistent label. Asserts the label usage entry has the basename (not the full path) and the type is `"configuration profile"` (not `"MDM Profile"`). - **`TestGetLabelUsageMultipleLabelKeysError`**: Creates a config with both `labels_include_all` and `labels_include_any` on the same profile. Asserts the error contains `"configuration profile"` and the short filename, and does **not** contain the directory path. Both tests were verified to **fail on unfixed code** and **pass on the fix** via a `git stash` round-trip. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Enhanced error messages for MDM configuration profile label validation to display concise filenames instead of full file paths, improving user experience. * **Refactor** * Updated internal label usage tracking to use configuration profile base filenames for consistency and clarity. * **Tests** * Added test coverage for configuration profile path shortening and error message validation in label key scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This commit is contained in:
@@ -851,7 +851,7 @@ func getLabelUsage(config *spec.GitOps) (map[string][]LabelUsage, error) {
|
||||
if osSettings, ok := getCustomSettings(osSettingName); ok {
|
||||
for _, setting := range osSettings {
|
||||
var labels []string
|
||||
err := fmt.Errorf("MDM profile '%s' has multiple label keys; please choose one of `labels_include_any`, `labels_include_all` or `labels_exclude_any`.", setting.Path)
|
||||
err := fmt.Errorf("configuration profile '%s' has multiple label keys; please choose one of `labels_include_any`, `labels_include_all` or `labels_exclude_any`.", filepath.Base(setting.Path))
|
||||
|
||||
if len(setting.LabelsIncludeAny) > 0 {
|
||||
labels = setting.LabelsIncludeAny
|
||||
@@ -869,7 +869,7 @@ func getLabelUsage(config *spec.GitOps) (map[string][]LabelUsage, error) {
|
||||
labels = setting.LabelsExcludeAny
|
||||
}
|
||||
|
||||
updateLabelUsage(labels, setting.Path, "MDM Profile", result)
|
||||
updateLabelUsage(labels, filepath.Base(setting.Path), "configuration profile", result)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -0,0 +1,70 @@
|
||||
package fleetctl
|
||||
|
||||
import (
|
||||
"testing"
|
||||
|
||||
"github.com/fleetdm/fleet/v4/pkg/spec"
|
||||
"github.com/fleetdm/fleet/v4/server/fleet"
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/require"
|
||||
)
|
||||
|
||||
func TestGetLabelUsageProfilePathShortened(t *testing.T) {
|
||||
// Simulate what happens when spec parsing resolves a relative path to absolute.
|
||||
absPath := "/home/runner/work/Detroit-GitOps-Workshop/Detroit-GitOps-Workshop/lib/macos/configuration-profiles/disable-bluetooth-file-sharing.mobileconfig"
|
||||
|
||||
config := &spec.GitOps{
|
||||
Controls: spec.GitOpsControls{
|
||||
MacOSSettings: &fleet.MacOSSettings{
|
||||
CustomSettings: []fleet.MDMProfileSpec{
|
||||
{
|
||||
Path: absPath,
|
||||
LabelsIncludeAll: []string{"nonexistent-label"},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
}
|
||||
|
||||
usage, err := getLabelUsage(config)
|
||||
require.NoError(t, err)
|
||||
|
||||
// The label "nonexistent-label" should be in the usage map.
|
||||
entries, ok := usage["nonexistent-label"]
|
||||
require.True(t, ok, "expected label to be in usage map")
|
||||
require.Len(t, entries, 1)
|
||||
|
||||
// The Name should be the base filename, not the full absolute path.
|
||||
assert.Equal(t, "disable-bluetooth-file-sharing.mobileconfig", entries[0].Name,
|
||||
"profile path should be shortened to just the filename")
|
||||
|
||||
// The Type should be "configuration profile", not "MDM Profile".
|
||||
assert.Equal(t, "configuration profile", entries[0].Type,
|
||||
"type should say 'configuration profile' not 'MDM Profile'")
|
||||
}
|
||||
|
||||
func TestGetLabelUsageMultipleLabelKeysError(t *testing.T) {
|
||||
absPath := "/absolute/path/to/profile.mobileconfig"
|
||||
|
||||
config := &spec.GitOps{
|
||||
Controls: spec.GitOpsControls{
|
||||
MacOSSettings: &fleet.MacOSSettings{
|
||||
CustomSettings: []fleet.MDMProfileSpec{
|
||||
{
|
||||
Path: absPath,
|
||||
LabelsIncludeAll: []string{"label-a"},
|
||||
LabelsIncludeAny: []string{"label-b"},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
}
|
||||
|
||||
_, err := getLabelUsage(config)
|
||||
require.Error(t, err)
|
||||
|
||||
// Error should use "configuration profile" and the short filename.
|
||||
assert.Contains(t, err.Error(), "configuration profile")
|
||||
assert.Contains(t, err.Error(), "profile.mobileconfig")
|
||||
assert.NotContains(t, err.Error(), "/absolute/path/to/")
|
||||
}
|
||||
Reference in New Issue
Block a user