From 348fa47c84d392719174ea84cb2acd0236e03a4d Mon Sep 17 00:00:00 2001 From: Scott Gress Date: Mon, 11 May 2026 11:49:02 -0700 Subject: [PATCH] Improve name key validation in GitOps (#44553) **Related issue:** Resolves # # Checklist for submitter If some of the following don't apply, delete the relevant line. - [X] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. ## Testing - [X] Added/updated automated tests - [X] QA'd all new/changed functionality manually - [X] omitted `name:` from a file without `org_settings:`, got: ``` * No `name` was provided in /tmp/testback/fleets/third-fleet.yml. If this file is intended to define org-level settings, add `org_settings:` as a top-level key. Otherwise, use `name` to specify the fleet name. ``` - [X] omitted `name:` from a file with `org_settings:`, got no error. - [X] omitted `name:` from `no-team.yml`, got: ``` * `name` must be `No Team` for `no-team.yml` ``` - [X] omitted `name:` from `unassigned.yml`, got: ``` * `name` must be `Unassigned` for `unassigned.yml` ``` ## Summary by CodeRabbit * **Bug Fixes** * Improved error messages when GitOps YAML files omit the required `name` field, with specific remediation guidance tailored to each configuration file type * Enhanced validation error messaging when top-level `org_settings` is missing or incorrectly placed, providing clearer instructions on required YAML structure --- ...43857-improve-error-on-missing-gitops-name | 1 + cmd/fleetctl/fleetctl/gitops_test.go | 2 +- pkg/spec/gitops.go | 13 ++++-- pkg/spec/gitops_test.go | 42 ++++++++++++++++--- 4 files changed, 49 insertions(+), 9 deletions(-) create mode 100644 changes/43857-improve-error-on-missing-gitops-name diff --git a/changes/43857-improve-error-on-missing-gitops-name b/changes/43857-improve-error-on-missing-gitops-name new file mode 100644 index 0000000000..3a064f2763 --- /dev/null +++ b/changes/43857-improve-error-on-missing-gitops-name @@ -0,0 +1 @@ +- Improved error when `name` key is omitted from a GitOps YAML file. diff --git a/cmd/fleetctl/fleetctl/gitops_test.go b/cmd/fleetctl/fleetctl/gitops_test.go index c3ae8e7c10..d04ef3b59d 100644 --- a/cmd/fleetctl/fleetctl/gitops_test.go +++ b/cmd/fleetctl/fleetctl/gitops_test.go @@ -155,7 +155,7 @@ org_settings: require.NoError(t, err) _, err = RunAppNoChecks([]string{"gitops", "-f", badFile.Name()}) require.Error(t, err) - assert.Contains(t, err.Error(), "'org_settings' is required") + assert.Contains(t, err.Error(), "add `org_settings:` as a top-level key") // DoGitOps error t.Setenv("ORG_NAME", "") diff --git a/pkg/spec/gitops.go b/pkg/spec/gitops.go index 6f6a79c88c..3c4674461f 100644 --- a/pkg/spec/gitops.go +++ b/pkg/spec/gitops.go @@ -468,10 +468,10 @@ func GitOpsFromFile(filePath, baseDir string, appConfig *fleet.EnrichedAppConfig // If the file is no-team.yml, the name must be "No team". switch { case filepath.Base(filePath) == "no-team.yml" && !result.IsNoTeam(): - multiError = multierror.Append(multiError, fmt.Errorf("file %q must have team name 'No Team'", filePath)) + multiError = multierror.Append(multiError, errors.New("`name` must be `No Team` for `no-team.yml`")) return result, multiError.ErrorOrNil() case filepath.Base(filePath) == "unassigned.yml" && !result.IsUnassignedTeam(): - multiError = multierror.Append(multiError, fmt.Errorf("file %q must have team name 'Unassigned'", filePath)) + multiError = multierror.Append(multiError, errors.New("`name` must be `Unassigned` for `unassigned.yml`")) return result, multiError.ErrorOrNil() case result.IsNoTeam() && filepath.Base(filePath) != "no-team.yml": multiError = multierror.Append(multiError, fmt.Errorf("file `%s` for No Team must be named `no-team.yml`", filePath)) @@ -496,7 +496,14 @@ func GitOpsFromFile(filePath, baseDir string, appConfig *fleet.EnrichedAppConfig multiError = parseTeamSettings(settingsRaw, result, baseDir, filePath, multiError) } default: - multiError = multierror.Append(multiError, errors.New("if `name` is not provided, 'org_settings' is required")) + switch filepath.Base(filePath) { + case "no-team.yml": + multiError = multierror.Append(multiError, errors.New("`name` must be `No Team` for `no-team.yml`")) + case "unassigned.yml": + multiError = multierror.Append(multiError, errors.New("`name` must be `Unassigned` for `unassigned.yml`")) + default: + multiError = multierror.Append(multiError, fmt.Errorf("No `name` was provided in %s. If this file is intended to define org-level settings, add `org_settings:` as a top-level key. Otherwise, use `name` to specify the fleet name.", filePath)) + } } for _, topKey := range topKeys { diff --git a/pkg/spec/gitops_test.go b/pkg/spec/gitops_test.go index 4052d72262..5970d85344 100644 --- a/pkg/spec/gitops_test.go +++ b/pkg/spec/gitops_test.go @@ -728,6 +728,38 @@ func TestWhitespaceOnlyTeamName(t *testing.T) { require.Contains(t, err.Error(), "team 'name' is required") } +func TestMissingNameErrorMessages(t *testing.T) { + t.Parallel() + + // Empty default.yml should report the generic missing-name guidance. + defaultPath, defaultBase := createNamedFileOnTempDir(t, "default.yml", "") + _, err := GitOpsFromFile(defaultPath, defaultBase, nil, nopLogf) + require.Error(t, err) + assert.Contains(t, err.Error(), "No `name` was provided") + assert.Contains(t, err.Error(), "add `org_settings:` as a top-level key.") + assert.Contains(t, err.Error(), "Otherwise, use `name` to specify the fleet name.") + + // Empty no-team.yml should report the No Team name requirement. + noTeamPath, noTeamBase := createNamedFileOnTempDir(t, "no-team.yml", "") + _, err = GitOpsFromFile(noTeamPath, noTeamBase, nil, nopLogf) + require.Error(t, err) + assert.Contains(t, err.Error(), "`name` must be `No Team` for `no-team.yml`") + + // Empty unassigned.yml should report the Unassigned name requirement. + unassignedPath, unassignedBase := createNamedFileOnTempDir(t, "unassigned.yml", "") + _, err = GitOpsFromFile(unassignedPath, unassignedBase, nil, nopLogf) + require.Error(t, err) + assert.Contains(t, err.Error(), "`name` must be `Unassigned` for `unassigned.yml`") + + // Any other team file missing `name` should report the generic requirement. + teamPath, teamBase := createNamedFileOnTempDir(t, "workstations.yml", "") + _, err = GitOpsFromFile(teamPath, teamBase, nil, nopLogf) + require.Error(t, err) + assert.Contains(t, err.Error(), "No `name` was provided") + assert.Contains(t, err.Error(), "add `org_settings:` as a top-level key.") + assert.Contains(t, err.Error(), "Otherwise, use `name` to specify the fleet name.") +} + func TestPaddedTeamNameIsTrimmed(t *testing.T) { t.Parallel() config := getTeamConfig([]string{"name"}) @@ -932,28 +964,28 @@ func TestInvalidGitOpsYaml(t *testing.T) { config += "name: SomeOtherTeam\nsettings:\n secrets:\n" noTeamPath7, noTeamBasePath7 := createNamedFileOnTempDir(t, "no-team.yml", config) _, err = GitOpsFromFile(noTeamPath7, noTeamBasePath7, nil, nopLogf) - assert.ErrorContains(t, err, fmt.Sprintf("file %q must have team name 'No Team'", noTeamPath7)) + require.ErrorContains(t, err, "`name` must be `No Team` for `no-team.yml`") // unassigned.yml with a non-"Unassigned" name should fail. config = getConfig([]string{"name", "settings"}) config += "name: SomeOtherTeam\nsettings:\n secrets:\n" unassignedPathBadName, unassignedBasePathBadName := createNamedFileOnTempDir(t, "unassigned.yml", config) _, err = GitOpsFromFile(unassignedPathBadName, unassignedBasePathBadName, nil, nopLogf) - assert.ErrorContains(t, err, fmt.Sprintf("file %q must have team name 'Unassigned'", unassignedPathBadName)) + require.ErrorContains(t, err, "`name` must be `Unassigned` for `unassigned.yml`") // no-team.yml with "Unassigned" name should fail (wrong name for this file). config = getConfig([]string{"name", "settings"}) config += "name: Unassigned\n" noTeamPath8, noTeamBasePath8 := createNamedFileOnTempDir(t, "no-team.yml", config) _, err = GitOpsFromFile(noTeamPath8, noTeamBasePath8, nil, nopLogf) - assert.ErrorContains(t, err, fmt.Sprintf("file %q must have team name 'No Team'", noTeamPath8)) + require.ErrorContains(t, err, "`name` must be `No Team` for `no-team.yml`") // unassigned.yml with "No team" name should fail (wrong name for this file). config = getConfig([]string{"name", "settings"}) config += "name: No team\n" unassignedPathNoTeam, unassignedBasePathNoTeam := createNamedFileOnTempDir(t, "unassigned.yml", config) _, err = GitOpsFromFile(unassignedPathNoTeam, unassignedBasePathNoTeam, nil, nopLogf) - assert.ErrorContains(t, err, fmt.Sprintf("file %q must have team name 'Unassigned'", unassignedPathNoTeam)) + require.ErrorContains(t, err, "`name` must be `Unassigned` for `unassigned.yml`") // 'Unassigned' team in unassigned.yml should work and coerce to "No team" internally. config = getConfig([]string{"name", "settings"}) @@ -1221,7 +1253,7 @@ func TestTopLevelGitOpsValidation(t *testing.T) { if test.shouldPass { assert.NoError(t, err) } else { - assert.ErrorContains(t, err, "is required") + assert.ErrorContains(t, err, "add `org_settings:` as a top-level key") } }, )