From 79cd2f1d85fcc79e8586c8685db90b2509215f44 Mon Sep 17 00:00:00 2001 From: Robbie Trencheny Date: Tue, 18 Aug 2026 15:02:57 -0400 Subject: [PATCH 1/3] feat(diff): field-level diff for the team settings block Closes #40. A team's `settings:` block (and the older `team_settings:` spelling) was accepted by the parser but never diffed, so changing a webhook URL or a host expiry window produced no output at all. GET /teams already returns the live values: webhook_settings, host_expiry_settings, integrations, and features sit on the team object next to software and agent_options. The client now keeps the raw team JSON alongside the typed struct, and the diff engine compares the YAML block against it key by key using the same flattening the global config diff uses. Details: - parser: ParsedTeam.Settings holds the block as a nested map. `settings:` wins over `team_settings:` when a file carries both, matching fleetctl gitops. - api: Team.UnmarshalJSON decodes into both the typed struct and a generic map, so a settings sub-key Fleet adds later needs no code change here. - diff: diffTeamSettings emits ConfigChange rows under the "settings" section. Sub-keys Fleet does not expose (for example `mdm`) are reported as skipped rather than reported as changes, and results are sorted so output is stable despite map iteration order. - `secrets:` is never diffed. Enroll secrets are credentials and this output lands in CI logs and MR comments. Values containing `$` are skipped as before, since Fleet substitutes them server-side. - Baseline subtraction covers settings too, so a change already merged to the base branch does not reappear in a later MR. No output changes: the renderers already handle per-result Config rows. Verified against the live Fleet instance with the production fleet-gitops repo: matching settings produce no rows, and a modified copy of the repo produces exactly the four expected rows. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01KEpzMNJnGaBLAfrPeqknCy --- docs/API-Endpoints.md | 2 +- docs/Architecture.md | 5 +- internal/api/client.go | 23 +++ internal/api/client_test.go | 50 +++++++ internal/diff/differ.go | 87 +++++++++++ internal/diff/differ_test.go | 255 ++++++++++++++++++++++++++++++++ internal/output/terminal.go | 3 +- internal/parser/parser.go | 34 ++++- internal/parser/parser_test.go | 95 ++++++++++++ testdata/teams/workstations.yml | 11 +- 10 files changed, 558 insertions(+), 7 deletions(-) diff --git a/docs/API-Endpoints.md b/docs/API-Endpoints.md index f0b4400..2b683ba 100644 --- a/docs/API-Endpoints.md +++ b/docs/API-Endpoints.md @@ -5,7 +5,7 @@ All read-only. fleet-plan never writes to your Fleet server. | Method | Endpoint | Purpose | |--------|----------|---------| | `GET` | `/api/v1/fleet/config` | Global config (org_settings, agent_options, controls) | -| `GET` | `/api/v1/fleet/teams` | Team list + embedded software config | +| `GET` | `/api/v1/fleet/teams` | Team list + embedded software config + team settings (`webhook_settings`, `host_expiry_settings`, `integrations`, `features`) | | `GET` | `/api/v1/fleet/labels` | Label validation and host counts | | `GET` | `/api/v1/fleet/teams/{id}/policies` | Per-team policies | | `GET` | `/api/v1/fleet/global/policies` | Global policies (when default.yml parsed) | diff --git a/docs/Architecture.md b/docs/Architecture.md index 72a91e7..90ff4dc 100644 --- a/docs/Architecture.md +++ b/docs/Architecture.md @@ -81,7 +81,7 @@ Config file supports multiple contexts: ## Parser -Walks `teams/*.yml`, resolves `path:` references, produces `ParsedRepo`. Also parses `default.yml` for labels, `org_settings`, `agent_options`, `controls`, and global policies/queries. All path references are validated against the repo root to prevent traversal. +Walks `teams/*.yml`, resolves `path:` references, produces `ParsedRepo`. Also parses `default.yml` for labels, `org_settings`, `agent_options`, `controls`, and global policies/queries. A team's `settings:` block (or the older `team_settings:` spelling) is kept as a nested map for field-level diffing. All path references are validated against the repo root to prevent traversal. --- @@ -91,7 +91,8 @@ Compares `FleetState` (API) vs `ParsedRepo` (YAML). Produces `[]DiffResult` per | Resource | Match key | Diff fields | |----------|-----------|-------------| -| Config sections | dot-path key | old/new value (skips `$VAR` placeholders) | +| Config sections (global) | dot-path key | old/new value (skips `$VAR` placeholders) | +| Team `settings:` | dot-path key | old/new value vs the team object from `GET /teams`; `secrets:` is never diffed | | Policies | `name` | query, description, resolution, platform, critical | | Queries | `name` | query, interval, platform, logging | | Software packages | `referenced_yaml_path` | url, hash, self_service | diff --git a/internal/api/client.go b/internal/api/client.go index 167ca55..bcc3760 100644 --- a/internal/api/client.go +++ b/internal/api/client.go @@ -139,6 +139,29 @@ type Team struct { SoftwareUnavailable bool // true when GetSoftware returned 403/404 (token lacks permission) ProfilesUnavailable bool // true when GetProfiles returned 403/404 (token lacks permission) ScriptsUnavailable bool // true when GetScripts returned 403/404 (token lacks permission) + + // Settings holds the raw team object as returned by the API, so the + // settings blocks a team YAML configures (webhook_settings, + // host_expiry_settings, integrations, features) can be diffed field by + // field. Populated by UnmarshalJSON. + Settings map[string]any `json:"-"` +} + +// UnmarshalJSON decodes a team twice: once into the typed struct and once into +// a generic map kept in Settings. The map is what the settings diff compares +// against, and keeping it avoids having to model every settings sub-key Fleet +// may add. +func (t *Team) UnmarshalJSON(data []byte) error { + type teamAlias Team // avoid recursing into this method + var alias teamAlias + if err := json.Unmarshal(data, &alias); err != nil { + return err + } + *t = Team(alias) + // A team that does not decode as an object is a server-side surprise, not + // something to fail the whole diff over: leave Settings nil. + _ = json.Unmarshal(data, &t.Settings) + return nil } // TeamSoftware mirrors /api/v1/fleet/teams[].software for managed software diff --git a/internal/api/client_test.go b/internal/api/client_test.go index d8d3c75..6952b0f 100644 --- a/internal/api/client_test.go +++ b/internal/api/client_test.go @@ -160,6 +160,56 @@ func TestGetTeams(t *testing.T) { } } +func TestGetTeamsCapturesRawSettings(t *testing.T) { + // The settings blocks a team YAML configures live on the team object + // itself; the client keeps the raw object so they can be diffed. + const body = `{"teams":[{ + "id": 1, + "name": "Workstations", + "host_expiry_settings": {"host_expiry_enabled": true, "host_expiry_window": 30}, + "webhook_settings": {"failing_policies_webhook": {"destination_url": "https://example.com/hook"}}, + "features": {"enable_software_inventory": true} + }]}` + + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + fmt.Fprint(w, body) + })) + defer ts.Close() + + teams, err := testClient(t, ts, "testtoken").GetTeams(context.Background()) + if err != nil { + t.Fatalf("GetTeams: %v", err) + } + if len(teams) != 1 { + t.Fatalf("got %d teams, want 1", len(teams)) + } + + // The typed fields still decode. + if teams[0].ID != 1 || teams[0].Name != "Workstations" { + t.Errorf("typed fields: got id=%d name=%q", teams[0].ID, teams[0].Name) + } + + hes, ok := teams[0].Settings["host_expiry_settings"].(map[string]any) + if !ok { + t.Fatalf("Settings missing host_expiry_settings: %+v", teams[0].Settings) + } + if hes["host_expiry_enabled"] != true { + t.Errorf("host_expiry_enabled: got %v, want true", hes["host_expiry_enabled"]) + } + if _, ok := teams[0].Settings["features"]; !ok { + t.Errorf("Settings missing features: %+v", teams[0].Settings) + } +} + +func TestTeamUnmarshalJSONNonObject(t *testing.T) { + // A team that is not a JSON object should surface as an error rather than + // panicking or silently producing a half-decoded team. + var team Team + if err := json.Unmarshal([]byte(`"not-an-object"`), &team); err == nil { + t.Fatal("expected an error unmarshalling a non-object team") + } +} + // ---------- GetPolicies ---------- func TestGetPoliciesPathRouting(t *testing.T) { diff --git a/internal/diff/differ.go b/internal/diff/differ.go index 73bd469..a794bc9 100644 --- a/internal/diff/differ.go +++ b/internal/diff/differ.go @@ -302,6 +302,7 @@ func Diff(current *api.FleetState, proposed *parser.ParsedRepo, teamFilters []st result.Policies = diffPolicies(currentTeam.Policies, proposedTeam.Policies) result.Queries = diffQueries(currentTeam.Queries, proposedTeam.Queries) + result.Config, result.SkippedConfigSections = diffTeamSettings(currentTeam.Settings, proposedTeam.Settings) // enrichedSoftware holds the API software state with fleet-maintained // app scripts populated. Hoisted here so the baseline subtraction @@ -356,6 +357,7 @@ func Diff(current *api.FleetState, proposed *parser.ParsedRepo, teamFilters []st baseDiff := DiffResult{} baseDiff.Policies = diffPolicies(currentTeam.Policies, baseTeam.Policies) baseDiff.Queries = diffQueries(currentTeam.Queries, baseTeam.Queries) + baseDiff.Config, _ = diffTeamSettings(currentTeam.Settings, baseTeam.Settings) if !currentTeam.SoftwareUnavailable { baseDiff.Software = diffSoftware(enrichedSoftware, baseTeam.Software) } @@ -371,6 +373,7 @@ func Diff(current *api.FleetState, proposed *parser.ParsedRepo, teamFilters []st if cfg.verbose { vlog(true, "[%s] baseline queries: %s", proposedTeam.Name, rdNames(baseDiff.Queries)) } + result.Config = subtractConfigChanges(result.Config, baseDiff.Config) result.Policies = subtractResourceDiff(result.Policies, baseDiff.Policies) result.Queries = subtractResourceDiff(result.Queries, baseDiff.Queries) result.Software = subtractResourceDiff(result.Software, baseDiff.Software) @@ -1472,6 +1475,90 @@ func diffConfig(apiConfig map[string]any, proposed *parser.ParsedGlobal) ([]Conf return changes, skipped } +// teamSettingsSections maps a sub-key of a team's `settings:` block to the +// field on the Fleet team object that holds its live value. Everything Fleet +// exposes on GET /teams is listed here; a sub-key that is not is reported as +// skipped rather than silently dropped, so a new Fleet setting shows up as +// "not diffed" instead of "no changes". +var teamSettingsSections = map[string]string{ + "webhook_settings": "webhook_settings", + "host_expiry_settings": "host_expiry_settings", + "integrations": "integrations", + "features": "features", +} + +// diffTeamSettings compares a team's live settings (the raw team object from +// GET /teams) against the `settings:` block in its YAML, field by field. It +// returns the changes plus the names of settings sub-keys that could not be +// diffed. +// +// `secrets:` is never diffed: enroll secrets are credentials, and the diff +// output ends up in CI logs and MR comments. +func diffTeamSettings(current, proposed map[string]any) ([]ConfigChange, []string) { + if len(proposed) == 0 { + return nil, nil + } + + var changes []ConfigChange + var skipped []string + + for section, v := range proposed { + if section == "secrets" { + continue + } + apiKey, known := teamSettingsSections[section] + if !known { + skipped = append(skipped, "settings."+section) + continue + } + proposedMap, ok := v.(map[string]any) + if !ok { + skipped = append(skipped, "settings."+section) + continue + } + apiSection, ok := current[apiKey].(map[string]any) + if !ok { + // Fleet did not return this section (older server, or a token + // without permission to see it): say so rather than reporting + // every proposed key as a change. + skipped = append(skipped, "settings."+section) + continue + } + + // Same guards as diffConfig: skip env var placeholders Fleet + // substitutes, and skip keys the API does not expose a value for, + // since "" cannot be distinguished from "not reported". + flattenMap(proposedMap, section, func(key, proposedVal string) { + if containsEnvVar(proposedVal) || proposedVal == "" || proposedVal == "" { + return + } + apiVal := getNestedValue(apiSection, strings.TrimPrefix(key, section+".")) + if apiVal == "" || apiVal == "" { + return + } + compareAPI, compareProposed := apiVal, proposedVal + if looksLikeJSON(apiVal) && looksLikeJSON(proposedVal) { + compareAPI = normalizeJSON(apiVal) + compareProposed = normalizeJSON(proposedVal) + } + if compareAPI != compareProposed { + changes = append(changes, ConfigChange{ + Section: "settings", + Key: key, + Old: apiVal, + New: proposedVal, + }) + } + }) + } + + // flattenMap walks maps in random order; sort so output is stable. + sort.Slice(changes, func(i, j int) bool { return changes[i].Key < changes[j].Key }) + sort.Strings(skipped) + + return changes, skipped +} + // containsEnvVar returns true if the string contains a $ (env var placeholder). func containsEnvVar(s string) bool { return strings.Contains(s, "$") diff --git a/internal/diff/differ_test.go b/internal/diff/differ_test.go index e888e56..290dcb3 100644 --- a/internal/diff/differ_test.go +++ b/internal/diff/differ_test.go @@ -39,6 +39,21 @@ func TestDiffTestdataAgainstMockAPI(t *testing.T) { { ID: 1, Name: "Workstations", + // Live settings: host expiry off and the failing-policies + // webhook disabled, both of which the fixture turns on. + Settings: map[string]any{ + "host_expiry_settings": map[string]any{ + "host_expiry_enabled": false, + "host_expiry_window": float64(0), + }, + "webhook_settings": map[string]any{ + "failing_policies_webhook": map[string]any{ + "enable_failing_policies_webhook": false, + "host_batch_size": float64(0), + }, + }, + "features": map[string]any{"enable_software_inventory": true}, + }, Policies: []api.Policy{ // FileVault exists but with a different (simpler) query → modified {Name: "[macOS] FileVault Enabled", Query: "SELECT 1 FROM disk_encryption WHERE encrypted = 1;", Platform: "darwin", Critical: true}, @@ -2355,3 +2370,243 @@ func TestSubtractConfigChanges(t *testing.T) { }) } } + +func TestDiffTeamSettings(t *testing.T) { + // A trimmed-down version of what GET /teams returns for a team. + liveTeam := func() map[string]any { + return map[string]any{ + "host_expiry_settings": map[string]any{ + "host_expiry_enabled": false, + "host_expiry_window": 0, + }, + "webhook_settings": map[string]any{ + "failing_policies_webhook": map[string]any{ + "enable_failing_policies_webhook": false, + "destination_url": "https://old.example.com/hook", + "host_batch_size": 0, + }, + }, + "features": map[string]any{"enable_software_inventory": true}, + "integrations": map[string]any{ + "google_calendar": map[string]any{"enable_calendar_events": false}, + }, + } + } + + tests := []struct { + name string + proposed map[string]any + wantChanges []ConfigChange + wantSkipped []string + }{ + { + name: "no settings block", + proposed: nil, + }, + { + name: "matching values produce no diff", + proposed: map[string]any{ + "features": map[string]any{"enable_software_inventory": true}, + }, + }, + { + name: "changed nested value", + proposed: map[string]any{ + "host_expiry_settings": map[string]any{ + "host_expiry_enabled": true, + "host_expiry_window": 30, + }, + }, + wantChanges: []ConfigChange{ + {Section: "settings", Key: "host_expiry_settings.host_expiry_enabled", Old: "false", New: "true"}, + {Section: "settings", Key: "host_expiry_settings.host_expiry_window", Old: "0", New: "30"}, + }, + }, + { + name: "deeply nested webhook value", + proposed: map[string]any{ + "webhook_settings": map[string]any{ + "failing_policies_webhook": map[string]any{ + "destination_url": "https://new.example.com/hook", + }, + }, + }, + wantChanges: []ConfigChange{ + { + Section: "settings", + Key: "webhook_settings.failing_policies_webhook.destination_url", + Old: "https://old.example.com/hook", + New: "https://new.example.com/hook", + }, + }, + }, + { + // Enroll secrets are credentials; they must never reach the diff, + // which lands in CI logs and MR comments. + name: "secrets are never diffed", + proposed: map[string]any{ + "secrets": []any{map[string]any{"secret": "literal-not-a-placeholder"}}, + }, + }, + { + // Fleet substitutes $VARS server-side, so the YAML value and the + // live value never match and comparing them is noise. + name: "env var placeholders are skipped", + proposed: map[string]any{ + "webhook_settings": map[string]any{ + "failing_policies_webhook": map[string]any{ + "destination_url": "$WEBHOOK_URL", + }, + }, + }, + }, + { + name: "unknown settings sub-key is reported as skipped", + proposed: map[string]any{ + "future_settings": map[string]any{"some_key": "value"}, + }, + wantSkipped: []string{"settings.future_settings"}, + }, + { + name: "sub-key absent from the API is reported as skipped", + proposed: map[string]any{ + "mdm": map[string]any{"enable_disk_encryption": true}, + }, + wantSkipped: []string{"settings.mdm"}, + }, + { + name: "non-map sub-key is reported as skipped", + proposed: map[string]any{ + "features": "not-a-map", + }, + wantSkipped: []string{"settings.features"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + changes, skipped := diffTeamSettings(liveTeam(), tt.proposed) + + if len(changes) != len(tt.wantChanges) { + t.Fatalf("changes: got %d %+v, want %d %+v", + len(changes), changes, len(tt.wantChanges), tt.wantChanges) + } + for i := range changes { + if changes[i] != tt.wantChanges[i] { + t.Errorf("change %d:\n got %+v\nwant %+v", i, changes[i], tt.wantChanges[i]) + } + } + if strings.Join(skipped, ",") != strings.Join(tt.wantSkipped, ",") { + t.Errorf("skipped: got %v, want %v", skipped, tt.wantSkipped) + } + }) + } +} + +func TestDiffTeamSettingsMissingAPISettings(t *testing.T) { + // A team object with no settings at all (older Fleet, or a token that + // cannot see them) must not report every proposed key as a change. + changes, skipped := diffTeamSettings(nil, map[string]any{ + "features": map[string]any{"enable_software_inventory": false}, + }) + if len(changes) != 0 { + t.Errorf("changes: got %+v, want none", changes) + } + if len(skipped) != 1 || skipped[0] != "settings.features" { + t.Errorf("skipped: got %v, want [settings.features]", skipped) + } +} + +func TestDiffTeamSettingsOrderIsStable(t *testing.T) { + current := map[string]any{ + "features": map[string]any{"a": "1", "b": "2", "c": "3"}, + "host_expiry_settings": map[string]any{"host_expiry_window": 0}, + } + proposed := map[string]any{ + "features": map[string]any{"a": "x", "b": "y", "c": "z"}, + "host_expiry_settings": map[string]any{"host_expiry_window": 30}, + } + + // flattenMap walks maps in random order, so run repeatedly. + var first []string + for i := 0; i < 20; i++ { + changes, _ := diffTeamSettings(current, proposed) + var keys []string + for _, c := range changes { + keys = append(keys, c.Key) + } + if i == 0 { + first = keys + continue + } + if strings.Join(keys, ",") != strings.Join(first, ",") { + t.Fatalf("unstable order: got %v, first run %v", keys, first) + } + } + want := "features.a,features.b,features.c,host_expiry_settings.host_expiry_window" + if strings.Join(first, ",") != want { + t.Errorf("keys: got %v, want %s", first, want) + } +} + +// TestDiffTestdataTeamSettings checks that the `settings:` block in the shared +// fixture is diffed field by field against the team's live settings. +func TestDiffTestdataTeamSettings(t *testing.T) { + root := testutil.TestdataRoot(t) + + proposed, err := parser.ParseRepo(root, nil, "") + if err != nil { + t.Fatalf("ParseRepo: %v", err) + } + + current := &api.FleetState{ + Teams: []api.Team{{ + ID: 1, + Name: "Workstations", + Settings: map[string]any{ + "host_expiry_settings": map[string]any{ + "host_expiry_enabled": false, + "host_expiry_window": float64(0), + }, + "webhook_settings": map[string]any{ + "failing_policies_webhook": map[string]any{ + "enable_failing_policies_webhook": false, + "host_batch_size": float64(0), + }, + }, + "features": map[string]any{"enable_software_inventory": true}, + }, + }}, + } + + results := Diff(current, proposed, []string{"Workstations"}, nil) + if len(results) != 1 { + t.Fatalf("got %d results, want 1", len(results)) + } + + got := make(map[string]string, len(results[0].Config)) + for _, c := range results[0].Config { + if c.Section != "settings" { + t.Errorf("section: got %q, want settings", c.Section) + } + got[c.Key] = c.Old + " → " + c.New + } + + want := map[string]string{ + "host_expiry_settings.host_expiry_enabled": "false → true", + "host_expiry_settings.host_expiry_window": "0 → 30", + "webhook_settings.failing_policies_webhook.enable_failing_policies_webhook": "false → true", + "webhook_settings.failing_policies_webhook.host_batch_size": "0 → 100", + } + for k, v := range want { + if got[k] != v { + t.Errorf("%s: got %q, want %q", k, got[k], v) + } + } + // features matches on both sides, and secrets must never appear. + for k := range got { + if strings.HasPrefix(k, "features") || strings.HasPrefix(k, "secrets") { + t.Errorf("unexpected change reported for %q", k) + } + } +} diff --git a/internal/output/terminal.go b/internal/output/terminal.go index dda534a..50018d6 100644 --- a/internal/output/terminal.go +++ b/internal/output/terminal.go @@ -85,7 +85,8 @@ func RenderDiffTerminal(results []diff.DiffResult, verbose bool) string { func renderTeamDiff(result diff.DiffResult, summary *DiffSummary, verbose bool) string { var lines []string - // Config changes (global scope only) + // Config changes: org_settings/agent_options/controls for the global + // scope, `settings:` for a team. if len(result.Config) > 0 { lines = append(lines, renderConfigChanges(result.Config, summary, verbose)) } diff --git a/internal/parser/parser.go b/internal/parser/parser.go index 1775174..3b7d4e0 100644 --- a/internal/parser/parser.go +++ b/internal/parser/parser.go @@ -108,7 +108,12 @@ type ParsedGlobal struct { // ParsedTeam represents a single team's configuration. type ParsedTeam struct { - Name string + Name string + // Settings holds the team's `settings:` block (or the older + // `team_settings:` spelling) as a nested map: webhook_settings, + // host_expiry_settings, integrations, features. Diffed field by field + // against the matching keys on the Fleet team object. + Settings map[string]any Policies []ParsedPolicy Queries []ParsedQuery Software ParsedSoftware @@ -220,7 +225,11 @@ func (e ParseError) Error() string { // ---------- Team YAML raw types (for initial parsing) ---------- type rawTeamFile struct { - Name string `yaml:"name"` + Name string `yaml:"name"` + // Settings is the modern spelling of team_settings. fleetctl gitops + // accepts both; when both are present the newer key wins, matching how + // Fleet resolves them. + Settings yaml.Node `yaml:"settings"` TeamSettings yaml.Node `yaml:"team_settings"` OrgSettings yaml.Node `yaml:"org_settings"` AgentOptions yaml.Node `yaml:"agent_options"` @@ -317,6 +326,26 @@ func MatchesAnyTeam(name string, filters []string) bool { return false } +// decodeSettingsNode decodes the first non-empty settings node into a nested +// map. Callers pass `settings:` before `team_settings:` so the modern spelling +// wins when a file carries both. Returns nil when neither is set or the node +// is not a mapping, which callers treat as "nothing to diff". +func decodeSettingsNode(nodes ...yaml.Node) map[string]any { + for _, n := range nodes { + if n.IsZero() { + continue + } + var m map[string]any + if err := n.Decode(&m); err != nil { + continue + } + if len(m) > 0 { + return m + } + } + return nil +} + // IsNoTeam reports whether a parsed team file describes Fleet's special // "hosts not assigned to any team" bucket rather than a real team. // @@ -432,6 +461,7 @@ func parseTeamFile(root, path string) (*ParsedTeam, []ParseError) { team := &ParsedTeam{ Name: raw.Name, + Settings: decodeSettingsNode(raw.Settings, raw.TeamSettings), SourceFile: path, } diff --git a/internal/parser/parser_test.go b/internal/parser/parser_test.go index b6b3f3b..818ad33 100644 --- a/internal/parser/parser_test.go +++ b/internal/parser/parser_test.go @@ -3,6 +3,7 @@ package parser import ( "os" "path/filepath" + "reflect" "strings" "testing" @@ -1110,3 +1111,97 @@ func TestIsNoTeam(t *testing.T) { }) } } + +func TestParseTeamSettings(t *testing.T) { + tests := []struct { + name string + yaml string + want map[string]any + wantNil bool + }{ + { + name: "modern settings key", + yaml: `name: T +settings: + host_expiry_settings: + host_expiry_enabled: true + host_expiry_window: 30 +`, + want: map[string]any{ + "host_expiry_settings": map[string]any{ + "host_expiry_enabled": true, + "host_expiry_window": 30, + }, + }, + }, + { + name: "legacy team_settings key", + yaml: `name: T +team_settings: + features: + enable_software_inventory: true +`, + want: map[string]any{ + "features": map[string]any{"enable_software_inventory": true}, + }, + }, + { + // fleetctl gitops accepts both spellings; the modern one wins. + name: "settings wins over team_settings", + yaml: `name: T +settings: + features: + enable_software_inventory: true +team_settings: + features: + enable_software_inventory: false +`, + want: map[string]any{ + "features": map[string]any{"enable_software_inventory": true}, + }, + }, + { + name: "neither key present", + yaml: "name: T\n", + wantNil: true, + }, + { + name: "empty settings block", + yaml: "name: T\nsettings: {}\n", + wantNil: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + root := t.TempDir() + teamsDir := filepath.Join(root, "teams") + if err := os.MkdirAll(teamsDir, 0o755); err != nil { + t.Fatal(err) + } + path := filepath.Join(teamsDir, "t.yml") + if err := os.WriteFile(path, []byte(tt.yaml), 0o600); err != nil { + t.Fatal(err) + } + + repo, err := ParseRepo(root, nil, "") + if err != nil { + t.Fatalf("ParseRepo: %v", err) + } + if len(repo.Teams) != 1 { + t.Fatalf("got %d teams, want 1 (errors: %v)", len(repo.Teams), repo.Errors) + } + + got := repo.Teams[0].Settings + if tt.wantNil { + if got != nil { + t.Fatalf("Settings: got %v, want nil", got) + } + return + } + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("Settings:\n got %#v\nwant %#v", got, tt.want) + } + }) + } +} diff --git a/testdata/teams/workstations.yml b/testdata/teams/workstations.yml index 003fbd3..62193a4 100644 --- a/testdata/teams/workstations.yml +++ b/testdata/teams/workstations.yml @@ -1,7 +1,16 @@ name: Workstations -team_settings: +settings: secrets: - secret: "$ENROLL_SECRET_WORKSTATIONS" + host_expiry_settings: + host_expiry_enabled: true + host_expiry_window: 30 + webhook_settings: + failing_policies_webhook: + enable_failing_policies_webhook: true + host_batch_size: 100 + features: + enable_software_inventory: true agent_options: config: options: From 74beae95a4ada1c7f79c08c69cac917890e26497 Mon Sep 17 00:00:00 2001 From: Robbie Trencheny Date: Tue, 18 Aug 2026 15:30:24 -0400 Subject: [PATCH 2/3] test(diff,parser): cover the settings-diff branches codecov flagged Three uncovered paths from the patch report: - a settings key the API reports no value for, which must not be guessed at - the JSON normalization path taken when both sides serialize as lists, which also documents that element order counts, matching the global config diff - a `settings:` node that is not a mapping, where the parser falls through to the legacy `team_settings:` key rather than failing the file Patch coverage for this branch is now 100%. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01KEpzMNJnGaBLAfrPeqknCy --- internal/diff/differ_test.go | 44 ++++++++++++++++++++++++++++++++++ internal/parser/parser_test.go | 14 +++++++++++ 2 files changed, 58 insertions(+) diff --git a/internal/diff/differ_test.go b/internal/diff/differ_test.go index 290dcb3..d00c26e 100644 --- a/internal/diff/differ_test.go +++ b/internal/diff/differ_test.go @@ -2389,6 +2389,7 @@ func TestDiffTeamSettings(t *testing.T) { "features": map[string]any{"enable_software_inventory": true}, "integrations": map[string]any{ "google_calendar": map[string]any{"enable_calendar_events": false}, + "allow_list": []any{"a", "b"}, }, } } @@ -2460,6 +2461,49 @@ func TestDiffTeamSettings(t *testing.T) { }, }, }, + { + // The API does not report a value for this key, so "" cannot be + // told apart from "Fleet has no opinion" -- reporting it would be + // a guess. + name: "key absent from the API section is not reported", + proposed: map[string]any{ + "features": map[string]any{"enable_future_thing": true}, + }, + }, + { + // List values are compared as serialized JSON, element order + // included -- the same rule the global config diff uses. + name: "reordered JSON list is reported as a change", + proposed: map[string]any{ + "integrations": map[string]any{ + "allow_list": []any{"b", "a"}, + }, + }, + wantChanges: []ConfigChange{ + { + Section: "settings", + Key: "integrations.allow_list", + Old: `["a","b"]`, + New: `["b","a"]`, + }, + }, + }, + { + name: "differing JSON lists are a change", + proposed: map[string]any{ + "integrations": map[string]any{ + "allow_list": []any{"a", "c"}, + }, + }, + wantChanges: []ConfigChange{ + { + Section: "settings", + Key: "integrations.allow_list", + Old: `["a","b"]`, + New: `["a","c"]`, + }, + }, + }, { name: "unknown settings sub-key is reported as skipped", proposed: map[string]any{ diff --git a/internal/parser/parser_test.go b/internal/parser/parser_test.go index 818ad33..1eca53d 100644 --- a/internal/parser/parser_test.go +++ b/internal/parser/parser_test.go @@ -1165,6 +1165,20 @@ team_settings: yaml: "name: T\n", wantNil: true, }, + { + // A scalar where a mapping belongs cannot be decoded; the parser + // moves on to the legacy key rather than failing the file. + name: "non-mapping settings falls through to team_settings", + yaml: `name: T +settings: "not a mapping" +team_settings: + features: + enable_software_inventory: true +`, + want: map[string]any{ + "features": map[string]any{"enable_software_inventory": true}, + }, + }, { name: "empty settings block", yaml: "name: T\nsettings: {}\n", From 0be1dc990d25b91a4ef65bfce98aed28684289f2 Mon Sep 17 00:00:00 2001 From: Robbie Trencheny Date: Tue, 18 Aug 2026 15:36:11 -0400 Subject: [PATCH 3/3] fix(parser): keep settings precedence when the mapping is empty Per review on #55: an explicit `settings: {}` fell through to a populated `team_settings:`, contradicting the documented rule that the modern key wins. An empty mapping is a declaration ("this team configures no settings"), so it now wins and yields no settings to diff. A key present but null (`settings:` with no value) declares nothing, so the legacy key is still used in that case. Both are covered by tests. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01KEpzMNJnGaBLAfrPeqknCy --- internal/parser/parser.go | 23 +++++++++++++++++------ internal/parser/parser_test.go | 25 +++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 6 deletions(-) diff --git a/internal/parser/parser.go b/internal/parser/parser.go index 3b7d4e0..8aa3057 100644 --- a/internal/parser/parser.go +++ b/internal/parser/parser.go @@ -326,10 +326,15 @@ func MatchesAnyTeam(name string, filters []string) bool { return false } -// decodeSettingsNode decodes the first non-empty settings node into a nested -// map. Callers pass `settings:` before `team_settings:` so the modern spelling -// wins when a file carries both. Returns nil when neither is set or the node -// is not a mapping, which callers treat as "nothing to diff". +// decodeSettingsNode decodes the first settings node that declares a mapping. +// Callers pass `settings:` before `team_settings:` so the modern spelling wins +// when a file carries both -- including when it is written as an explicit +// empty mapping, which declares "this team configures no settings" and must +// not silently fall back to the legacy key. +// +// A key present but null (`settings:` with no value) declares nothing, so the +// next node is tried. Returns nil when no node declares a mapping, which +// callers treat as "nothing to diff". func decodeSettingsNode(nodes ...yaml.Node) map[string]any { for _, n := range nodes { if n.IsZero() { @@ -339,9 +344,15 @@ func decodeSettingsNode(nodes ...yaml.Node) map[string]any { if err := n.Decode(&m); err != nil { continue } - if len(m) > 0 { - return m + if m == nil { + continue + } + if len(m) == 0 { + // An explicit empty mapping declares "no settings": it wins over + // the legacy key, and there is nothing to diff. + return nil } + return m } return nil } diff --git a/internal/parser/parser_test.go b/internal/parser/parser_test.go index 1eca53d..d239918 100644 --- a/internal/parser/parser_test.go +++ b/internal/parser/parser_test.go @@ -1184,6 +1184,31 @@ team_settings: yaml: "name: T\nsettings: {}\n", wantNil: true, }, + { + // An explicit empty mapping is a declaration ("no settings"), so + // it wins over the legacy key just like a populated one would. + name: "empty settings beats team_settings", + yaml: `name: T +settings: {} +team_settings: + features: + enable_software_inventory: true +`, + wantNil: true, + }, + { + // A key with no value declares nothing, so the legacy key is used. + name: "null settings falls through to team_settings", + yaml: `name: T +settings: +team_settings: + features: + enable_software_inventory: true +`, + want: map[string]any{ + "features": map[string]any{"enable_software_inventory": true}, + }, + }, } for _, tt := range tests {