From 02f0adcb4e4c7a14d3442d9806f9a6d05ad5a832 Mon Sep 17 00:00:00 2001 From: Giulio Cardillo Date: Thu, 19 Feb 2026 20:45:49 +0100 Subject: [PATCH 1/2] Fixed reconciliation conditions: - when syncPolicy changes - when order of helm parameters changes - when order of helm valueFiles changes --- internal/controller/argo.go | 143 ++++++++++++++++------- internal/controller/argo_test.go | 195 +++++++++++++++++-------------- 2 files changed, 212 insertions(+), 126 deletions(-) diff --git a/internal/controller/argo.go b/internal/controller/argo.go index 556f352b9..359cffc3e 100644 --- a/internal/controller/argo.go +++ b/internal/controller/argo.go @@ -916,14 +916,8 @@ func updateApplication(client argoclient.Interface, target, current *argoapi.App } else if target == nil { return false, fmt.Errorf("target application was nil") } - if current.Spec.Sources == nil { - if compareSource(target.Spec.Source, current.Spec.Source) { - return false, nil - } - } else { - if compareSources(target.Spec.Sources, current.Spec.Sources) { - return false, nil - } + if compareApplication(current, target) { + return false, nil } spec := current.Spec.DeepCopy() @@ -938,8 +932,79 @@ func removeApplication(client argoclient.Interface, name, namespace string) erro return client.ArgoprojV1alpha1().Applications(namespace).Delete(context.Background(), name, metav1.DeleteOptions{}) } +func compareApplication(goal, actual *argoapi.Application) bool { + if goal == nil && actual == nil { + return true + } + if (goal == nil) != (actual == nil) { + return false + } + if !compareSource(goal.Spec.Source, actual.Spec.Source) { + return false + } + if !compareSources(goal.Spec.Sources, actual.Spec.Sources) { + return false + } + if !compareSyncPolicy(goal.Spec.SyncPolicy, actual.Spec.SyncPolicy) { + return false + } + return true +} + +func compareSyncPolicy(goal, actual *argoapi.SyncPolicy) bool { + if goal == nil && actual == nil { + return true + } + if (goal == nil) != (actual == nil) { + return false + } + if !compareAutomatedSyncPolicy(goal.Automated, actual.Automated) { + return false + } + if !compareSyncOptions(goal.SyncOptions, actual.SyncOptions) { + return false + } + return true +} + +func compareAutomatedSyncPolicy(goal, actual *argoapi.SyncPolicyAutomated) bool { + if goal == nil && actual == nil { + return true + } + if (goal == nil) != (actual == nil) { + return false + } + if goal.Prune != actual.Prune { + log.Printf("SyncPolicy Prune changed %t -> %t\n", actual.Prune, goal.Prune) + return false + } + return true +} + +func compareSyncOptions(goal, actual argoapi.SyncOptions) bool { + if goal == nil && actual == nil { + return true + } + if (goal == nil) != (actual == nil) { + return false + } + if len(goal) != len(actual) { + return false + } + for i, gS := range goal { + if gS != actual[i] { + log.Printf("SyncOption at position %d changed: %s -> %s\n", i, actual[i], gS) + return false + } + } + return true +} + func compareSource(goal, actual *argoapi.ApplicationSource) bool { - if goal == nil || actual == nil { + if goal == nil && actual == nil { + return true + } + if (goal == nil) != (actual == nil) { return false } if goal.RepoURL != actual.RepoURL { @@ -971,7 +1036,10 @@ func compareSource(goal, actual *argoapi.ApplicationSource) bool { } func compareSources(goal, actual argoapi.ApplicationSources) bool { - if actual == nil || goal == nil { + if goal == nil && actual == nil { + return true + } + if (goal == nil) != (actual == nil) { return false } if len(actual) != len(goal) { @@ -1000,49 +1068,46 @@ func compareHelmSource(goal, actual *argoapi.ApplicationSourceHelm) bool { return true } -func compareHelmParameter(goal argoapi.HelmParameter, actual []argoapi.HelmParameter) bool { - for _, param := range actual { - if goal.Name == param.Name { - if goal.Value == param.Value { - return true - } - log.Printf("Parameter %q changed: %q -> %q", goal.Name, param.Value, goal.Value) - return false - } - } - log.Printf("Parameter %q not found", goal.Name) - return false -} - func compareHelmParameters(goal, actual []argoapi.HelmParameter) bool { + if goal == nil && actual == nil { + return true + } + if (goal == nil) != (actual == nil) { + return false + } if len(goal) != len(actual) { return false } - - for _, gP := range goal { - if !compareHelmParameter(gP, actual) { + for i, gP := range goal { + if gP.Name != actual[i].Name { + log.Printf("Helm parameter at position %d changed: %s -> %s\n", i, actual[i].Name, gP.Name) return false } - } - return true -} - -func compareHelmValueFile(goal string, actual []string) bool { - for _, value := range actual { - if goal == value { - return true + if gP.Value != actual[i].Value { + log.Printf("Helm parameter %s changed: %s -> %s\n", actual[i].Name, actual[i].Value, gP.Value) + return false + } + if gP.ForceString != actual[i].ForceString { + log.Printf("ForceString for Helm parameter %s changed: %t -> %t\n", actual[i].Name, actual[i].ForceString, gP.ForceString) + return false } } - log.Printf("Values file %q not found", goal) - return false + return true } func compareHelmValueFiles(goal, actual []string) bool { + if goal == nil && actual == nil { + return true + } + if (goal == nil) != (actual == nil) { + return false + } if len(goal) != len(actual) { return false } - for _, gV := range goal { - if !compareHelmValueFile(gV, actual) { + for i, gV := range goal { + if gV != actual[i] { + log.Printf("ValueFile at position %d changed: %s -> %s\n", i, actual[i], gV) return false } } diff --git a/internal/controller/argo_test.go b/internal/controller/argo_test.go index ff5a6089e..42b4006f4 100644 --- a/internal/controller/argo_test.go +++ b/internal/controller/argo_test.go @@ -6,6 +6,8 @@ import ( "fmt" "log" "os" + "slices" + "strings" argooperator "github.com/argoproj-labs/argocd-operator/api/v1beta1" . "github.com/onsi/ginkgo/v2" @@ -295,6 +297,15 @@ var _ = Describe("Argo Pattern", func() { sameGoal := goal Expect(compareHelmValueFiles(goal, sameGoal)).To(BeTrue()) }) + It("Compare Helm Value Files with different order", func() { + sortedGoal := make([]string, len(goal)) + reversedGoal := make([]string, len(goal)) + _ = copy(sortedGoal, goal) + _ = copy(reversedGoal, goal) + slices.Sort(sortedGoal) + slices.Reverse(reversedGoal) + Expect(compareHelmValueFiles(sortedGoal, reversedGoal)).To(BeFalse()) + }) }) Context("Compare Helm Parameters", func() { @@ -305,6 +316,19 @@ var _ = Describe("Argo Pattern", func() { sameGoalHelm := goalHelm Expect(compareHelmParameters(goalHelm, sameGoalHelm)).To(BeTrue()) }) + It("Compare Helm Parameters with different order", func() { + sortedGoalHelm := make([]argoapi.HelmParameter, len(goalHelm)) + reversedGoalHelm := make([]argoapi.HelmParameter, len(goalHelm)) + _ = copy(sortedGoalHelm, goalHelm) + _ = copy(reversedGoalHelm, goalHelm) + slices.SortFunc(sortedGoalHelm, func(a, b argoapi.HelmParameter) int { + return strings.Compare(a.Name, b.Name) + }) + slices.SortFunc(reversedGoalHelm, func(a, b argoapi.HelmParameter) int { + return strings.Compare(a.Name, b.Name) * -1 + }) + Expect(compareHelmParameters(sortedGoalHelm, reversedGoalHelm)).To(BeFalse()) + }) It("Test updateHelmParameter non existing Parameter", func() { nonexistantParam := api.PatternParameter{ Name: "Nonexistant", @@ -528,6 +552,90 @@ var _ = Describe("Argo Pattern", func() { }) }) + + Context("Compare SyncPolicy", func() { + var multiSourceArgoApp *argoapi.Application + var syncPolicy *argoapi.SyncPolicy + + BeforeEach(func() { + multiSourceArgoApp = newMultiSourceApplication(pattern) + syncPolicy = multiSourceArgoApp.Spec.SyncPolicy + }) + It("compareSyncPolicy() function identical", func() { + Expect(compareSyncPolicy(syncPolicy, syncPolicy)).To(BeTrue()) + }) + It("compareSyncPolicy() function differing", func() { + syncPolicyChanged := &argoapi.SyncPolicy{} + Expect(compareSyncPolicy(syncPolicy, syncPolicyChanged)).To(BeFalse()) + }) + It("compareSyncPolicy() function with nil arg1", func() { + Expect(compareSyncPolicy(syncPolicy, nil)).To(BeFalse()) + }) + It("compareSyncPolicy() function with nil arg2", func() { + Expect(compareSyncPolicy(nil, syncPolicy)).To(BeFalse()) + }) + + }) + + Context("Compare AutomatedSyncPolicy", func() { + var multiSourceArgoApp *argoapi.Application + var automatedSyncPolicy *argoapi.SyncPolicyAutomated + + BeforeEach(func() { + multiSourceArgoApp = newMultiSourceApplication(pattern) + automatedSyncPolicy = multiSourceArgoApp.Spec.SyncPolicy.Automated + }) + It("compareAutomatedSyncPolicy() function identical", func() { + Expect(compareAutomatedSyncPolicy(automatedSyncPolicy, automatedSyncPolicy)).To(BeTrue()) + }) + It("should return false and log the appropriate message", func() { + automatedSyncPolicyChanged := automatedSyncPolicy.DeepCopy() + automatedSyncPolicyChanged.Prune = true + logBuffer := new(bytes.Buffer) + log.SetOutput(logBuffer) + defer log.SetOutput(os.Stderr) + + result := compareAutomatedSyncPolicy(automatedSyncPolicy, automatedSyncPolicyChanged) + Expect(result).To(BeFalse()) + Expect(logBuffer.String()).To(ContainSubstring("SyncPolicy Prune changed true -> false")) + }) + It("compareAutomatedSyncPolicy() function with nil arg1", func() { + Expect(compareAutomatedSyncPolicy(automatedSyncPolicy, nil)).To(BeFalse()) + }) + It("compareAutomatedSyncPolicy() function with nil arg2", func() { + Expect(compareAutomatedSyncPolicy(nil, automatedSyncPolicy)).To(BeFalse()) + }) + + }) + + Context("Compare SyncOptions", func() { + var multiSourceArgoApp *argoapi.Application + var syncOptions argoapi.SyncOptions + + BeforeEach(func() { + multiSourceArgoApp = newMultiSourceApplication(pattern) + syncOptions = multiSourceArgoApp.Spec.SyncPolicy.SyncOptions + }) + It("compareSyncOptions() function identical", func() { + Expect(compareSyncOptions(syncOptions, syncOptions)).To(BeTrue()) + }) + It("compareSyncOptions() function differing", func() { + syncOptionsChanged := append(syncOptions, "key=value") + Expect(compareSyncOptions(syncOptions, syncOptionsChanged)).To(BeFalse()) + }) + It("Compare SyncOptions with different order", func() { + syncOptions1 := []string{"opt1=value1", "opt2=value2"} + syncOptions2 := []string{"opt2=value2", "opt1=value1"} + Expect(compareSyncOptions(syncOptions1, syncOptions2)).To(BeFalse()) + }) + It("compareSyncOptions() function with nil arg1", func() { + Expect(compareSyncOptions(syncOptions, nil)).To(BeFalse()) + }) + It("compareSyncOptions() function with nil arg2", func() { + Expect(compareSyncOptions(nil, syncOptions)).To(BeFalse()) + }) + + }) }) }) @@ -655,93 +763,6 @@ var _ = Describe("NewApplicationValues", func() { }) }) -var _ = Describe("CompareHelmValueFile", func() { - var ( - goal string - actual []string - ) - - Context("when the goal value is in the actual slice", func() { - BeforeEach(func() { - goal = "value1" - actual = []string{"value1", "value2", "value3"} - }) - - It("should return true", func() { - result := compareHelmValueFile(goal, actual) - Expect(result).To(BeTrue()) - }) - }) - - Context("when the goal value is not in the actual slice", func() { - BeforeEach(func() { - goal = "value4" - actual = []string{"value1", "value2", "value3"} - }) - - It("should return false and log the appropriate message", func() { - logBuffer := new(bytes.Buffer) - log.SetOutput(logBuffer) - defer log.SetOutput(os.Stderr) - - result := compareHelmValueFile(goal, actual) - Expect(result).To(BeFalse()) - Expect(logBuffer.String()).To(ContainSubstring("Values file \"value4\" not found")) - }) - }) - - Context("when the actual slice is empty", func() { - BeforeEach(func() { - goal = "value1" - actual = []string{} - }) - - It("should return false and log the appropriate message", func() { - logBuffer := new(bytes.Buffer) - log.SetOutput(logBuffer) - defer log.SetOutput(os.Stderr) - - result := compareHelmValueFile(goal, actual) - Expect(result).To(BeFalse()) - Expect(logBuffer.String()).To(ContainSubstring("Values file \"value1\" not found")) - }) - }) - - Context("when the goal value is empty", func() { - BeforeEach(func() { - goal = "" - actual = []string{"value1", "value2", "value3"} - }) - - It("should return false and log the appropriate message", func() { - logBuffer := new(bytes.Buffer) - log.SetOutput(logBuffer) - defer log.SetOutput(os.Stderr) - - result := compareHelmValueFile(goal, actual) - Expect(result).To(BeFalse()) - Expect(logBuffer.String()).To(ContainSubstring("Values file \"\" not found")) - }) - }) - - Context("when both the goal value and the actual slice are empty", func() { - BeforeEach(func() { - goal = "" - actual = []string{} - }) - - It("should return false and log the appropriate message", func() { - logBuffer := new(bytes.Buffer) - log.SetOutput(logBuffer) - defer log.SetOutput(os.Stderr) - - result := compareHelmValueFile(goal, actual) - Expect(result).To(BeFalse()) - Expect(logBuffer.String()).To(ContainSubstring("Values file \"\" not found")) - }) - }) -}) - var _ = Describe("NewArgoCD", func() { var ( name string From 93ee57b58e93fd4529860ee328cf95b916ef2443 Mon Sep 17 00:00:00 2001 From: Giulio Cardillo Date: Sun, 22 Feb 2026 18:52:04 +0100 Subject: [PATCH 2/2] Add check for syncPolicy.automated.allowEmpty and syncPolicy.automated.selfHeal --- internal/controller/argo.go | 8 ++++++++ internal/controller/argo_test.go | 22 ++++++++++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/internal/controller/argo.go b/internal/controller/argo.go index 359cffc3e..5398c4810 100644 --- a/internal/controller/argo.go +++ b/internal/controller/argo.go @@ -978,6 +978,14 @@ func compareAutomatedSyncPolicy(goal, actual *argoapi.SyncPolicyAutomated) bool log.Printf("SyncPolicy Prune changed %t -> %t\n", actual.Prune, goal.Prune) return false } + if goal.AllowEmpty != actual.AllowEmpty { + log.Printf("SyncPolicy AllowEmpty changed %t -> %t\n", actual.AllowEmpty, goal.AllowEmpty) + return false + } + if goal.SelfHeal != actual.SelfHeal { + log.Printf("SyncPolicy SelfHeal changed %t -> %t\n", actual.SelfHeal, goal.SelfHeal) + return false + } return true } diff --git a/internal/controller/argo_test.go b/internal/controller/argo_test.go index 42b4006f4..f7170f2dc 100644 --- a/internal/controller/argo_test.go +++ b/internal/controller/argo_test.go @@ -599,6 +599,28 @@ var _ = Describe("Argo Pattern", func() { Expect(result).To(BeFalse()) Expect(logBuffer.String()).To(ContainSubstring("SyncPolicy Prune changed true -> false")) }) + It("should return false and log the appropriate message", func() { + automatedSyncPolicyChanged := automatedSyncPolicy.DeepCopy() + automatedSyncPolicyChanged.AllowEmpty = true + logBuffer := new(bytes.Buffer) + log.SetOutput(logBuffer) + defer log.SetOutput(os.Stderr) + + result := compareAutomatedSyncPolicy(automatedSyncPolicy, automatedSyncPolicyChanged) + Expect(result).To(BeFalse()) + Expect(logBuffer.String()).To(ContainSubstring("SyncPolicy AllowEmpty changed true -> false")) + }) + It("should return false and log the appropriate message", func() { + automatedSyncPolicyChanged := automatedSyncPolicy.DeepCopy() + automatedSyncPolicyChanged.SelfHeal = true + logBuffer := new(bytes.Buffer) + log.SetOutput(logBuffer) + defer log.SetOutput(os.Stderr) + + result := compareAutomatedSyncPolicy(automatedSyncPolicy, automatedSyncPolicyChanged) + Expect(result).To(BeFalse()) + Expect(logBuffer.String()).To(ContainSubstring("SyncPolicy SelfHeal changed true -> false")) + }) It("compareAutomatedSyncPolicy() function with nil arg1", func() { Expect(compareAutomatedSyncPolicy(automatedSyncPolicy, nil)).To(BeFalse()) })