From 5d75cd15e1c7b30b055d734e6cce33fcb3007e21 Mon Sep 17 00:00:00 2001 From: Michael Beemer Date: Fri, 7 Nov 2025 14:17:53 -0500 Subject: [PATCH 1/6] feat: improve compare command Signed-off-by: Michael Beemer --- docs/commands/openfeature_compare.md | 7 +- internal/cmd/compare.go | 139 ++++++++- internal/cmd/compare_test.go | 51 +++ internal/manifest/compare.go | 285 ++++++++++++++++- internal/manifest/compare_test.go | 443 ++++++++++++++++++++++++++- 5 files changed, 894 insertions(+), 31 deletions(-) diff --git a/docs/commands/openfeature_compare.md b/docs/commands/openfeature_compare.md index 94969c0c..e27af86f 100644 --- a/docs/commands/openfeature_compare.md +++ b/docs/commands/openfeature_compare.md @@ -15,9 +15,10 @@ openfeature compare [flags] ### Options ``` - -a, --against string Path to the target manifest file to compare against - -h, --help help for compare - -o, --output string Output format. Valid formats: tree, flat, json, yaml (default "tree") + -a, --against string Path to the target manifest file to compare against + -h, --help help for compare + -i, --ignore stringArray Field pattern to ignore during comparison (can be specified multiple times). Supports shorthand (e.g., 'description') and full paths with wildcards (e.g., 'flags.*.description', 'metadata.*') + -o, --output string Output format. Valid formats: tree, flat, json, yaml (default "tree") ``` ### Options inherited from parent commands diff --git a/internal/cmd/compare.go b/internal/cmd/compare.go index 6f448f61..80c0e8af 100644 --- a/internal/cmd/compare.go +++ b/internal/cmd/compare.go @@ -4,6 +4,8 @@ import ( "encoding/json" "fmt" "os" + "reflect" + "sort" "strings" "github.com/open-feature/cli/internal/config" @@ -26,6 +28,7 @@ func GetCompareCmd() *cobra.Command { sourcePath := config.GetManifestPath(cmd) targetPath, _ := cmd.Flags().GetString("against") outputFormat, _ := cmd.Flags().GetString("output") + ignorePatterns, _ := cmd.Flags().GetStringArray("ignore") // Validate flags if sourcePath == "" || targetPath == "" { @@ -49,8 +52,10 @@ func GetCompareCmd() *cobra.Command { return fmt.Errorf("error loading target manifest: %w", err) } - // Compare manifests - changes, err := manifest.Compare(sourceManifest, targetManifest) + // Compare manifests with ignore patterns + changes, err := manifest.Compare(sourceManifest, targetManifest, manifest.CompareOptions{ + IgnorePatterns: ignorePatterns, + }) if err != nil { return fmt.Errorf("error comparing manifests: %w", err) } @@ -79,6 +84,9 @@ func GetCompareCmd() *cobra.Command { compareCmd.Flags().StringP("against", "a", "", "Path to the target manifest file to compare against") compareCmd.Flags().StringP("output", "o", string(manifest.OutputFormatTree), fmt.Sprintf("Output format. Valid formats: %s", strings.Join(manifest.GetValidOutputFormats(), ", "))) + compareCmd.Flags().StringArrayP("ignore", "i", []string{}, + "Field pattern to ignore during comparison (can be specified multiple times). "+ + "Supports shorthand (e.g., 'description') and full paths with wildcards (e.g., 'flags.*.description', 'metadata.*')") // Mark required flags _ = compareCmd.MarkFlagRequired("against") @@ -156,19 +164,20 @@ func renderTreeDiff(changes []manifest.Change, cmd *cobra.Command) error { flagName := strings.TrimPrefix(change.Path, "flags.") pterm.FgYellow.Printf(" ~ %s\n", flagName) - // Marshall the values - oldJSON, _ := json.MarshalIndent(change.OldValue, "", " ") - newJSON, _ := json.MarshalIndent(change.NewValue, "", " ") - - // Print the diff - fmt.Println(" Before:") - for _, line := range strings.Split(string(oldJSON), "\n") { - fmt.Printf(" %s\n", line) - } - - fmt.Println(" After:") - for _, line := range strings.Split(string(newJSON), "\n") { - fmt.Printf(" %s\n", line) + // Show field-level diff + fieldChanges := getFieldChanges(change.OldValue, change.NewValue) + if len(fieldChanges) > 0 { + for _, fc := range fieldChanges { + fmt.Printf(" • %s: %s → %s\n", fc.Field, fc.OldValue, fc.NewValue) + } + } else { + // Fallback to full object display if we can't parse + oldJSON, _ := json.MarshalIndent(change.OldValue, " ", " ") + newJSON, _ := json.MarshalIndent(change.NewValue, " ", " ") + fmt.Println(" Before:") + fmt.Printf(" %s\n", oldJSON) + fmt.Println(" After:") + fmt.Printf(" %s\n", newJSON) } } } @@ -176,6 +185,106 @@ func renderTreeDiff(changes []manifest.Change, cmd *cobra.Command) error { return nil } +// fieldChange represents a change to a specific field +type fieldChange struct { + Field string + OldValue string + NewValue string +} + +// getFieldChanges extracts field-level changes between two flag objects +func getFieldChanges(oldVal, newVal any) []fieldChange { + var changes []fieldChange + + // Convert to maps + oldMap, oldOk := oldVal.(map[string]any) + newMap, newOk := newVal.(map[string]any) + + if !oldOk || !newOk { + return changes // Return empty if not maps + } + + // Get all unique field names + allFields := make(map[string]bool) + for field := range oldMap { + allFields[field] = true + } + for field := range newMap { + allFields[field] = true + } + + // Compare each field + for field := range allFields { + oldFieldVal, oldExists := oldMap[field] + newFieldVal, newExists := newMap[field] + + // Field was added + if !oldExists && newExists { + changes = append(changes, fieldChange{ + Field: field, + OldValue: "(not set)", + NewValue: formatFieldValue(newFieldVal), + }) + continue + } + + // Field was removed + if oldExists && !newExists { + changes = append(changes, fieldChange{ + Field: field, + OldValue: formatFieldValue(oldFieldVal), + NewValue: "(removed)", + }) + continue + } + + // Field changed + if !reflect.DeepEqual(oldFieldVal, newFieldVal) { + changes = append(changes, fieldChange{ + Field: field, + OldValue: formatFieldValue(oldFieldVal), + NewValue: formatFieldValue(newFieldVal), + }) + } + } + + // Sort changes by field name for consistent output + sort.Slice(changes, func(i, j int) bool { + return changes[i].Field < changes[j].Field + }) + + return changes +} + +// formatFieldValue converts a value to a human-readable string for field-level diff display +func formatFieldValue(val any) string { + if val == nil { + return "null" + } + + switch v := val.(type) { + case string: + return fmt.Sprintf("%q", v) + case bool: + return fmt.Sprintf("%t", v) + case float64: + // Check if it's actually an integer + if v == float64(int64(v)) { + return fmt.Sprintf("%d", int64(v)) + } + return fmt.Sprintf("%g", v) + case map[string]any, []any: + // For complex types, marshal to compact JSON + jsonBytes, err := json.Marshal(v) + if err != nil { + return fmt.Sprintf("%v", v) + } + return string(jsonBytes) + default: + return fmt.Sprintf("%v", v) + } +} + // renderFlatDiff renders changes in a flat format func renderFlatDiff(changes []manifest.Change, cmd *cobra.Command) error { pterm.Info.Printf("Found %d difference(s) between manifests:\n\n", len(changes)) diff --git a/internal/cmd/compare_test.go b/internal/cmd/compare_test.go index 56093e5a..dc19c0b6 100644 --- a/internal/cmd/compare_test.go +++ b/internal/cmd/compare_test.go @@ -21,6 +21,10 @@ func TestGetCompareCmd(t *testing.T) { outputFlag := cmd.Flag("output") assert.NotNil(t, outputFlag) assert.Equal(t, "tree", outputFlag.DefValue) + + // Verify ignore flag + ignoreFlag := cmd.Flag("ignore") + assert.NotNil(t, ignoreFlag) } func TestCompareManifests(t *testing.T) { @@ -48,3 +52,50 @@ func TestCompareManifests(t *testing.T) { }) } } + +func TestCompareWithIgnoreFlag(t *testing.T) { + // Test that the ignore flag is properly parsed and passed to comparison + + t.Run("single_ignore_pattern", func(t *testing.T) { + rootCmd := GetRootCmd() + + rootCmd.SetArgs([]string{ + "compare", + "--manifest", "testdata/source_manifest.json", + "--against", "testdata/target_manifest.json", + "--ignore", "description", + }) + + err := rootCmd.Execute() + assert.NoError(t, err, "Command should execute with single ignore pattern") + }) + + t.Run("multiple_ignore_patterns", func(t *testing.T) { + rootCmd := GetRootCmd() + + rootCmd.SetArgs([]string{ + "compare", + "--manifest", "testdata/source_manifest.json", + "--against", "testdata/target_manifest.json", + "--ignore", "description", + "--ignore", "metadata.*", + }) + + err := rootCmd.Execute() + assert.NoError(t, err, "Command should execute with multiple ignore patterns") + }) + + t.Run("ignore_with_wildcard", func(t *testing.T) { + rootCmd := GetRootCmd() + + rootCmd.SetArgs([]string{ + "compare", + "--manifest", "testdata/source_manifest.json", + "--against", "testdata/target_manifest.json", + "--ignore", "flags.*.description", + }) + + err := rootCmd.Execute() + assert.NoError(t, err, "Command should execute with wildcard ignore pattern") + }) +} diff --git a/internal/manifest/compare.go b/internal/manifest/compare.go index a59ec138..15b453b3 100644 --- a/internal/manifest/compare.go +++ b/internal/manifest/compare.go @@ -2,7 +2,10 @@ package manifest import ( "fmt" + "maps" + "path/filepath" "reflect" + "strings" ) type Change struct { @@ -12,14 +15,22 @@ type Change struct { NewValue any `json:"newValue,omitempty"` } -func Compare(oldManifest, newManifest *Manifest) ([]Change, error) { +// CompareOptions holds options for comparing manifests +type CompareOptions struct { + IgnorePatterns []string +} + +// Compare compares two manifests and returns differences, optionally ignoring specified fields +func Compare(oldManifest, newManifest *Manifest, opts CompareOptions) ([]Change, error) { var changes []Change oldFlags := oldManifest.Flags newFlags := newManifest.Flags + // Check for changes and additions for key, newFlag := range newFlags { if oldFlag, exists := oldFlags[key]; exists { - if !reflect.DeepEqual(oldFlag, newFlag) { + // Compare flags semantically with ignore patterns + if flagHasChanges(oldFlag, newFlag, key, opts.IgnorePatterns) { changes = append(changes, Change{ Type: "change", Path: fmt.Sprintf("flags.%s", key), @@ -36,6 +47,7 @@ func Compare(oldManifest, newManifest *Manifest) ([]Change, error) { } } + // Check for removals for key, oldFlag := range oldFlags { if _, exists := newFlags[key]; !exists { changes = append(changes, Change{ @@ -48,3 +60,272 @@ func Compare(oldManifest, newManifest *Manifest) ([]Change, error) { return changes, nil } + +// getKnownFlagProperties returns the set of known schema properties for flags +// by extracting JSON field names from the BaseFlag struct +func getKnownFlagProperties() map[string]bool { + props := make(map[string]bool) + + // Extract fields from BaseFlag struct + t := reflect.TypeOf(BaseFlag{}) + for i := 0; i < t.NumField(); i++ { + field := t.Field(i) + jsonTag := field.Tag.Get("json") + if jsonTag != "" { + // Extract field name from json tag (e.g., "flagType,omitempty" -> "flagType") + fieldName := strings.Split(jsonTag, ",")[0] + if fieldName != "" && fieldName != "-" { + props[fieldName] = true + } + } + } + + // All flag types have defaultValue, but it's defined in the specific flag type structs + // (BooleanFlag, StringFlag, etc.), not in BaseFlag, so we add it manually here. + // This is necessary because each flag type has a different type for defaultValue + // (bool, string, int, float64, any), so it can't be in the base struct. + props["defaultValue"] = true + + return props +} + +// Cache the known properties (initialized once) +var knownFlagProperties = getKnownFlagProperties() + +// flagHasChanges checks if two flags have semantic differences, ignoring specified patterns +func flagHasChanges(oldFlag, newFlag any, flagKey string, ignorePatterns []string) bool { + // Flatten both flags to path->value maps + oldPaths := flattenToMap(oldFlag, fmt.Sprintf("flags.%s", flagKey)) + newPaths := flattenToMap(newFlag, fmt.Sprintf("flags.%s", flagKey)) + + // Get all unique paths + allPaths := make(map[string]bool) + for path := range oldPaths { + allPaths[path] = true + } + for path := range newPaths { + allPaths[path] = true + } + + // Check each path for differences (skipping ignored paths and unknown properties) + for path := range allPaths { + // Skip paths that should be ignored + if shouldIgnorePath(path, ignorePatterns) { + continue + } + + // Skip unknown properties (only compare known schema properties) + if !isKnownProperty(path, flagKey) { + continue + } + + oldVal, oldExists := oldPaths[path] + newVal, newExists := newPaths[path] + + // If existence differs or values differ, we have a change + if oldExists != newExists || !reflect.DeepEqual(oldVal, newVal) { + return true + } + } + + return false +} + +// isKnownProperty checks if a path represents a known schema property +func isKnownProperty(path, flagKey string) bool { + // Extract the property name from the path + // Path format: flags.. or flags... + prefix := fmt.Sprintf("flags.%s.", flagKey) + if !strings.HasPrefix(path, prefix) { + return false + } + + // Get the first property after the flag key + remainder := strings.TrimPrefix(path, prefix) + parts := strings.Split(remainder, ".") + if len(parts) == 0 { + return false + } + + propertyName := parts[0] + + // Check if it's a known property + return knownFlagProperties[propertyName] +} + +// flattenToMap recursively flattens a nested structure into a map of path->value +func flattenToMap(obj any, prefix string) map[string]any { + result := make(map[string]any) + + switch v := obj.(type) { + case map[string]any: + for key, value := range v { + path := fmt.Sprintf("%s.%s", prefix, key) + + // If the value is a simple type, add it directly + if isSimpleType(value) { + result[path] = value + } else { + // Otherwise, recursively flatten + nested := flattenToMap(value, path) + maps.Copy(result, nested) + } + } + case []any: + // For arrays, include them as-is at their path + result[prefix] = v + default: + // Simple value + result[prefix] = v + } + + return result +} + +// isSimpleType checks if a value is a simple type (not a map or struct) +func isSimpleType(v any) bool { + if v == nil { + return true + } + + switch v.(type) { + case map[string]any: + return false + case []any: + // Arrays are considered simple for comparison purposes + return true + default: + return true + } +} + +// shouldIgnorePath checks if a path should be ignored based on the ignore patterns +func shouldIgnorePath(path string, ignorePatterns []string) bool { + for _, pattern := range ignorePatterns { + if matchesPattern(path, pattern) { + return true + } + } + return false +} + +// matchesPattern checks if a path matches a given pattern +// Supports: +// - Full paths with globs: flags.*.description +// - Shorthand: description (matches **.description anywhere) +// - Partial wildcards: metadata.* (matches any path ending with metadata.) +func matchesPattern(path, pattern string) bool { + // Check for exact match first + if path == pattern { + return true + } + + // If pattern contains a dot, treat it as a path pattern + if strings.Contains(pattern, ".") { + // Try direct filepath.Match first + matched, err := filepath.Match(pattern, path) + if err == nil && matched { + return true + } + + // Try matching with wildcard support + if matchesWithDoublestar(path, pattern) { + return true + } + + // For patterns like "metadata.*", check if path ends with that pattern + // This allows "metadata.*" to match "flags.flagName.metadata.field" + if strings.Contains(pattern, "*") { + // Extract the suffix after the last non-wildcard part + // e.g., "metadata.*" should match any path containing "metadata." + if matchesPartialPath(path, pattern) { + return true + } + } + } else { + // Shorthand pattern - matches field anywhere in path + // E.g., "description" matches "flags.myFlag.description" + pathParts := strings.Split(path, ".") + for _, part := range pathParts { + if part == pattern { + return true + } + } + } + + return false +} + +// matchesPartialPath checks if a path matches a pattern with wildcards anywhere in the path +// E.g., "metadata.*" matches "flags.flagName.metadata.author" +func matchesPartialPath(path, pattern string) bool { + pathParts := strings.Split(path, ".") + patternParts := strings.Split(pattern, ".") + + // Try to find a matching subsequence in the path + for i := 0; i <= len(pathParts)-len(patternParts); i++ { + if matchesSubsequence(pathParts[i:], patternParts) { + return true + } + } + + return false +} + +// matchesSubsequence checks if a path subsequence matches a pattern +func matchesSubsequence(pathParts, patternParts []string) bool { + if len(pathParts) < len(patternParts) { + return false + } + + for i, patternPart := range patternParts { + if patternPart == "*" { + // Wildcard matches any single part + continue + } + if pathParts[i] != patternPart { + return false + } + } + + return true +} + +// matchesWithDoublestar handles patterns with * wildcards matching any segment +func matchesWithDoublestar(path, pattern string) bool { + pathParts := strings.Split(path, ".") + patternParts := strings.Split(pattern, ".") + + // If pattern has more parts than path, can't match + if len(patternParts) > len(pathParts) { + return false + } + + // Match each pattern part against corresponding path part + pathIdx := 0 + for patternIdx, patternPart := range patternParts { + switch patternPart { + case "*": + // Wildcard matches any single segment + pathIdx++ + case "**": + // Double wildcard matches any number of segments + // This is complex, so we'll keep it simple for now + return false + default: + // Exact match required + if pathIdx >= len(pathParts) || pathParts[pathIdx] != patternPart { + return false + } + pathIdx++ + } + + // If this is the last pattern part and it's *, and we've matched all path parts, success + if patternIdx == len(patternParts)-1 && patternPart == "*" && pathIdx == len(pathParts) { + return true + } + } + + // All pattern parts matched and we consumed all path parts + return pathIdx == len(pathParts) +} diff --git a/internal/manifest/compare_test.go b/internal/manifest/compare_test.go index 878f2c5b..3c512ba5 100644 --- a/internal/manifest/compare_test.go +++ b/internal/manifest/compare_test.go @@ -9,27 +9,51 @@ import ( func TestCompareDifferentManifests(t *testing.T) { oldManifest := &Manifest{ Flags: map[string]any{ - "flag1": "value1", - "flag2": "value2", + "flag1": map[string]any{ + "flagType": "string", + "defaultValue": "value1", + }, + "flag2": map[string]any{ + "flagType": "string", + "defaultValue": "value2", + }, }, } newManifest := &Manifest{ Flags: map[string]any{ - "flag1": "value1", - "flag2": "newValue2", - "flag3": "value3", + "flag1": map[string]any{ + "flagType": "string", + "defaultValue": "value1", + }, + "flag2": map[string]any{ + "flagType": "string", + "defaultValue": "newValue2", + }, + "flag3": map[string]any{ + "flagType": "string", + "defaultValue": "value3", + }, }, } - changes, err := Compare(oldManifest, newManifest) + changes, err := Compare(oldManifest, newManifest, CompareOptions{}) if err != nil { t.Fatalf("unexpected error: %v", err) } expectedChanges := []Change{ - {Type: "change", Path: "flags.flag2", OldValue: "value2", NewValue: "newValue2"}, - {Type: "add", Path: "flags.flag3", NewValue: "value3"}, + {Type: "change", Path: "flags.flag2", OldValue: map[string]any{ + "flagType": "string", + "defaultValue": "value2", + }, NewValue: map[string]any{ + "flagType": "string", + "defaultValue": "newValue2", + }}, + {Type: "add", Path: "flags.flag3", NewValue: map[string]any{ + "flagType": "string", + "defaultValue": "value3", + }}, } sortChanges(changes) @@ -43,12 +67,18 @@ func TestCompareDifferentManifests(t *testing.T) { func TestCompareIdenticalManifests(t *testing.T) { manifest := &Manifest{ Flags: map[string]any{ - "flag1": "value1", - "flag2": "value2", + "flag1": map[string]any{ + "flagType": "string", + "defaultValue": "value1", + }, + "flag2": map[string]any{ + "flagType": "boolean", + "defaultValue": true, + }, }, } - changes, err := Compare(manifest, manifest) + changes, err := Compare(manifest, manifest, CompareOptions{}) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -63,3 +93,394 @@ func sortChanges(changes []Change) { return changes[i].Path < changes[j].Path }) } + +// Test that property order differences don't trigger changes +func TestComparePropertyOrderDifferences(t *testing.T) { + oldManifest := &Manifest{ + Flags: map[string]any{ + "darkMode": map[string]any{ + "flagType": "boolean", + "defaultValue": false, + "description": "Enable dark mode", + }, + }, + } + + // Same content, different property order (maps don't have order in Go, but testing the logic) + newManifest := &Manifest{ + Flags: map[string]any{ + "darkMode": map[string]any{ + "description": "Enable dark mode", + "flagType": "boolean", + "defaultValue": false, + }, + }, + } + + changes, err := Compare(oldManifest, newManifest, CompareOptions{}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if len(changes) != 0 { + t.Errorf("expected no changes for property order differences, got %d changes: %v", len(changes), changes) + } +} + +// Test that extra properties are silently ignored +func TestCompareExtraProperties(t *testing.T) { + oldManifest := &Manifest{ + Flags: map[string]any{ + "darkMode": map[string]any{ + "flagType": "boolean", + "defaultValue": false, + }, + }, + } + + newManifest := &Manifest{ + Flags: map[string]any{ + "darkMode": map[string]any{ + "flagType": "boolean", + "defaultValue": false, + "extraProp": "value", + }, + }, + } + + changes, err := Compare(oldManifest, newManifest, CompareOptions{}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + // Extra properties should be silently ignored + if len(changes) != 0 { + t.Errorf("expected 0 changes (extra properties ignored), got %d: %v", len(changes), changes) + } +} + +// Test ignoring description with shorthand pattern +func TestCompareIgnoreDescriptionShorthand(t *testing.T) { + oldManifest := &Manifest{ + Flags: map[string]any{ + "darkMode": map[string]any{ + "flagType": "boolean", + "defaultValue": false, + "description": "Old description", + }, + }, + } + + newManifest := &Manifest{ + Flags: map[string]any{ + "darkMode": map[string]any{ + "flagType": "boolean", + "defaultValue": false, + "description": "New description", + }, + }, + } + + // Without ignore - should detect change + changes, err := Compare(oldManifest, newManifest, CompareOptions{}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(changes) != 1 { + t.Errorf("expected 1 change without ignore, got %d", len(changes)) + } + + // With ignore pattern - should not detect change + changes, err = Compare(oldManifest, newManifest, CompareOptions{ + IgnorePatterns: []string{"description"}, + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(changes) != 0 { + t.Errorf("expected 0 changes with ignore pattern, got %d: %v", len(changes), changes) + } +} + +// Test ignoring description with full path pattern +func TestCompareIgnoreDescriptionFullPath(t *testing.T) { + oldManifest := &Manifest{ + Flags: map[string]any{ + "darkMode": map[string]any{ + "flagType": "boolean", + "defaultValue": false, + "description": "Old description", + }, + "featureX": map[string]any{ + "flagType": "string", + "defaultValue": "value", + "description": "Old feature description", + }, + }, + } + + newManifest := &Manifest{ + Flags: map[string]any{ + "darkMode": map[string]any{ + "flagType": "boolean", + "defaultValue": false, + "description": "New description", + }, + "featureX": map[string]any{ + "flagType": "string", + "defaultValue": "value", + "description": "New feature description", + }, + }, + } + + // With full path pattern - should not detect changes + changes, err := Compare(oldManifest, newManifest, CompareOptions{ + IgnorePatterns: []string{"flags.*.description"}, + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(changes) != 0 { + t.Errorf("expected 0 changes with full path pattern, got %d: %v", len(changes), changes) + } +} + +// Test detecting functional changes +func TestCompareFunctionalChanges(t *testing.T) { + oldManifest := &Manifest{ + Flags: map[string]any{ + "darkMode": map[string]any{ + "flagType": "boolean", + "defaultValue": false, + "description": "Enable dark mode", + }, + }, + } + + newManifest := &Manifest{ + Flags: map[string]any{ + "darkMode": map[string]any{ + "flagType": "boolean", + "defaultValue": true, // Changed default value + "description": "Enable dark mode", + }, + }, + } + + changes, err := Compare(oldManifest, newManifest, CompareOptions{}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if len(changes) != 1 { + t.Errorf("expected 1 change for defaultValue change, got %d", len(changes)) + } + + if changes[0].Type != "change" { + t.Errorf("expected change type, got %s", changes[0].Type) + } +} + +// Test detecting flagType changes +func TestCompareFlagTypeChange(t *testing.T) { + oldManifest := &Manifest{ + Flags: map[string]any{ + "feature": map[string]any{ + "flagType": "boolean", + "defaultValue": false, + }, + }, + } + + newManifest := &Manifest{ + Flags: map[string]any{ + "feature": map[string]any{ + "flagType": "string", + "defaultValue": "false", + }, + }, + } + + changes, err := Compare(oldManifest, newManifest, CompareOptions{}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if len(changes) != 1 { + t.Errorf("expected 1 change for flagType change, got %d", len(changes)) + } +} + +// Test multiple ignore patterns +func TestCompareMultipleIgnorePatterns(t *testing.T) { + oldManifest := &Manifest{ + Flags: map[string]any{ + "darkMode": map[string]any{ + "flagType": "boolean", + "defaultValue": false, + "description": "Old description", + "metadata": map[string]any{ + "author": "Old author", + "timestamp": "2023-01-01", + }, + }, + }, + } + + newManifest := &Manifest{ + Flags: map[string]any{ + "darkMode": map[string]any{ + "flagType": "boolean", + "defaultValue": false, + "description": "New description", + "metadata": map[string]any{ + "author": "New author", + "timestamp": "2024-01-01", + }, + }, + }, + } + + // Ignore both description and metadata fields + changes, err := Compare(oldManifest, newManifest, CompareOptions{ + IgnorePatterns: []string{"description", "metadata.*"}, + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if len(changes) != 0 { + t.Errorf("expected 0 changes with multiple ignore patterns, got %d: %v", len(changes), changes) + } +} + +// Test wildcard pattern matching +func TestCompareWildcardPattern(t *testing.T) { + oldManifest := &Manifest{ + Flags: map[string]any{ + "darkMode": map[string]any{ + "flagType": "boolean", + "defaultValue": false, + "metadata": map[string]any{ + "author": "John", + "date": "2023-01-01", + }, + }, + }, + } + + newManifest := &Manifest{ + Flags: map[string]any{ + "darkMode": map[string]any{ + "flagType": "boolean", + "defaultValue": false, + "metadata": map[string]any{ + "author": "Jane", + "date": "2024-01-01", + }, + }, + }, + } + + // Use wildcard to ignore all metadata fields + changes, err := Compare(oldManifest, newManifest, CompareOptions{ + IgnorePatterns: []string{"flags.*.metadata.*"}, + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if len(changes) != 0 { + t.Errorf("expected 0 changes with wildcard pattern, got %d: %v", len(changes), changes) + } +} + +// Test that functional changes are still detected when ignoring non-functional fields +func TestCompareFunctionalChangesWithIgnore(t *testing.T) { + oldManifest := &Manifest{ + Flags: map[string]any{ + "darkMode": map[string]any{ + "flagType": "boolean", + "defaultValue": false, + "description": "Old description", + }, + }, + } + + newManifest := &Manifest{ + Flags: map[string]any{ + "darkMode": map[string]any{ + "flagType": "boolean", + "defaultValue": true, // Functional change + "description": "New description", + }, + }, + } + + // Even when ignoring description, defaultValue change should be detected + changes, err := Compare(oldManifest, newManifest, CompareOptions{ + IgnorePatterns: []string{"description"}, + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if len(changes) != 1 { + t.Errorf("expected 1 change for defaultValue, got %d", len(changes)) + } + + if changes[0].Type != "change" { + t.Errorf("expected change type, got %s", changes[0].Type) + } +} + +// Test specific flag ignore pattern +func TestCompareIgnoreSpecificFlag(t *testing.T) { + oldManifest := &Manifest{ + Flags: map[string]any{ + "darkMode": map[string]any{ + "flagType": "boolean", + "defaultValue": false, + "description": "Dark mode description", + }, + "featureX": map[string]any{ + "flagType": "boolean", + "defaultValue": true, + "description": "Feature X description", + }, + }, + } + + newManifest := &Manifest{ + Flags: map[string]any{ + "darkMode": map[string]any{ + "flagType": "boolean", + "defaultValue": false, + "description": "Updated dark mode description", + }, + "featureX": map[string]any{ + "flagType": "boolean", + "defaultValue": true, + "description": "Updated feature X description", + }, + }, + } + + // Ignore only darkMode's description + changes, err := Compare(oldManifest, newManifest, CompareOptions{ + IgnorePatterns: []string{"flags.darkMode.description"}, + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + // Should detect change only in featureX description + if len(changes) != 1 { + t.Errorf("expected 1 change, got %d: %v", len(changes), changes) + } + + if changes[0].Path != "flags.featureX" { + t.Errorf("expected change in featureX, got %s", changes[0].Path) + } +} From 058d60e2e0a061129aa3350bc47a1e75e30d6af1 Mon Sep 17 00:00:00 2001 From: Michael Beemer Date: Fri, 7 Nov 2025 14:44:56 -0500 Subject: [PATCH 2/6] remove doublestar logic, update tests, remove ignored or invalid output props Signed-off-by: Michael Beemer --- internal/cmd/compare.go | 33 +++++++---- internal/cmd/compare_test.go | 111 +++++++++++++++++++++++++++-------- internal/manifest/compare.go | 25 ++++---- 3 files changed, 122 insertions(+), 47 deletions(-) diff --git a/internal/cmd/compare.go b/internal/cmd/compare.go index 80c0e8af..1d16b345 100644 --- a/internal/cmd/compare.go +++ b/internal/cmd/compare.go @@ -69,13 +69,13 @@ func GetCompareCmd() *cobra.Command { // Render differences based on the output format switch manifest.OutputFormat(outputFormat) { case manifest.OutputFormatFlat: - return renderFlatDiff(changes, cmd) + return renderFlatDiff(changes, ignorePatterns, cmd) case manifest.OutputFormatJSON: - return renderJSONDiff(changes, cmd) + return renderJSONDiff(changes, ignorePatterns, cmd) case manifest.OutputFormatYAML: - return renderYAMLDiff(changes, cmd) + return renderYAMLDiff(changes, ignorePatterns, cmd) default: - return renderTreeDiff(changes, cmd) + return renderTreeDiff(changes, ignorePatterns, cmd) } }, } @@ -112,7 +112,7 @@ func loadManifest(path string) (*manifest.Manifest, error) { } // renderTreeDiff renders changes with tree-structured inline differences -func renderTreeDiff(changes []manifest.Change, cmd *cobra.Command) error { +func renderTreeDiff(changes []manifest.Change, ignorePatterns []string, cmd *cobra.Command) error { pterm.Info.Printf("Found %d difference(s) between manifests:\n\n", len(changes)) // Group changes by type for easier reading @@ -165,7 +165,7 @@ func renderTreeDiff(changes []manifest.Change, cmd *cobra.Command) error { pterm.FgYellow.Printf(" ~ %s\n", flagName) // Show field-level diff - fieldChanges := getFieldChanges(change.OldValue, change.NewValue) + fieldChanges := getFieldChanges(flagName, change.OldValue, change.NewValue, ignorePatterns) if len(fieldChanges) > 0 { for _, fc := range fieldChanges { fmt.Printf(" • %s: %s → %s\n", fc.Field, fc.OldValue, fc.NewValue) @@ -192,8 +192,8 @@ type fieldChange struct { NewValue string } -// getFieldChanges extracts field-level changes between two flag objects -func getFieldChanges(oldVal, newVal any) []fieldChange { +// getFieldChanges extracts field-level changes between two flag objects, filtering out ignored and unknown fields +func getFieldChanges(flagName string, oldVal, newVal any, ignorePatterns []string) []fieldChange { var changes []fieldChange // Convert to maps @@ -215,6 +215,17 @@ func getFieldChanges(oldVal, newVal any) []fieldChange { // Compare each field for field := range allFields { + // Check if this field should be ignored + fieldPath := fmt.Sprintf("flags.%s.%s", flagName, field) + if manifest.ShouldIgnorePathForTest(fieldPath, ignorePatterns) { + continue + } + + // Check if this is a known property + if !manifest.IsKnownPropertyForTest(fieldPath, flagName) { + continue + } + oldFieldVal, oldExists := oldMap[field] newFieldVal, newExists := newMap[field] @@ -286,7 +297,7 @@ func formatFieldValue(val any) string { } // renderFlatDiff renders changes in a flat format -func renderFlatDiff(changes []manifest.Change, cmd *cobra.Command) error { +func renderFlatDiff(changes []manifest.Change, ignorePatterns []string, cmd *cobra.Command) error { pterm.Info.Printf("Found %d difference(s) between manifests:\n\n", len(changes)) for _, change := range changes { @@ -305,7 +316,7 @@ func renderFlatDiff(changes []manifest.Change, cmd *cobra.Command) error { } // renderJSONDiff renders changes in JSON format -func renderJSONDiff(changes []manifest.Change, cmd *cobra.Command) error { +func renderJSONDiff(changes []manifest.Change, ignorePatterns []string, cmd *cobra.Command) error { // Create a structured response that can be easily consumed by tools type structuredOutput struct { TotalChanges int `json:"totalChanges" yaml:"totalChanges"` @@ -341,7 +352,7 @@ func renderJSONDiff(changes []manifest.Change, cmd *cobra.Command) error { } // renderYAMLDiff renders changes in YAML format -func renderYAMLDiff(changes []manifest.Change, cmd *cobra.Command) error { +func renderYAMLDiff(changes []manifest.Change, ignorePatterns []string, cmd *cobra.Command) error { // Use the same structured output type as JSON but with YAML tags type structuredOutput struct { TotalChanges int `json:"totalChanges" yaml:"totalChanges"` diff --git a/internal/cmd/compare_test.go b/internal/cmd/compare_test.go index dc19c0b6..bbe98ec1 100644 --- a/internal/cmd/compare_test.go +++ b/internal/cmd/compare_test.go @@ -1,7 +1,10 @@ package cmd import ( + "bytes" "fmt" + "io" + "os" "testing" "github.com/stretchr/testify/assert" @@ -53,49 +56,105 @@ func TestCompareManifests(t *testing.T) { } } +// captureStdout captures stdout during test execution +func captureStdout(f func()) string { + old := os.Stdout + r, w, _ := os.Pipe() + os.Stdout = w + + f() + + w.Close() + os.Stdout = old + + var buf bytes.Buffer + io.Copy(&buf, r) + return buf.String() +} + func TestCompareWithIgnoreFlag(t *testing.T) { // Test that the ignore flag is properly parsed and passed to comparison t.Run("single_ignore_pattern", func(t *testing.T) { - rootCmd := GetRootCmd() + output := captureStdout(func() { + rootCmd := GetRootCmd() + + rootCmd.SetArgs([]string{ + "compare", + "--manifest", "testdata/source_manifest.json", + "--against", "testdata/target_manifest.json", + "--ignore", "description", + }) - rootCmd.SetArgs([]string{ - "compare", - "--manifest", "testdata/source_manifest.json", - "--against", "testdata/target_manifest.json", - "--ignore", "description", + err := rootCmd.Execute() + assert.NoError(t, err, "Command should execute with single ignore pattern") }) - err := rootCmd.Execute() - assert.NoError(t, err, "Command should execute with single ignore pattern") + // Verify that the output doesn't contain description changes in the field-level diff + // The word "description" may still appear in JSON for additions/removals, which is fine + assert.NotContains(t, output, "• description:", + "Output should not show description field changes when it's ignored") }) t.Run("multiple_ignore_patterns", func(t *testing.T) { - rootCmd := GetRootCmd() - - rootCmd.SetArgs([]string{ - "compare", - "--manifest", "testdata/source_manifest.json", - "--against", "testdata/target_manifest.json", - "--ignore", "description", - "--ignore", "metadata.*", + output := captureStdout(func() { + rootCmd := GetRootCmd() + + rootCmd.SetArgs([]string{ + "compare", + "--manifest", "testdata/source_manifest.json", + "--against", "testdata/target_manifest.json", + "--ignore", "description", + "--ignore", "metadata.*", + }) + + err := rootCmd.Execute() + assert.NoError(t, err, "Command should execute with multiple ignore patterns") }) - err := rootCmd.Execute() - assert.NoError(t, err, "Command should execute with multiple ignore patterns") + // Verify that the output doesn't contain ignored fields in the field-level diff + assert.NotContains(t, output, "• description:", + "Output should not show description field changes when it's ignored") + assert.NotContains(t, output, "• metadata", + "Output should not show metadata field changes when it's ignored") }) t.Run("ignore_with_wildcard", func(t *testing.T) { - rootCmd := GetRootCmd() + output := captureStdout(func() { + rootCmd := GetRootCmd() - rootCmd.SetArgs([]string{ - "compare", - "--manifest", "testdata/source_manifest.json", - "--against", "testdata/target_manifest.json", - "--ignore", "flags.*.description", + rootCmd.SetArgs([]string{ + "compare", + "--manifest", "testdata/source_manifest.json", + "--against", "testdata/target_manifest.json", + "--ignore", "flags.*.description", + }) + + err := rootCmd.Execute() + assert.NoError(t, err, "Command should execute with wildcard ignore pattern") + }) + + // Verify that the output doesn't contain description changes in the field-level diff + assert.NotContains(t, output, "• description:", + "Output should not show description field changes when using wildcard pattern 'flags.*.description'") + }) + + t.Run("without_ignore_shows_description", func(t *testing.T) { + output := captureStdout(func() { + rootCmd := GetRootCmd() + + rootCmd.SetArgs([]string{ + "compare", + "--manifest", "testdata/source_manifest.json", + "--against", "testdata/target_manifest.json", + }) + + err := rootCmd.Execute() + assert.NoError(t, err, "Command should execute without ignore pattern") }) - err := rootCmd.Execute() - assert.NoError(t, err, "Command should execute with wildcard ignore pattern") + // Verify that the output DOES contain description field changes when not ignored + assert.Contains(t, output, "• description:", + "Output should show description field changes when it's not ignored") }) } diff --git a/internal/manifest/compare.go b/internal/manifest/compare.go index 15b453b3..f0b23c92 100644 --- a/internal/manifest/compare.go +++ b/internal/manifest/compare.go @@ -153,6 +153,11 @@ func isKnownProperty(path, flagKey string) bool { return knownFlagProperties[propertyName] } +// IsKnownPropertyForTest checks if a path is a known property (exported for testing/rendering) +func IsKnownPropertyForTest(path, flagKey string) bool { + return isKnownProperty(path, flagKey) +} + // flattenToMap recursively flattens a nested structure into a map of path->value func flattenToMap(obj any, prefix string) map[string]any { result := make(map[string]any) @@ -209,6 +214,11 @@ func shouldIgnorePath(path string, ignorePatterns []string) bool { return false } +// ShouldIgnorePathForTest checks if a path should be ignored (exported for testing/rendering) +func ShouldIgnorePathForTest(path string, ignorePatterns []string) bool { + return shouldIgnorePath(path, ignorePatterns) +} + // matchesPattern checks if a path matches a given pattern // Supports: // - Full paths with globs: flags.*.description @@ -229,7 +239,7 @@ func matchesPattern(path, pattern string) bool { } // Try matching with wildcard support - if matchesWithDoublestar(path, pattern) { + if matchesPathSegments(path, pattern) { return true } @@ -291,8 +301,8 @@ func matchesSubsequence(pathParts, patternParts []string) bool { return true } -// matchesWithDoublestar handles patterns with * wildcards matching any segment -func matchesWithDoublestar(path, pattern string) bool { +// matchesPathSegments handles patterns with * wildcards matching any single segment +func matchesPathSegments(path, pattern string) bool { pathParts := strings.Split(path, ".") patternParts := strings.Split(pattern, ".") @@ -304,15 +314,10 @@ func matchesWithDoublestar(path, pattern string) bool { // Match each pattern part against corresponding path part pathIdx := 0 for patternIdx, patternPart := range patternParts { - switch patternPart { - case "*": + if patternPart == "*" { // Wildcard matches any single segment pathIdx++ - case "**": - // Double wildcard matches any number of segments - // This is complex, so we'll keep it simple for now - return false - default: + } else { // Exact match required if pathIdx >= len(pathParts) || pathParts[pathIdx] != patternPart { return false From 3abedaa5e86a35fe662a11f5dcf12837f666ffe3 Mon Sep 17 00:00:00 2001 From: Michael Beemer Date: Fri, 7 Nov 2025 14:50:31 -0500 Subject: [PATCH 3/6] fixed linting issue Signed-off-by: Michael Beemer --- internal/cmd/compare_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/cmd/compare_test.go b/internal/cmd/compare_test.go index bbe98ec1..71c62f39 100644 --- a/internal/cmd/compare_test.go +++ b/internal/cmd/compare_test.go @@ -68,7 +68,7 @@ func captureStdout(f func()) string { os.Stdout = old var buf bytes.Buffer - io.Copy(&buf, r) + _, _ = io.Copy(&buf, r) return buf.String() } From d33e449c61ebbcd2a6e53177080addc858d7d4c0 Mon Sep 17 00:00:00 2001 From: Michael Beemer Date: Fri, 7 Nov 2025 15:05:28 -0500 Subject: [PATCH 4/6] address pr feedback Signed-off-by: Michael Beemer --- internal/cmd/compare.go | 16 ++++++++-------- internal/manifest/compare.go | 29 ++++++----------------------- 2 files changed, 14 insertions(+), 31 deletions(-) diff --git a/internal/cmd/compare.go b/internal/cmd/compare.go index 1d16b345..d7099748 100644 --- a/internal/cmd/compare.go +++ b/internal/cmd/compare.go @@ -69,11 +69,11 @@ func GetCompareCmd() *cobra.Command { // Render differences based on the output format switch manifest.OutputFormat(outputFormat) { case manifest.OutputFormatFlat: - return renderFlatDiff(changes, ignorePatterns, cmd) + return renderFlatDiff(changes, cmd) case manifest.OutputFormatJSON: - return renderJSONDiff(changes, ignorePatterns, cmd) + return renderJSONDiff(changes, cmd) case manifest.OutputFormatYAML: - return renderYAMLDiff(changes, ignorePatterns, cmd) + return renderYAMLDiff(changes, cmd) default: return renderTreeDiff(changes, ignorePatterns, cmd) } @@ -217,12 +217,12 @@ func getFieldChanges(flagName string, oldVal, newVal any, ignorePatterns []strin for field := range allFields { // Check if this field should be ignored fieldPath := fmt.Sprintf("flags.%s.%s", flagName, field) - if manifest.ShouldIgnorePathForTest(fieldPath, ignorePatterns) { + if manifest.ShouldIgnorePath(fieldPath, ignorePatterns) { continue } // Check if this is a known property - if !manifest.IsKnownPropertyForTest(fieldPath, flagName) { + if !manifest.IsKnownProperty(fieldPath, flagName) { continue } @@ -297,7 +297,7 @@ func formatFieldValue(val any) string { } // renderFlatDiff renders changes in a flat format -func renderFlatDiff(changes []manifest.Change, ignorePatterns []string, cmd *cobra.Command) error { +func renderFlatDiff(changes []manifest.Change, cmd *cobra.Command) error { pterm.Info.Printf("Found %d difference(s) between manifests:\n\n", len(changes)) for _, change := range changes { @@ -316,7 +316,7 @@ func renderFlatDiff(changes []manifest.Change, ignorePatterns []string, cmd *cob } // renderJSONDiff renders changes in JSON format -func renderJSONDiff(changes []manifest.Change, ignorePatterns []string, cmd *cobra.Command) error { +func renderJSONDiff(changes []manifest.Change, cmd *cobra.Command) error { // Create a structured response that can be easily consumed by tools type structuredOutput struct { TotalChanges int `json:"totalChanges" yaml:"totalChanges"` @@ -352,7 +352,7 @@ func renderJSONDiff(changes []manifest.Change, ignorePatterns []string, cmd *cob } // renderYAMLDiff renders changes in YAML format -func renderYAMLDiff(changes []manifest.Change, ignorePatterns []string, cmd *cobra.Command) error { +func renderYAMLDiff(changes []manifest.Change, cmd *cobra.Command) error { // Use the same structured output type as JSON but with YAML tags type structuredOutput struct { TotalChanges int `json:"totalChanges" yaml:"totalChanges"` diff --git a/internal/manifest/compare.go b/internal/manifest/compare.go index f0b23c92..7bac5553 100644 --- a/internal/manifest/compare.go +++ b/internal/manifest/compare.go @@ -3,7 +3,6 @@ package manifest import ( "fmt" "maps" - "path/filepath" "reflect" "strings" ) @@ -110,12 +109,12 @@ func flagHasChanges(oldFlag, newFlag any, flagKey string, ignorePatterns []strin // Check each path for differences (skipping ignored paths and unknown properties) for path := range allPaths { // Skip paths that should be ignored - if shouldIgnorePath(path, ignorePatterns) { + if ShouldIgnorePath(path, ignorePatterns) { continue } // Skip unknown properties (only compare known schema properties) - if !isKnownProperty(path, flagKey) { + if !IsKnownProperty(path, flagKey) { continue } @@ -131,8 +130,8 @@ func flagHasChanges(oldFlag, newFlag any, flagKey string, ignorePatterns []strin return false } -// isKnownProperty checks if a path represents a known schema property -func isKnownProperty(path, flagKey string) bool { +// IsKnownProperty checks if a path represents a known schema property +func IsKnownProperty(path, flagKey string) bool { // Extract the property name from the path // Path format: flags.. or flags... prefix := fmt.Sprintf("flags.%s.", flagKey) @@ -153,11 +152,6 @@ func isKnownProperty(path, flagKey string) bool { return knownFlagProperties[propertyName] } -// IsKnownPropertyForTest checks if a path is a known property (exported for testing/rendering) -func IsKnownPropertyForTest(path, flagKey string) bool { - return isKnownProperty(path, flagKey) -} - // flattenToMap recursively flattens a nested structure into a map of path->value func flattenToMap(obj any, prefix string) map[string]any { result := make(map[string]any) @@ -204,8 +198,8 @@ func isSimpleType(v any) bool { } } -// shouldIgnorePath checks if a path should be ignored based on the ignore patterns -func shouldIgnorePath(path string, ignorePatterns []string) bool { +// ShouldIgnorePath checks if a path should be ignored based on the ignore patterns +func ShouldIgnorePath(path string, ignorePatterns []string) bool { for _, pattern := range ignorePatterns { if matchesPattern(path, pattern) { return true @@ -214,11 +208,6 @@ func shouldIgnorePath(path string, ignorePatterns []string) bool { return false } -// ShouldIgnorePathForTest checks if a path should be ignored (exported for testing/rendering) -func ShouldIgnorePathForTest(path string, ignorePatterns []string) bool { - return shouldIgnorePath(path, ignorePatterns) -} - // matchesPattern checks if a path matches a given pattern // Supports: // - Full paths with globs: flags.*.description @@ -232,12 +221,6 @@ func matchesPattern(path, pattern string) bool { // If pattern contains a dot, treat it as a path pattern if strings.Contains(pattern, ".") { - // Try direct filepath.Match first - matched, err := filepath.Match(pattern, path) - if err == nil && matched { - return true - } - // Try matching with wildcard support if matchesPathSegments(path, pattern) { return true From f195f92ac041102347625ef6e3856c570ebc08de Mon Sep 17 00:00:00 2001 From: Michael Beemer Date: Fri, 7 Nov 2025 15:31:58 -0500 Subject: [PATCH 5/6] unify the filtering output Signed-off-by: Michael Beemer --- internal/cmd/compare.go | 22 ++++--------- internal/manifest/compare.go | 64 +++++++++++++++++++++++++++++++++--- 2 files changed, 66 insertions(+), 20 deletions(-) diff --git a/internal/cmd/compare.go b/internal/cmd/compare.go index d7099748..ef0c3092 100644 --- a/internal/cmd/compare.go +++ b/internal/cmd/compare.go @@ -75,7 +75,7 @@ func GetCompareCmd() *cobra.Command { case manifest.OutputFormatYAML: return renderYAMLDiff(changes, cmd) default: - return renderTreeDiff(changes, ignorePatterns, cmd) + return renderTreeDiff(changes, cmd) } }, } @@ -112,7 +112,7 @@ func loadManifest(path string) (*manifest.Manifest, error) { } // renderTreeDiff renders changes with tree-structured inline differences -func renderTreeDiff(changes []manifest.Change, ignorePatterns []string, cmd *cobra.Command) error { +func renderTreeDiff(changes []manifest.Change, cmd *cobra.Command) error { pterm.Info.Printf("Found %d difference(s) between manifests:\n\n", len(changes)) // Group changes by type for easier reading @@ -165,7 +165,7 @@ func renderTreeDiff(changes []manifest.Change, ignorePatterns []string, cmd *cob pterm.FgYellow.Printf(" ~ %s\n", flagName) // Show field-level diff - fieldChanges := getFieldChanges(flagName, change.OldValue, change.NewValue, ignorePatterns) + fieldChanges := getFieldChanges(flagName, change.OldValue, change.NewValue) if len(fieldChanges) > 0 { for _, fc := range fieldChanges { fmt.Printf(" • %s: %s → %s\n", fc.Field, fc.OldValue, fc.NewValue) @@ -192,8 +192,8 @@ type fieldChange struct { NewValue string } -// getFieldChanges extracts field-level changes between two flag objects, filtering out ignored and unknown fields -func getFieldChanges(flagName string, oldVal, newVal any, ignorePatterns []string) []fieldChange { +// getFieldChanges extracts field-level changes between two flag objects +func getFieldChanges(flagName string, oldVal, newVal any) []fieldChange { var changes []fieldChange // Convert to maps @@ -214,18 +214,8 @@ func getFieldChanges(flagName string, oldVal, newVal any, ignorePatterns []strin } // Compare each field + // Note: Fields are already filtered at the Compare() level, so we don't need to filter here for field := range allFields { - // Check if this field should be ignored - fieldPath := fmt.Sprintf("flags.%s.%s", flagName, field) - if manifest.ShouldIgnorePath(fieldPath, ignorePatterns) { - continue - } - - // Check if this is a known property - if !manifest.IsKnownProperty(fieldPath, flagName) { - continue - } - oldFieldVal, oldExists := oldMap[field] newFieldVal, newExists := newMap[field] diff --git a/internal/manifest/compare.go b/internal/manifest/compare.go index 7bac5553..fe0fa380 100644 --- a/internal/manifest/compare.go +++ b/internal/manifest/compare.go @@ -33,15 +33,15 @@ func Compare(oldManifest, newManifest *Manifest, opts CompareOptions) ([]Change, changes = append(changes, Change{ Type: "change", Path: fmt.Sprintf("flags.%s", key), - OldValue: oldFlag, - NewValue: newFlag, + OldValue: filterFlagFields(oldFlag, key, opts.IgnorePatterns), + NewValue: filterFlagFields(newFlag, key, opts.IgnorePatterns), }) } } else { changes = append(changes, Change{ Type: "add", Path: fmt.Sprintf("flags.%s", key), - NewValue: newFlag, + NewValue: filterFlagFields(newFlag, key, opts.IgnorePatterns), }) } } @@ -52,7 +52,7 @@ func Compare(oldManifest, newManifest *Manifest, opts CompareOptions) ([]Change, changes = append(changes, Change{ Type: "remove", Path: fmt.Sprintf("flags.%s", key), - OldValue: oldFlag, + OldValue: filterFlagFields(oldFlag, key, opts.IgnorePatterns), }) } } @@ -208,6 +208,62 @@ func ShouldIgnorePath(path string, ignorePatterns []string) bool { return false } +// filterFlagFields returns a filtered copy of the flag with ignored and unknown fields removed +func filterFlagFields(flag any, flagKey string, ignorePatterns []string) any { + flagMap, ok := flag.(map[string]any) + if !ok { + return flag + } + + filtered := make(map[string]any) + + for field, value := range flagMap { + fieldPath := fmt.Sprintf("flags.%s.%s", flagKey, field) + + // Skip if ignored + if ShouldIgnorePath(fieldPath, ignorePatterns) { + continue + } + + // Skip if unknown property + if !IsKnownProperty(fieldPath, flagKey) { + continue + } + + // For nested objects, recursively filter + if nestedMap, ok := value.(map[string]any); ok { + filtered[field] = filterNestedFields(nestedMap, fieldPath, ignorePatterns) + } else { + filtered[field] = value + } + } + + return filtered +} + +// filterNestedFields recursively filters nested map structures +func filterNestedFields(nested map[string]any, parentPath string, ignorePatterns []string) map[string]any { + filtered := make(map[string]any) + + for key, value := range nested { + path := fmt.Sprintf("%s.%s", parentPath, key) + + // Skip if ignored + if ShouldIgnorePath(path, ignorePatterns) { + continue + } + + // Recursively handle nested objects + if nestedMap, ok := value.(map[string]any); ok { + filtered[key] = filterNestedFields(nestedMap, path, ignorePatterns) + } else { + filtered[key] = value + } + } + + return filtered +} + // matchesPattern checks if a path matches a given pattern // Supports: // - Full paths with globs: flags.*.description From 3130895c90a06ed339012726bd96c72af463ac2c Mon Sep 17 00:00:00 2001 From: Michael Beemer Date: Fri, 7 Nov 2025 15:40:42 -0500 Subject: [PATCH 6/6] Update internal/cmd/compare_test.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Michael Beemer --- internal/cmd/compare_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/internal/cmd/compare_test.go b/internal/cmd/compare_test.go index 71c62f39..7cd05cf4 100644 --- a/internal/cmd/compare_test.go +++ b/internal/cmd/compare_test.go @@ -68,7 +68,10 @@ func captureStdout(f func()) string { os.Stdout = old var buf bytes.Buffer - _, _ = io.Copy(&buf, r) + _, err := io.Copy(&buf, r) + if err != nil { + fmt.Fprintf(os.Stderr, "captureStdout: error copying output: %v\n", err) + } return buf.String() }