From aa72a421edfd79b20131d931deef6dc62581b4f9 Mon Sep 17 00:00:00 2001 From: Robbie Trencheny Date: Fri, 22 May 2026 21:18:18 -0400 Subject: [PATCH 1/4] feat: support both fleets/ and teams/ directory layouts Fleet upstream renamed the per-team YAML directory from teams/ to fleets/ in #40726 ("Migrating teams to fleets and queries to reports"). New repos generated by `fleetctl new` use fleets/. fleet-plan previously hard-coded teams/ everywhere, so it could not be used against current Fleet GitOps repos. Add an internal/teamdir package that resolves the directory at runtime, preferring fleets/ when both exist. Wire it into the parser, git scope detector, baseline checkout, and CLI error messages. Existing teams/ repos continue to work unchanged. Tests cover both layouts and the precedence rule. --- cmd/fleet-plan/main.go | 8 ++-- internal/git/baseline.go | 9 +++-- internal/git/scope.go | 29 +++++++++++---- internal/git/scope_test.go | 57 +++++++++++++++++++++++++++- internal/parser/parser.go | 13 ++++--- internal/parser/parser_test.go | 64 +++++++++++++++++++++++++++++++- internal/teamdir/teamdir.go | 49 ++++++++++++++++++++++++ internal/teamdir/teamdir_test.go | 60 ++++++++++++++++++++++++++++++ 8 files changed, 267 insertions(+), 22 deletions(-) create mode 100644 internal/teamdir/teamdir.go create mode 100644 internal/teamdir/teamdir_test.go diff --git a/cmd/fleet-plan/main.go b/cmd/fleet-plan/main.go index f79eac1..34123ff 100644 --- a/cmd/fleet-plan/main.go +++ b/cmd/fleet-plan/main.go @@ -17,10 +17,11 @@ import ( "github.com/TsekNet/fleet-plan/internal/api" "github.com/TsekNet/fleet-plan/internal/config" "github.com/TsekNet/fleet-plan/internal/diff" - "github.com/TsekNet/fleet-plan/internal/merge" "github.com/TsekNet/fleet-plan/internal/git" + "github.com/TsekNet/fleet-plan/internal/merge" "github.com/TsekNet/fleet-plan/internal/output" "github.com/TsekNet/fleet-plan/internal/parser" + "github.com/TsekNet/fleet-plan/internal/teamdir" ) // Set via -ldflags at build time. @@ -132,10 +133,11 @@ func runDiff(cmd *cobra.Command, _ []string) error { } if len(repo.Teams) == 0 && len(repo.Errors) == 0 { + dirName := teamdir.Resolve(flagRepo) if len(teams) > 0 { - return fmt.Errorf("no teams matching %v found in %s/teams/", teams, flagRepo) + return fmt.Errorf("no teams matching %v found in %s/%s/", teams, flagRepo, dirName) } - return fmt.Errorf("no teams found in %s/teams/\nAre you in a fleet-gitops repo? Try --repo /path/to/repo", flagRepo) + return fmt.Errorf("no teams found in %s/%s/\nAre you in a fleet-gitops repo? Try --repo /path/to/repo", flagRepo, dirName) } // Parse baseline (base branch) for subtraction when in --git mode. diff --git a/internal/git/baseline.go b/internal/git/baseline.go index 2a04b2e..c701e6b 100644 --- a/internal/git/baseline.go +++ b/internal/git/baseline.go @@ -6,6 +6,8 @@ import ( "os/exec" "path/filepath" "strings" + + "github.com/TsekNet/fleet-plan/internal/teamdir" ) // CheckoutBaseline extracts the base-branch versions of the given files into a @@ -26,8 +28,9 @@ func CheckoutBaseline(repoRoot string, baseRef string, files []string) (tmpRoot } cleanup = func() { os.RemoveAll(tmpRoot) } - // Ensure teams/ directory exists so ParseRepo doesn't bail early. - os.MkdirAll(filepath.Join(tmpRoot, "teams"), 0o755) + // Ensure the team directory exists so ParseRepo doesn't bail early. + // Mirror whichever name the source repo uses (fleets/ or teams/). + os.MkdirAll(filepath.Join(tmpRoot, teamdir.Resolve(repoRoot)), 0o755) // Resolve which files we need: the explicitly changed files, plus any // files they reference (path: directives in team YAML). We start with @@ -84,7 +87,7 @@ func collectBaselineFiles(repoRoot, baseRef string, changedFiles []string) []str // references. Also extract any referenced resource files so the parser can // resolve them. for _, f := range changedFiles { - if !strings.HasPrefix(f, "teams/") && f != "base.yml" && f != "default.yml" { + if !teamdir.HasPrefix(f) && f != "base.yml" && f != "default.yml" { continue } content, err := gitShow(repoRoot, baseRef, f) diff --git a/internal/git/scope.go b/internal/git/scope.go index 5cd2b13..b4d68dd 100644 --- a/internal/git/scope.go +++ b/internal/git/scope.go @@ -5,6 +5,7 @@ import ( "path/filepath" "strings" + "github.com/TsekNet/fleet-plan/internal/teamdir" "gopkg.in/yaml.v3" ) @@ -40,7 +41,7 @@ func ResolveScope(root string, changedFiles []string, envFile string) Scope { case f == "base.yml", f == envFile, strings.HasPrefix(f, "labels/") && !strings.HasSuffix(f, ".md"): scope.IncludeGlobal = true - case strings.HasPrefix(f, "teams/") && (strings.HasSuffix(f, ".yml") || strings.HasSuffix(f, ".yaml")): + case isTeamYAML(f): name := readTeamName(filepath.Join(root, f)) if name != "" && !teamsSeen[name] { teamsSeen[name] = true @@ -80,7 +81,16 @@ func isFleetResource(f string) bool { } func isFleetResourceOrTeam(f string) bool { - return isFleetResource(f) || (strings.HasPrefix(f, "teams/") && (strings.HasSuffix(f, ".yml") || strings.HasSuffix(f, ".yaml"))) + return isFleetResource(f) || isTeamYAML(f) +} + +// isTeamYAML reports whether a repo-relative path is a per-team YAML file +// under any recognized team directory (fleets/ or teams/). +func isTeamYAML(f string) bool { + if !strings.HasSuffix(f, ".yml") && !strings.HasSuffix(f, ".yaml") { + return false + } + return teamdir.HasPrefix(f) } // buildSearchPatterns returns the set of path strings to grep for in team YAMLs. @@ -103,12 +113,17 @@ func buildSearchPatterns(root, f string) []string { return patterns } -// teamsReferencingAny reads teams/*.yml and returns team names whose file -// content contains any of the given patterns (plain string search). +// teamsReferencingAny reads every team YAML in the repo (fleets/ or teams/) +// and returns team names whose file content contains any of the given patterns +// (plain string search). func teamsReferencingAny(root string, patterns []string) []string { - ymlMatches, _ := filepath.Glob(filepath.Join(root, "teams", "*.yml")) - yamlMatches, _ := filepath.Glob(filepath.Join(root, "teams", "*.yaml")) - matches := append(ymlMatches, yamlMatches...) + var matches []string + for _, dir := range teamdir.Names() { + for _, ext := range []string{"*.yml", "*.yaml"} { + m, _ := filepath.Glob(filepath.Join(root, dir, ext)) + matches = append(matches, m...) + } + } seen := map[string]bool{} var names []string for _, teamFile := range matches { diff --git a/internal/git/scope_test.go b/internal/git/scope_test.go index a0dd695..25a883d 100644 --- a/internal/git/scope_test.go +++ b/internal/git/scope_test.go @@ -7,10 +7,18 @@ import ( ) // writeTeamFile creates a team YAML file in root/teams/ with the given name -// and optional body content referencing resources. +// and optional body content referencing resources. The legacy teams/ name +// is kept here so existing test cases continue exercising backwards compat. func writeTeamFile(t *testing.T, root, filename, teamName, body string) { t.Helper() - dir := filepath.Join(root, "teams") + writeTeamFileIn(t, root, "teams", filename, teamName, body) +} + +// writeTeamFileIn is the same as writeTeamFile but lets the caller pick the +// directory name (used to exercise the canonical fleets/ layout). +func writeTeamFileIn(t *testing.T, root, dirName, filename, teamName, body string) { + t.Helper() + dir := filepath.Join(root, dirName) if err := os.MkdirAll(dir, 0o755); err != nil { t.Fatal(err) } @@ -130,6 +138,29 @@ func TestResolveScope(t *testing.T) { wantTeams: []string{"Workstations"}, wantTeamCount: 1, }, + { + name: "fleets/ team YAML change resolves team name", + setup: func(t *testing.T, root string) { + writeTeamFileIn(t, root, "fleets", "infra.yml", "Infrastructure", "") + }, + changedFiles: []string{"fleets/infra.yml"}, + wantGlobal: false, + wantTeams: []string{"Infrastructure"}, + wantChanged: []string{"fleets/infra.yml"}, + wantTeamCount: 1, + }, + { + name: "resource change finds referencing fleets/ team", + setup: func(t *testing.T, root string) { + writeTeamFileIn(t, root, "fleets", "alpha.yml", "Alpha", + "controls:\n scripts:\n - path: ../scripts/setup.ps1\n") + }, + changedFiles: []string{"scripts/setup.ps1"}, + wantGlobal: false, + wantTeams: []string{"Alpha"}, + wantChanged: []string{"scripts/setup.ps1"}, + wantTeamCount: 1, + }, } for _, tt := range tests { @@ -196,6 +227,28 @@ func TestIsFleetResource(t *testing.T) { } } +func TestIsTeamYAML(t *testing.T) { + t.Parallel() + cases := []struct { + path string + want bool + }{ + {"fleets/workstations.yml", true}, + {"teams/workstations.yml", true}, + {"fleets/infra.yaml", true}, + {"teams/infra.yaml", true}, + {"fleets/workstations.md", false}, + {"policies/disk.yml", false}, + {"fleets-extra/x.yml", false}, + {"", false}, + } + for _, c := range cases { + if got := isTeamYAML(c.path); got != c.want { + t.Errorf("isTeamYAML(%q) = %v, want %v", c.path, got, c.want) + } + } +} + func contains(ss []string, s string) bool { for _, v := range ss { if v == s { diff --git a/internal/parser/parser.go b/internal/parser/parser.go index 8d0a77f..e09ee09 100644 --- a/internal/parser/parser.go +++ b/internal/parser/parser.go @@ -10,6 +10,7 @@ import ( "path/filepath" "strings" + "github.com/TsekNet/fleet-plan/internal/teamdir" "gopkg.in/yaml.v3" ) @@ -284,17 +285,18 @@ func MatchesAnyTeam(name string, filters []string) bool { func ParseRepo(root string, teamFilters []string, defaultFile string) (*ParsedRepo, error) { repo := &ParsedRepo{} - teamsDir := filepath.Join(root, "teams") + teamsDirName := teamdir.Resolve(root) + teamsDir := filepath.Join(root, teamsDirName) entries, err := os.ReadDir(teamsDir) if err != nil { if os.IsNotExist(err) { repo.Errors = append(repo.Errors, ParseError{ File: teamsDir, - Message: "teams/ directory not found. Are you in a fleet-gitops repo?", + Message: fmt.Sprintf("%s/ directory not found. Are you in a fleet-gitops repo?", teamsDirName), }) return repo, nil } - return nil, fmt.Errorf("reading teams directory: %w", err) + return nil, fmt.Errorf("reading %s directory: %w", teamsDirName, err) } for _, entry := range entries { @@ -336,7 +338,8 @@ func ParseRepo(root string, teamFilters []string, defaultFile string) (*ParsedRe return repo, nil } -// parseTeamFile parses a single teams/*.yml file and resolves all path: refs. +// parseTeamFile parses a single team YAML file (under fleets/ or teams/) and +// resolves all path: refs. func parseTeamFile(root, path string) (*ParsedTeam, []ParseError) { var errs []ParseError @@ -661,7 +664,7 @@ func extractQueryFromYAML(data []byte) string { return strings.TrimSpace(string(data)) } -// NormalizeSoftwarePath canonicalizes teams/*.yml software package paths so +// NormalizeSoftwarePath canonicalizes team YAML software package paths so // they match Fleet API's software.packages[].referenced_yaml_path format. // Example: "../software/mac/slack/slack.yml" -> "software/mac/slack/slack.yml" func NormalizeSoftwarePath(p string) string { diff --git a/internal/parser/parser_test.go b/internal/parser/parser_test.go index d96883d..081e712 100644 --- a/internal/parser/parser_test.go +++ b/internal/parser/parser_test.go @@ -420,8 +420,8 @@ team_settings: {} }, { name: "missing teams directory", - wantErrMsg: "teams", - setup: func(root string) {}, // empty dir, no teams/ + wantErrMsg: "directory not found", + setup: func(root string) {}, // empty dir, no fleets/ or teams/ }, { name: "missing name field", @@ -626,3 +626,63 @@ team_settings: {} t.Fatalf("expected duplicate software package error, got: %+v", repo.Errors) } } + +// TestParseRepoFleetsDir verifies the new canonical directory name (fleets/) +// produced by Fleet's `fleetctl new` since the teams->fleets migration. +func TestParseRepoFleetsDir(t *testing.T) { + root := t.TempDir() + fleetsDir := filepath.Join(root, "fleets") + if err := os.MkdirAll(fleetsDir, 0o755); err != nil { + t.Fatal(err) + } + teamYAML := `name: Workstations +policies: [] +queries: [] +software: + packages: [] +team_settings: {} +` + if err := os.WriteFile(filepath.Join(fleetsDir, "workstations.yml"), []byte(teamYAML), 0o644); 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("expected 1 team from fleets/, got %d (errors: %v)", len(repo.Teams), repo.Errors) + } + if repo.Teams[0].Name != "Workstations" { + t.Errorf("team name = %q, want %q", repo.Teams[0].Name, "Workstations") + } +} + +// TestParseRepoFleetsTakesPriorityOverTeams verifies fleets/ wins when both +// directories exist (defensive behavior for repos mid-migration). +func TestParseRepoFleetsTakesPriorityOverTeams(t *testing.T) { + root := t.TempDir() + fleetsDir := filepath.Join(root, "fleets") + teamsDir := filepath.Join(root, "teams") + if err := os.MkdirAll(fleetsDir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(teamsDir, 0o755); err != nil { + t.Fatal(err) + } + write := func(dir, name, body string) { + if err := os.WriteFile(filepath.Join(dir, name), []byte(body), 0o644); err != nil { + t.Fatal(err) + } + } + write(fleetsDir, "ws.yml", "name: FromFleets\npolicies: []\nqueries: []\nsoftware:\n packages: []\nteam_settings: {}\n") + write(teamsDir, "ws.yml", "name: FromTeams\npolicies: []\nqueries: []\nsoftware:\n packages: []\nteam_settings: {}\n") + + repo, err := ParseRepo(root, nil, "") + if err != nil { + t.Fatalf("ParseRepo: %v", err) + } + if len(repo.Teams) != 1 || repo.Teams[0].Name != "FromFleets" { + t.Fatalf("expected single team named FromFleets, got %d teams: %+v", len(repo.Teams), repo.Teams) + } +} diff --git a/internal/teamdir/teamdir.go b/internal/teamdir/teamdir.go new file mode 100644 index 0000000..59f9db8 --- /dev/null +++ b/internal/teamdir/teamdir.go @@ -0,0 +1,49 @@ +// Package teamdir resolves the directory name that holds per-team YAML files +// inside a fleet-gitops repository. +// +// Fleet originally called these "teams" and stored them in teams/. Starting +// with Fleet PR #40726, the canonical name is "fleets" stored in fleets/. +// Both layouts remain valid: this package prefers fleets/, falls back to +// teams/, and reports which one a given repo uses. +package teamdir + +import ( + "os" + "path/filepath" + "strings" +) + +// Preferred is the name used when neither directory exists yet (e.g. when +// preparing a baseline scratch dir) or when both exist. +const Preferred = "fleets" + +// Legacy is the older directory name still accepted for backwards compatibility. +const Legacy = "teams" + +// Names returns the candidate directory names in priority order. Useful for +// callers that need to scan both layouts (e.g. git scope detection where the +// changed-file path's prefix is what's known, not the on-disk directory). +func Names() []string { return []string{Preferred, Legacy} } + +// Resolve returns the name of the team directory inside root, preferring +// fleets/ over teams/. If neither exists, Preferred is returned so callers +// have a sensible default for error messages and scratch-dir creation. +func Resolve(root string) string { + for _, name := range Names() { + if info, err := os.Stat(filepath.Join(root, name)); err == nil && info.IsDir() { + return name + } + } + return Preferred +} + +// HasPrefix reports whether the given repo-relative path lives under any +// recognized team directory (fleets/ or teams/). +func HasPrefix(path string) bool { + for _, name := range Names() { + if strings.HasPrefix(path, name+"/") { + return true + } + } + return false +} diff --git a/internal/teamdir/teamdir_test.go b/internal/teamdir/teamdir_test.go new file mode 100644 index 0000000..506e77d --- /dev/null +++ b/internal/teamdir/teamdir_test.go @@ -0,0 +1,60 @@ +package teamdir + +import ( + "os" + "path/filepath" + "testing" +) + +func TestResolvePrefersFleets(t *testing.T) { + t.Parallel() + root := t.TempDir() + if err := os.Mkdir(filepath.Join(root, "fleets"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.Mkdir(filepath.Join(root, "teams"), 0o755); err != nil { + t.Fatal(err) + } + if got := Resolve(root); got != "fleets" { + t.Errorf("Resolve = %q, want %q", got, "fleets") + } +} + +func TestResolveFallsBackToTeams(t *testing.T) { + t.Parallel() + root := t.TempDir() + if err := os.Mkdir(filepath.Join(root, "teams"), 0o755); err != nil { + t.Fatal(err) + } + if got := Resolve(root); got != "teams" { + t.Errorf("Resolve = %q, want %q", got, "teams") + } +} + +func TestResolveDefaultsToFleets(t *testing.T) { + t.Parallel() + root := t.TempDir() + if got := Resolve(root); got != "fleets" { + t.Errorf("Resolve = %q, want %q (default)", got, "fleets") + } +} + +func TestHasPrefix(t *testing.T) { + t.Parallel() + cases := []struct { + path string + want bool + }{ + {"fleets/workstations.yml", true}, + {"teams/workstations.yml", true}, + {"fleetstuff/x.yml", false}, + {"policies/disk.yml", false}, + {"", false}, + {"FLEETS/x.yml", false}, // case-sensitive + } + for _, c := range cases { + if got := HasPrefix(c.path); got != c.want { + t.Errorf("HasPrefix(%q) = %v, want %v", c.path, got, c.want) + } + } +} From c07e2124440d88c42ef52f388145d2f75759a510 Mon Sep 17 00:00:00 2001 From: Robbie Trencheny Date: Fri, 22 May 2026 22:01:43 -0400 Subject: [PATCH 2/4] feat(parser): support modern Fleet GitOps controls and top-level schema MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three additions to keep up with current fleetdm/fleet GitOps yaml: 1. controls.apple_settings.configuration_profiles — the unified replacement for controls.macos_settings.custom_settings. Entries may be .mobileconfig, .json (DDM declarations), or .xml. Platform is inferred from the lib/macos/ vs lib/ipados/ vs lib/ios/ path segment. Diff identity remains the embedded profile name. 2. Top-level `settings:` and `reports:` keys are now accepted (parsed as opaque maps). Field-level diffing of these sections isn't implemented yet, but the absence of an error keeps the rest of the diff trustworthy. 3. labels_include_any / labels_exclude_any / labels_include_all sibling keys on profile refs are now captured by the raw struct. Without these, fleet-plan against current Fleet GitOps repos parses profiles=0 for every team and reports every live Fleet profile as "REMOVED" — a false-positive that makes the PR comment dangerously misleading. Tests: TestParseRepoAppleSettings covers .mobileconfig + .json DDM + platform inference; TestParseRepoAcceptsSettingsAndReportsKeys covers the new top-level keys. --- internal/parser/parser.go | 64 ++++++++++++++++++- internal/parser/parser_test.go | 111 +++++++++++++++++++++++++++++++++ 2 files changed, 172 insertions(+), 3 deletions(-) diff --git a/internal/parser/parser.go b/internal/parser/parser.go index e09ee09..1e818c6 100644 --- a/internal/parser/parser.go +++ b/internal/parser/parser.go @@ -65,6 +65,13 @@ var ValidTopLevelKeys = map[string]bool{ "queries": true, "software": true, "labels": true, + // Newer Fleet schema additions accepted by `fleetctl gitops`. We parse + // these as opaque maps so they don't break validation; field-level diffing + // of these sections isn't implemented yet but the absence of an error + // keeps the rest of the diff (profiles, policies, queries, software) + // trustworthy. + "settings": true, + "reports": true, } // Valid label membership types (from server/fleet/labels.go). @@ -252,17 +259,28 @@ type rawSoftwarePackage struct { } type rawControls struct { - Scripts []rawPathRef `yaml:"scripts"` - MacOSSettings struct { + Scripts []rawPathRef `yaml:"scripts"` + MacOSSettings struct { CustomSettings []rawProfileRef `yaml:"custom_settings"` } `yaml:"macos_settings"` WindowsSettings struct { CustomSettings []rawProfileRef `yaml:"custom_settings"` } `yaml:"windows_settings"` + // AppleSettings is the modern unified Apple-platform block, replacing + // macos_settings.custom_settings. configuration_profiles entries can be + // .mobileconfig (macOS/iOS/iPadOS), .json (DDM declarations), or .xml. + // The platform is inferred from the file path (e.g. lib/ipados/...) and + // from file extension if no path hint is present. + AppleSettings struct { + ConfigurationProfiles []rawProfileRef `yaml:"configuration_profiles"` + } `yaml:"apple_settings"` } type rawProfileRef struct { - Path string `yaml:"path"` + Path string `yaml:"path"` + LabelsIncludeAny []string `yaml:"labels_include_any"` + LabelsExcludeAny []string `yaml:"labels_exclude_any"` + LabelsIncludeAll []string `yaml:"labels_include_all"` } // ---------- Parser ---------- @@ -496,9 +514,49 @@ func parseTeamFile(root, path string) (*ParsedTeam, []ParseError) { }) } + // apple_settings.configuration_profiles is the unified modern block that + // supersedes macos_settings.custom_settings. Entries may be .mobileconfig + // (macOS/iOS/iPadOS), .json (DDM declarations), or .xml. Diff identity is + // by name (PayloadDisplayName or filename for DDM); platform is informational. + for _, ref := range raw.Controls.AppleSettings.ConfigurationProfiles { + resolved := filepath.Join(dir, ref.Path) + if root != "" { + if err := safePath(root, resolved); err != nil { + errs = append(errs, ParseError{File: path, Message: err.Error()}) + continue + } + } + name := extractProfileName(resolved) + if name == "" { + name = profileNameFromFilename(resolved) + } + team.Profiles = append(team.Profiles, ParsedProfile{ + Path: resolved, + Name: name, + Platform: inferApplePlatform(ref.Path), + SourceFile: path, + }) + } + return team, errs } +// inferApplePlatform returns a coarse platform string based on the path +// segment. lib/ipados/* → ipados, lib/ios/* → ios, anything else → darwin +// (covers lib/macos/, lib/all/, lib/unassigned/, etc.). Fleet's diff identity +// is the embedded profile name, not platform — this is purely for display. +func inferApplePlatform(refPath string) string { + clean := strings.ToLower(filepath.ToSlash(filepath.Clean(refPath))) + switch { + case strings.Contains(clean, "/ipados/"): + return "ipados" + case strings.Contains(clean, "/ios/"): + return "ios" + default: + return "darwin" + } +} + // readYAMLRef resolves a path: reference, validates it stays within the repo // root, reads the file, and returns the raw bytes and resolved path. This is // the shared core of resolvePolicyRef, resolveQueryRef, and resolveSoftwareRef. diff --git a/internal/parser/parser_test.go b/internal/parser/parser_test.go index 081e712..1c5fd05 100644 --- a/internal/parser/parser_test.go +++ b/internal/parser/parser_test.go @@ -686,3 +686,114 @@ func TestParseRepoFleetsTakesPriorityOverTeams(t *testing.T) { t.Fatalf("expected single team named FromFleets, got %d teams: %+v", len(repo.Teams), repo.Teams) } } + +// TestParseRepoAppleSettings exercises controls.apple_settings.configuration_profiles, +// the modern unified block that replaced macos_settings.custom_settings. Each +// referenced file should produce a ParsedProfile with platform inferred from +// the path (macos/ → darwin, ipados/ → ipados, ios/ → ios). +func TestParseRepoAppleSettings(t *testing.T) { + root := t.TempDir() + fleetsDir := filepath.Join(root, "fleets") + macosDir := filepath.Join(root, "lib", "macos", "profiles") + ipadosDir := filepath.Join(root, "lib", "ipados", "profiles") + iosDir := filepath.Join(root, "lib", "ios", "profiles") + for _, d := range []string{fleetsDir, macosDir, ipadosDir, iosDir} { + if err := os.MkdirAll(d, 0o755); err != nil { + t.Fatal(err) + } + } + + // .mobileconfig with a PayloadDisplayName so extractProfileName succeeds. + mobileconfig := ` + + + + PayloadDisplayName + Mac Energy Saver + PayloadType + Configuration + + +` + if err := os.WriteFile(filepath.Join(macosDir, "energy.mobileconfig"), []byte(mobileconfig), 0o644); err != nil { + t.Fatal(err) + } + // DDM .json — name comes from filename. + if err := os.WriteFile(filepath.Join(ipadosDir, "ipad-force-os.json"), []byte(`{}`), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(iosDir, "ios-restrictions.json"), []byte(`{}`), 0o644); err != nil { + t.Fatal(err) + } + + teamYAML := `name: Mixed Apple +controls: + apple_settings: + configuration_profiles: + - path: ../lib/macos/profiles/energy.mobileconfig + labels_include_any: [mac-fleet] + - path: ../lib/ipados/profiles/ipad-force-os.json + labels_include_any: [ipad-fleet] + - path: ../lib/ios/profiles/ios-restrictions.json +team_settings: {} +` + if err := os.WriteFile(filepath.Join(fleetsDir, "mixed.yml"), []byte(teamYAML), 0o644); err != nil { + t.Fatal(err) + } + + repo, err := ParseRepo(root, nil, "") + if err != nil { + t.Fatalf("ParseRepo: %v", err) + } + if len(repo.Errors) > 0 { + t.Fatalf("unexpected errors: %v", repo.Errors) + } + if len(repo.Teams) != 1 { + t.Fatalf("expected 1 team, got %d", len(repo.Teams)) + } + got := map[string]string{} + for _, p := range repo.Teams[0].Profiles { + got[p.Name] = p.Platform + } + want := map[string]string{ + "Mac Energy Saver": "darwin", + "ipad-force-os": "ipados", + "ios-restrictions": "ios", + } + for name, plat := range want { + if got[name] != plat { + t.Errorf("profile %q: platform = %q, want %q (full map: %v)", name, got[name], plat, got) + } + } +} + +// TestParseRepoAcceptsSettingsAndReportsKeys covers top-level `settings:` and +// `reports:` keys that current Fleet GitOps yaml includes but fleet-plan +// originally rejected as unknown. +func TestParseRepoAcceptsSettingsAndReportsKeys(t *testing.T) { + root := t.TempDir() + fleetsDir := filepath.Join(root, "fleets") + if err := os.MkdirAll(fleetsDir, 0o755); err != nil { + t.Fatal(err) + } + teamYAML := `name: T1 +team_settings: {} +settings: + host_expiry_settings: + host_expiry_enabled: false +reports: + - name: example +` + if err := os.WriteFile(filepath.Join(fleetsDir, "t1.yml"), []byte(teamYAML), 0o644); err != nil { + t.Fatal(err) + } + repo, err := ParseRepo(root, nil, "") + if err != nil { + t.Fatalf("ParseRepo: %v", err) + } + for _, e := range repo.Errors { + if strings.Contains(e.Message, "unknown top-level key") { + t.Errorf("unexpected unknown-key error: %v", e) + } + } +} From 4fc400ee36195ef10f4050e15eeaa7a2acbf3559 Mon Sep 17 00:00:00 2001 From: Robbie Trencheny Date: Fri, 22 May 2026 22:12:35 -0400 Subject: [PATCH 3/4] fix(parser): use dict-depth to find top-level PayloadDisplayName MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit extractMobileconfigName previously took the LAST PayloadDisplayName in the file, assuming the top-level one always appears after PayloadContent. That holds for plists emitted by ProfileCreator and similar tools, but Apple Configurator (and many hand-edited profiles) emit the top-level PayloadDisplayName BEFORE PayloadContent. With that ordering, the "last" occurrence is a nested payload's display name -- which is often a templated literal shared across many files. Concrete symptom: a fleet-gitops repo with 50+ per-location activation code profiles, each with a unique outer PayloadDisplayName like "Zoom Room Activation Code — University of Pennsylvania (Room2)" and two inner payloads named "Zoom Room Activation Code (Mac)". fleet-plan reported every profile as "Zoom Room Activation Code (Mac)", collapsing 50 distinct profiles into one duplicate name and producing a fake ADDED/REMOVED storm. Fix: stream through the XML with a nesting depth counter and only accept PayloadDisplayName at depth 1 (direct child of the root dict). Position within the file no longer matters. Existing tests for top-level-last ordering still pass; new test case covers the top-level-first ordering from real-world activation profiles. --- internal/parser/parser.go | 74 ++++++++++++++++++++++++++-------- internal/parser/parser_test.go | 33 ++++++++++++++- 2 files changed, 88 insertions(+), 19 deletions(-) diff --git a/internal/parser/parser.go b/internal/parser/parser.go index 1e818c6..a4608b7 100644 --- a/internal/parser/parser.go +++ b/internal/parser/parser.go @@ -888,33 +888,73 @@ func extractProfileName(filePath string) string { // PayloadDisplayName) and a top-level PayloadDisplayName. Fleet uses the // top-level one as the profile identity. // -// In standard plist layout, the top-level PayloadDisplayName appears AFTER -// the PayloadContent array, so it's the last occurrence in the file. +// The order of PayloadDisplayName relative to PayloadContent is not +// standardized — Apple Configurator emits it before the array, ProfileCreator +// after, hand-edited files vary. So we can't rely on first/last position; +// instead we track nesting depth and only accept PayloadDisplayName +// at depth 1 (direct child of the root dict; depth 0 is ). func extractMobileconfigName(data []byte) string { s := string(data) - needle := "PayloadDisplayName" - last := "" + const needle = "PayloadDisplayName" + + depth := 0 // nesting depth, starting at 0 (outside 's root ). + // dictOpen reports whether a tag at position i is an opening tag + // (not self-closing). + scanTags := func(seg string) { + i := 0 + for i < len(seg) { + lt := strings.IndexByte(seg[i:], '<') + if lt < 0 { + return + } + i += lt + if strings.HasPrefix(seg[i:], "") { + depth++ + i += len("") + continue + } + if strings.HasPrefix(seg[i:], "") { + // Self-closing — no children; depth unchanged. + i += len("") + continue + } + if strings.HasPrefix(seg[i:], "") { + depth-- + i += len("") + continue + } + // Skip past this tag. + gt := strings.IndexByte(seg[i:], '>') + if gt < 0 { + return + } + i += gt + 1 + } + } - // Find all occurrences and keep the last one — that's the top-level dict entry. - remaining := s + cursor := 0 for { - idx := strings.Index(remaining, needle) + idx := strings.Index(s[cursor:], needle) if idx < 0 { break } - after := remaining[idx+len(needle):] - after = strings.TrimSpace(after) - if strings.HasPrefix(after, "") { - after = after[len(""):] - end := strings.Index(after, "") - if end >= 0 { - last = strings.TrimSpace(after[:end]) + absIdx := cursor + idx + // Update depth based on every / between cursor and this match. + scanTags(s[cursor:absIdx]) + // Skip past the PayloadDisplayName tag itself. + cursor = absIdx + len(needle) + // At depth 1, this is the top-level dict's PayloadDisplayName. + if depth == 1 { + after := strings.TrimSpace(s[cursor:]) + if strings.HasPrefix(after, "") { + after = after[len(""):] + if end := strings.Index(after, ""); end >= 0 { + return strings.TrimSpace(after[:end]) + } } } - remaining = remaining[idx+len(needle):] } - - return last + return "" } diff --git a/internal/parser/parser_test.go b/internal/parser/parser_test.go index 1c5fd05..dd12200 100644 --- a/internal/parser/parser_test.go +++ b/internal/parser/parser_test.go @@ -260,8 +260,10 @@ func TestExtractProfileNameFallback(t *testing.T) { } // TestExtractMobileconfigName verifies PayloadDisplayName extraction from plist XML. -// Fleet uses the top-level PayloadDisplayName (the last occurrence in the file) -// as the profile identity, not the nested ones inside PayloadContent. +// Fleet uses the top-level PayloadDisplayName (the one at the root dict, depth 1) +// as the profile identity, not the nested ones inside PayloadContent. Position +// within the file is not meaningful — different profile generators emit the +// top-level PayloadDisplayName before or after PayloadContent. func TestExtractMobileconfigName(t *testing.T) { // Top-level PayloadDisplayName after PayloadContent array — should return // the top-level one (last occurrence), not the inner one. @@ -323,6 +325,33 @@ func TestExtractMobileconfigName(t *testing.T) { t.Errorf("expected %q, got %q", "Simple Profile", name) } + // Top-level PayloadDisplayName BEFORE PayloadContent — the order Apple + // Configurator emits and what most hand-edited profiles look like. + // Earlier extractor used the last occurrence and would have returned + // the deepest nested name; now we require depth=1. + topFirst := ` + + + PayloadDisplayName + Zoom Room Activation Code — University of Pennsylvania (Room2) + PayloadContent + + + PayloadDisplayName + Zoom Room Activation Code + + + PayloadDisplayName + Zoom Room Activation Code (Mac) + + + +` + name = extractMobileconfigName([]byte(topFirst)) + if name != "Zoom Room Activation Code — University of Pennsylvania (Room2)" { + t.Errorf("top-first ordering: expected the outer PayloadDisplayName, got %q", name) + } + // No PayloadDisplayName at all empty := ` From 32770f02de9e7e40da3d6176871ca2d0f5d9c136 Mon Sep 17 00:00:00 2001 From: Robbie Trencheny Date: Fri, 22 May 2026 22:25:34 -0400 Subject: [PATCH 4/4] feat(parser): treat reports: as an alias for queries: Fleet's fleetdm/fleet#40726 renamed two top-level GitOps keys: teams/ -> fleets/ AND queries: -> reports:. The teams/->fleets/ half was already handled here; the queries:->reports: half was previously accepted as opaque so it wouldn't error, but its path refs were silently dropped. Symptom: a fleets/zoom-rooms.yml that lists 2 queries under reports: parsed with 0 queries on the proposed side. diffQueries then flagged both live Fleet queries as REMOVED -- a false signal that looks like real drift. Fix: add rawTeamFile.Reports (and the matching field in the default.yml rawStruct) and append them to the same resolveQueryRef loop. Both keys remain valid; `fleetctl gitops` accepts either or both during the transition, so we do too. Tests: replaced TestParseRepoAcceptsSettingsAndReportsKeys (which only asserted no unknown-key error and used a bogus inline body) with two focused tests -- TestParseRepoAcceptsSettingsKey for the opaque settings: block, and TestParseRepoReportsAliasesQueries that verifies a query defined under reports: actually shows up in the team's Queries list. End-to-end verified against the real Fleet instance: a fleet-gitops repo where two queries are listed under reports: now diffs as "no changes" (matches live state) instead of "2 deleted". A PR that adds one profile shows exactly "1 added" -- the actual change. --- internal/parser/parser.go | 34 +++++++++++++-------- internal/parser/parser_test.go | 55 ++++++++++++++++++++++++++++++---- 2 files changed, 71 insertions(+), 18 deletions(-) diff --git a/internal/parser/parser.go b/internal/parser/parser.go index a4608b7..574c59c 100644 --- a/internal/parser/parser.go +++ b/internal/parser/parser.go @@ -65,11 +65,11 @@ var ValidTopLevelKeys = map[string]bool{ "queries": true, "software": true, "labels": true, - // Newer Fleet schema additions accepted by `fleetctl gitops`. We parse - // these as opaque maps so they don't break validation; field-level diffing - // of these sections isn't implemented yet but the absence of an error - // keeps the rest of the diff (profiles, policies, queries, software) - // trustworthy. + // Newer Fleet schema additions accepted by `fleetctl gitops`. + // `reports:` is the renamed `queries:` (fleetdm/fleet#40726); we parse + // it fully via rawTeamFile.Reports. `settings:` is currently accepted + // as opaque — field-level diffing isn't implemented but the absence of + // an error keeps the rest of the diff trustworthy. "settings": true, "reports": true, } @@ -219,8 +219,13 @@ type rawTeamFile struct { Controls rawControls `yaml:"controls"` Policies []rawPathRef `yaml:"policies"` Queries []rawPathRef `yaml:"queries"` - Software rawSoftwareBlock `yaml:"software"` - Labels []rawPathRef `yaml:"labels"` + // Reports is the renamed-in-fleetdm/fleet#40726 spelling of queries. + // `fleetctl gitops` accepts both; we treat them as equivalent here so + // repos that use the modern `reports:` keyword don't show every live + // Fleet query as REMOVED on the diff. + Reports []rawPathRef `yaml:"reports"` + Software rawSoftwareBlock `yaml:"software"` + Labels []rawPathRef `yaml:"labels"` } type rawPathRef struct { @@ -402,8 +407,10 @@ func parseTeamFile(root, path string) (*ParsedTeam, []ParseError) { team.Policies = append(team.Policies, policies...) } - // Resolve queries - for _, ref := range raw.Queries { + // Resolve queries. Both `queries:` and `reports:` are accepted — Fleet + // renamed the key in fleetdm/fleet#40726 but kept the old spelling + // working, so repos may use either or both during transition. + for _, ref := range append(append([]rawPathRef(nil), raw.Queries...), raw.Reports...) { queries, parseErrs := resolveQueryRef(root, dir, ref.Path, path) errs = append(errs, parseErrs...) team.Queries = append(team.Queries, queries...) @@ -760,11 +767,13 @@ func parseDefaultFile(root, path string) (*parsedDefault, []ParseError) { return nil, []ParseError{{File: path, Message: fmt.Sprintf("YAML parse error: %s", err)}} } - // Parse structured fields (labels, policies, queries path refs) + // Parse structured fields (labels, policies, queries path refs). + // `reports:` is the renamed-in-#40726 alias for `queries:`. var rawStruct struct { Labels []rawPathRef `yaml:"labels"` Policies []rawPathRef `yaml:"policies"` Queries []rawPathRef `yaml:"queries"` + Reports []rawPathRef `yaml:"reports"` } if err := yaml.Unmarshal(data, &rawStruct); err != nil { return nil, []ParseError{{File: path, Message: fmt.Sprintf("YAML parse error: %s", err)}} @@ -798,8 +807,9 @@ func parseDefaultFile(root, path string) (*parsedDefault, []ParseError) { global.Policies = append(global.Policies, policies...) } - // Resolve global queries - for _, ref := range rawStruct.Queries { + // Resolve global queries. Accept both `queries:` and the renamed + // `reports:` (fleetdm/fleet#40726); see rawTeamFile for context. + for _, ref := range append(append([]rawPathRef(nil), rawStruct.Queries...), rawStruct.Reports...) { queries, parseErrs := resolveQueryRef(root, dir, ref.Path, path) errs = append(errs, parseErrs...) global.Queries = append(global.Queries, queries...) diff --git a/internal/parser/parser_test.go b/internal/parser/parser_test.go index dd12200..9f5618d 100644 --- a/internal/parser/parser_test.go +++ b/internal/parser/parser_test.go @@ -796,10 +796,10 @@ team_settings: {} } } -// TestParseRepoAcceptsSettingsAndReportsKeys covers top-level `settings:` and -// `reports:` keys that current Fleet GitOps yaml includes but fleet-plan -// originally rejected as unknown. -func TestParseRepoAcceptsSettingsAndReportsKeys(t *testing.T) { +// TestParseRepoAcceptsSettingsKey covers the top-level `settings:` block, +// which current Fleet GitOps yaml includes but fleet-plan originally +// rejected as unknown. It is parsed opaquely (no field-level diff yet). +func TestParseRepoAcceptsSettingsKey(t *testing.T) { root := t.TempDir() fleetsDir := filepath.Join(root, "fleets") if err := os.MkdirAll(fleetsDir, 0o755); err != nil { @@ -810,8 +810,6 @@ team_settings: {} settings: host_expiry_settings: host_expiry_enabled: false -reports: - - name: example ` if err := os.WriteFile(filepath.Join(fleetsDir, "t1.yml"), []byte(teamYAML), 0o644); err != nil { t.Fatal(err) @@ -826,3 +824,48 @@ reports: } } } + +// TestParseRepoReportsAliasesQueries verifies that `reports:` (the renamed +// `queries:` from fleetdm/fleet#40726) is parsed as path refs and merged +// into the team's query list. Without this, queries defined under `reports:` +// don't appear on the proposed side and Fleet's live queries get reported +// as REMOVED. +func TestParseRepoReportsAliasesQueries(t *testing.T) { + root := t.TempDir() + fleetsDir := filepath.Join(root, "fleets") + queriesDir := filepath.Join(root, "lib", "queries") + for _, d := range []string{fleetsDir, queriesDir} { + if err := os.MkdirAll(d, 0o755); err != nil { + t.Fatal(err) + } + } + // A query file in the standard Fleet GitOps list-of-queries format. + queryYAML := `- name: My Report Query + query: SELECT 1; + platform: darwin +` + if err := os.WriteFile(filepath.Join(queriesDir, "rpt.yml"), []byte(queryYAML), 0o644); err != nil { + t.Fatal(err) + } + teamYAML := `name: T1 +team_settings: {} +reports: + - path: ../lib/queries/rpt.yml +` + if err := os.WriteFile(filepath.Join(fleetsDir, "t1.yml"), []byte(teamYAML), 0o644); err != nil { + t.Fatal(err) + } + repo, err := ParseRepo(root, nil, "") + if err != nil { + t.Fatalf("ParseRepo: %v", err) + } + if len(repo.Errors) != 0 { + t.Fatalf("unexpected errors: %v", repo.Errors) + } + if len(repo.Teams) != 1 { + t.Fatalf("expected 1 team, got %d", len(repo.Teams)) + } + if len(repo.Teams[0].Queries) != 1 || repo.Teams[0].Queries[0].Name != "My Report Query" { + t.Errorf("expected 1 query named %q from reports:, got %+v", "My Report Query", repo.Teams[0].Queries) + } +}