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") } }, )