From 6b969c4b07592b81e08901faa8555108940701f3 Mon Sep 17 00:00:00 2001 From: Dan Tsekhanskiy Date: Tue, 24 Mar 2026 10:37:02 -0400 Subject: [PATCH] fix: Detect modified profile XML in changed-file filter Ref #30 --- internal/diff/differ.go | 54 ++++++++++++++-------- internal/diff/differ_test.go | 88 +++++++++++++++++++++++++++++++----- 2 files changed, 111 insertions(+), 31 deletions(-) diff --git a/internal/diff/differ.go b/internal/diff/differ.go index 47464b1..36819e7 100644 --- a/internal/diff/differ.go +++ b/internal/diff/differ.go @@ -23,15 +23,15 @@ var wsRE = regexp.MustCompile(`\s+`) // DiffResult holds the diff for a single team (or global scope). type DiffResult struct { - Team string // "(global)" for default.yml scope - Policies ResourceDiff - Queries ResourceDiff - Software ResourceDiff - Profiles ResourceDiff - Scripts ResourceDiff - Labels LabelValidation - Config []ConfigChange // org_settings, agent_options, controls diffs - Errors []string + Team string // "(global)" for default.yml scope + Policies ResourceDiff + Queries ResourceDiff + Software ResourceDiff + Profiles ResourceDiff + Scripts ResourceDiff + Labels LabelValidation + Config []ConfigChange // org_settings, agent_options, controls diffs + Errors []string SkippedConfigSections []string // config sections absent from API (e.g. "agent_options") } @@ -100,9 +100,9 @@ type ScriptEnricher interface { type DiffOption func(*diffOptions) type diffOptions struct { - enricher ScriptEnricher - baseline *parser.ParsedRepo - verbose bool + enricher ScriptEnricher + baseline *parser.ParsedRepo + verbose bool includeGlobal bool } @@ -299,7 +299,7 @@ func Diff(current *api.FleetState, proposed *parser.ParsedRepo, teamFilters []st result.Errors = append(result.Errors, "profiles diff skipped: API token lacks permission to read profiles") } else { var profileWarnings []string - result.Profiles, profileWarnings = diffProfiles(currentTeam.Profiles, proposedTeam.Profiles) + result.Profiles, profileWarnings = diffProfiles(currentTeam.Profiles, proposedTeam.Profiles, changedFiles) result.Errors = append(result.Errors, profileWarnings...) } @@ -329,7 +329,7 @@ func Diff(current *api.FleetState, proposed *parser.ParsedRepo, teamFilters []st baseDiff.Software = diffSoftware(enrichedSoftware, baseTeam.Software) } if !currentTeam.ProfilesUnavailable { - baseDiff.Profiles, _ = diffProfiles(currentTeam.Profiles, baseTeam.Profiles) + baseDiff.Profiles, _ = diffProfiles(currentTeam.Profiles, baseTeam.Profiles, nil) } if !currentTeam.ScriptsUnavailable { baseDiff.Scripts = diffScripts(currentTeam.Scripts, baseTeam.Scripts) @@ -421,6 +421,7 @@ func buildSourceMap(team parser.ParsedTeam) map[string][]string { for _, p := range team.Profiles { add(p.Name, p.SourceFile) add(p.Name, team.SourceFile) + add(p.Name, p.Path) } for _, s := range team.Scripts { add(s.Name, s.SourceFile) @@ -797,9 +798,9 @@ func diffSoftware(current api.TeamSoftware, proposed parser.ParsedSoftware) Reso } } for _, sc := range []struct { - name string - curVal string - newVal string + name string + curVal string + newVal string }{ {"install_script", cur.InstallScript, a.InstallScript}, {"uninstall_script", cur.UninstallScript, a.UninstallScript}, @@ -1080,7 +1081,7 @@ func normalizeFleetPlatform(platform string) string { } } -func diffProfiles(current []api.Profile, proposed []parser.ParsedProfile) (ResourceDiff, []string) { +func diffProfiles(current []api.Profile, proposed []parser.ParsedProfile, changedFiles []string) (ResourceDiff, []string) { var diff ResourceDiff var warnings []string @@ -1089,6 +1090,12 @@ func diffProfiles(current []api.Profile, proposed []parser.ParsedProfile) (Resou currentMap[p.Name] = p } + // Build a set of changed file paths for fast lookup. + changedSet := make(map[string]bool, len(changedFiles)) + for _, cf := range changedFiles { + changedSet[cf] = true + } + // Profile identity is determined by the name embedded in the file content // (e.g., PayloadDisplayName for .mobileconfig), which is what Fleet uses. // The parser extracts this into ParsedProfile.Name during parsing. @@ -1110,6 +1117,16 @@ func diffProfiles(current []api.Profile, proposed []parser.ParsedProfile) (Resou Name: name, Fields: fields, }) + } else if changedSet[p.Path] { + // The Fleet API does not return profile content, so we cannot + // compare payloads. When the profile XML file appears in the + // git changed-files list, treat it as modified. + diff.Modified = append(diff.Modified, ResourceChange{ + Name: name, + Fields: map[string]FieldDiff{ + "path": {New: p.Path}, + }, + }) } } @@ -1508,4 +1525,3 @@ func normalizeScript(s string) string { func normalizeWS(s string) string { return strings.TrimSpace(wsRE.ReplaceAllString(s, " ")) } - diff --git a/internal/diff/differ_test.go b/internal/diff/differ_test.go index fd8885b..487364d 100644 --- a/internal/diff/differ_test.go +++ b/internal/diff/differ_test.go @@ -266,10 +266,10 @@ func TestDiffPolicyScenarios(t *testing.T) { wantModified: 1, checkName: "Disk Encryption", checkFields: []string{"query", "critical"}, }, { - name: "policy deleted with hosts", - current: []api.Policy{{Name: "Keep", Platform: "darwin"}, {Name: "Delete This", Platform: "darwin", PassingHostCount: 50}}, - proposed: []parser.ParsedPolicy{{Name: "Keep", Platform: "darwin"}}, - wantDeleted: 1, checkName: "Delete This", checkWarning: true, + name: "policy deleted with hosts", + current: []api.Policy{{Name: "Keep", Platform: "darwin"}, {Name: "Delete This", Platform: "darwin", PassingHostCount: 50}}, + proposed: []parser.ParsedPolicy{{Name: "Keep", Platform: "darwin"}}, + wantDeleted: 1, checkName: "Delete This", checkWarning: true, }, { name: "no changes", @@ -1213,7 +1213,7 @@ func TestDiffProfilesMatchByContentName(t *testing.T) { {Path: "/repo/profiles/mac/wifi-corporate.mobileconfig", Name: "wifi-corporate", Platform: "darwin"}, } - diff, warnings := diffProfiles(current, proposed) + diff, warnings := diffProfiles(current, proposed, nil) if len(warnings) > 0 { t.Errorf("unexpected warnings: %v", warnings) } @@ -1236,7 +1236,7 @@ func TestDiffProfilesDetectsRealChanges(t *testing.T) { {Path: "/repo/profiles/mac/new-profile.mobileconfig", Name: "new-profile", Platform: "darwin"}, } - diff, _ := diffProfiles(current, proposed) + diff, _ := diffProfiles(current, proposed, nil) if len(diff.Added) != 1 || diff.Added[0].Name != "new-profile" { t.Errorf("expected 1 added profile 'new-profile', got %v", diff.Added) } @@ -1249,12 +1249,12 @@ func TestDiffProfilesDetectsRealChanges(t *testing.T) { func TestDiffGlobalConfig(t *testing.T) { tests := []struct { - name string - apiConfig map[string]any - proposedOrg map[string]any - wantChanges int - wantKey string // key to find in changes (empty = skip check) - wantKeyAbsent string // key that must NOT appear in changes + name string + apiConfig map[string]any + proposedOrg map[string]any + wantChanges int + wantKey string // key to find in changes (empty = skip check) + wantKeyAbsent string // key that must NOT appear in changes wantOld, wantNew string }{ { @@ -1882,6 +1882,70 @@ func assertChangesEqual(t *testing.T, label string, want, got []ResourceChange) } } +// TestDiffChangedFileFilterIncludesProfilePath verifies that modifying a profile +// XML file (not the team YAML) is detected by the changed-file filter. +func TestDiffChangedFileFilterIncludesProfilePath(t *testing.T) { + t.Parallel() + + current := &api.FleetState{ + Teams: []api.Team{{ + ID: 1, + Name: "T", + Profiles: []api.Profile{ + {ProfileUUID: "uuid-1", Name: "experience_windows", Platform: "windows"}, + {ProfileUUID: "uuid-2", Name: "newsandinterests_windows", Platform: "windows"}, + }, + }}, + } + + proposed := &parser.ParsedRepo{ + Teams: []parser.ParsedTeam{{ + Name: "T", + Profiles: []parser.ParsedProfile{ + { + Name: "experience_windows", + Path: "profiles/windows/experience_windows.xml", + Platform: "windows", + SourceFile: "/repo/teams/t.yml", + }, + { + Name: "newsandinterests_windows", + Path: "profiles/windows/newsandinterests_windows.xml", + Platform: "windows", + SourceFile: "/repo/teams/t.yml", + }, + }, + }}, + } + + // Only the profile XML changed, not the team YAML. + changedFiles := []string{ + "profiles/windows/experience_windows.xml", + } + + results := Diff(current, proposed, nil, changedFiles) + if len(results) != 1 { + t.Fatalf("expected 1 result, got %d", len(results)) + } + r := results[0] + + // experience_windows should appear (its XML path is in changedFiles). + if len(r.Profiles.Modified) != 1 { + t.Fatalf("expected 1 modified profile (XML path match), got %d modified, %d added, %d deleted", + len(r.Profiles.Modified), len(r.Profiles.Added), len(r.Profiles.Deleted)) + } + if r.Profiles.Modified[0].Name != "experience_windows" { + t.Errorf("expected experience_windows in modified, got %q", r.Profiles.Modified[0].Name) + } + + // newsandinterests_windows should be filtered out (its XML is not in changedFiles). + for _, m := range r.Profiles.Modified { + if m.Name == "newsandinterests_windows" { + t.Errorf("newsandinterests_windows should be filtered out, but found in modified") + } + } +} + // findTeam locates a DiffResult by team name, failing the test if not found. func findTeam(t *testing.T, results []DiffResult, name string) *DiffResult { t.Helper()