From 1ffd3be4940fc239cb886cef157f9a23865ed138 Mon Sep 17 00:00:00 2001 From: Shane Bryzak Date: Fri, 3 May 2024 12:47:56 +1000 Subject: [PATCH 1/7] add ConditionsMatch function --- pkg/condition/condition.go | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/pkg/condition/condition.go b/pkg/condition/condition.go index 630e74e2..72acb1c4 100644 --- a/pkg/condition/condition.go +++ b/pkg/condition/condition.go @@ -146,3 +146,32 @@ func HasConditionReason(conditions []toolchainv1alpha1.Condition, conditionType con, found := FindConditionByType(conditions, conditionType) return found && con.Reason == reason } + +// ConditionsMatch checks whether the specified conditions match and return true if they do +func ConditionsMatch(first, second []toolchainv1alpha1.Condition) bool { + if len(first) != len(second) { + return false + } + for _, c := range first { + if !ContainsCondition(second, c) { + return false + } + } + for _, c := range second { + if !ContainsCondition(first, c) { + return false + } + } + return true +} + +// ContainsCondition returns true if the specified list of conditions contains the specified condition and the statuses of the conditions match. +// LastTransitionTime is ignored. +func ContainsCondition(conditions []toolchainv1alpha1.Condition, contains toolchainv1alpha1.Condition) bool { + for _, c := range conditions { + if c.Type == contains.Type { + return contains.Status == c.Status && contains.Reason == c.Reason && contains.Message == c.Message + } + } + return false +} From 7f94e3e7c0443cb4e6b7415f27ca41ea1b6ec128 Mon Sep 17 00:00:00 2001 From: Shane Bryzak Date: Fri, 3 May 2024 14:30:57 +1000 Subject: [PATCH 2/7] added tests --- pkg/condition/condition_blackbox_test.go | 49 ++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/pkg/condition/condition_blackbox_test.go b/pkg/condition/condition_blackbox_test.go index e1c676b5..0b33d40f 100644 --- a/pkg/condition/condition_blackbox_test.go +++ b/pkg/condition/condition_blackbox_test.go @@ -543,3 +543,52 @@ func reverseStatus(status apiv1.ConditionStatus) apiv1.ConditionStatus { return apiv1.ConditionFalse } } + +func TestConditionsMatch(t *testing.T) { + conditions1 := []toolchainv1alpha1.Condition{ + { + Type: toolchainv1alpha1.UserSignupComplete, + Status: apiv1.ConditionTrue, + Reason: toolchainv1alpha1.UserSignupApprovedAutomaticallyReason, + Message: "", + }, + { + Type: toolchainv1alpha1.UserSignupUserDeactivatingNotificationCreated, + Status: apiv1.ConditionTrue, + Reason: toolchainv1alpha1.UserSignupUserDeactivatingReason, + Message: "", + }, + } + conditions2 := []toolchainv1alpha1.Condition{ + { + Type: toolchainv1alpha1.UserSignupComplete, + Status: apiv1.ConditionTrue, + Reason: toolchainv1alpha1.UserSignupApprovedAutomaticallyReason, + Message: "", + }, + { + Type: toolchainv1alpha1.UserSignupUserDeactivatingNotificationCreated, + Status: apiv1.ConditionFalse, + Reason: "Foo", + Message: "", + }, + } + + // The conditions are different so no match + require.False(t, condition.ConditionsMatch(conditions1, conditions2)) + + // Update the condition so that they now match + conditions2[1].Status = apiv1.ConditionTrue + conditions2[1].Reason = toolchainv1alpha1.UserSignupUserDeactivatingReason + require.True(t, condition.ConditionsMatch(conditions1, conditions2)) + + // Add another condition, now they should not match + conditions2 = append(conditions2, toolchainv1alpha1.Condition{ + Type: toolchainv1alpha1.UserSignupUserDeactivatedNotificationCreated, + Status: apiv1.ConditionTrue, + Reason: toolchainv1alpha1.UserSignupUserDeactivatedReason, + Message: "", + }) + + require.False(t, condition.ConditionsMatch(conditions1, conditions2)) +} From f5d26685c18f0bcfbc5945fdbaddbfd5b29e1ebb Mon Sep 17 00:00:00 2001 From: Shane Bryzak Date: Fri, 3 May 2024 20:22:40 +1000 Subject: [PATCH 3/7] more coverage --- pkg/condition/condition_blackbox_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/pkg/condition/condition_blackbox_test.go b/pkg/condition/condition_blackbox_test.go index 0b33d40f..868e70de 100644 --- a/pkg/condition/condition_blackbox_test.go +++ b/pkg/condition/condition_blackbox_test.go @@ -591,4 +591,10 @@ func TestConditionsMatch(t *testing.T) { }) require.False(t, condition.ConditionsMatch(conditions1, conditions2)) + + conditions3 := []toolchainv1alpha1.Condition{} + conditions3 = append(conditions3, conditions2[0]) + conditions3 = append(conditions3, conditions2[2]) + + require.False(t, condition.ConditionsMatch(conditions1, conditions3)) } From 2c8876fc4b8e3a07b59804f531cf9e08ef33ad4b Mon Sep 17 00:00:00 2001 From: Shane Bryzak Date: Mon, 6 May 2024 08:21:42 +1000 Subject: [PATCH 4/7] refactor --- pkg/test/condition.go | 23 +++-------------------- 1 file changed, 3 insertions(+), 20 deletions(-) diff --git a/pkg/test/condition.go b/pkg/test/condition.go index 580b7dc1..482b258b 100644 --- a/pkg/test/condition.go +++ b/pkg/test/condition.go @@ -2,6 +2,7 @@ package test import ( "fmt" + "github.com/codeready-toolchain/toolchain-common/pkg/condition" "time" toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" @@ -56,29 +57,11 @@ func AssertTimestampsAreRecent(t T, conditions []toolchainv1alpha1.Condition) { // ConditionsMatch returns true if the specified list A of conditions is equal to specified // list B of conditions ignoring the order of the elements func ConditionsMatch(actual []toolchainv1alpha1.Condition, expected ...toolchainv1alpha1.Condition) bool { - if len(expected) != len(actual) { - return false - } - for _, c := range expected { - if !ContainsCondition(actual, c) { - return false - } - } - for _, c := range actual { - if !ContainsCondition(expected, c) { - return false - } - } - return true + return condition.ConditionsMatch(actual, expected) } // ContainsCondition returns true if the specified list of conditions contains the specified condition. // LastTransitionTime is ignored. func ContainsCondition(conditions []toolchainv1alpha1.Condition, contains toolchainv1alpha1.Condition) bool { - for _, c := range conditions { - if c.Type == contains.Type { - return contains.Status == c.Status && contains.Reason == c.Reason && contains.Message == c.Message - } - } - return false + return condition.ContainsCondition(conditions, contains) } From 7fbaad79dece66dde275e37af2f75551c1d1a648 Mon Sep 17 00:00:00 2001 From: Shane Bryzak Date: Wed, 8 May 2024 18:50:45 +1000 Subject: [PATCH 5/7] refactor --- pkg/condition/condition.go | 20 ++++++++++++++++---- pkg/test/condition.go | 24 +++++++++++++++++++----- 2 files changed, 35 insertions(+), 9 deletions(-) diff --git a/pkg/condition/condition.go b/pkg/condition/condition.go index 72acb1c4..e322f88e 100644 --- a/pkg/condition/condition.go +++ b/pkg/condition/condition.go @@ -147,7 +147,8 @@ func HasConditionReason(conditions []toolchainv1alpha1.Condition, conditionType return found && con.Reason == reason } -// ConditionsMatch checks whether the specified conditions match and return true if they do +// ConditionsMatch checks whether the specified conditions match and return true if they do, ignoring the value of the +// message property. func ConditionsMatch(first, second []toolchainv1alpha1.Condition) bool { if len(first) != len(second) { return false @@ -165,13 +166,24 @@ func ConditionsMatch(first, second []toolchainv1alpha1.Condition) bool { return true } -// ContainsCondition returns true if the specified list of conditions contains the specified condition and the statuses of the conditions match. -// LastTransitionTime is ignored. -func ContainsCondition(conditions []toolchainv1alpha1.Condition, contains toolchainv1alpha1.Condition) bool { +func containsConditions(conditions []toolchainv1alpha1.Condition, contains toolchainv1alpha1.Condition, ignoreMessage bool) bool { for _, c := range conditions { if c.Type == contains.Type { + if ignoreMessage { + return contains.Status == c.Status && contains.Reason == c.Reason + } return contains.Status == c.Status && contains.Reason == c.Reason && contains.Message == c.Message } } return false } + +// ContainsCondition returns true if the specified list of conditions contains the specified condition and the statuses of the conditions match. +// LastTransitionTime is ignored. +func ContainsCondition(conditions []toolchainv1alpha1.Condition, contains toolchainv1alpha1.Condition) bool { + return containsConditions(conditions, contains, true) +} + +func ContainsConditionWithMessage(conditions []toolchainv1alpha1.Condition, contains toolchainv1alpha1.Condition) bool { + return containsConditions(conditions, contains, false) +} diff --git a/pkg/test/condition.go b/pkg/test/condition.go index 482b258b..77512555 100644 --- a/pkg/test/condition.go +++ b/pkg/test/condition.go @@ -54,14 +54,28 @@ func AssertTimestampsAreRecent(t T, conditions []toolchainv1alpha1.Condition) { } } -// ConditionsMatch returns true if the specified list A of conditions is equal to specified -// list B of conditions ignoring the order of the elements -func ConditionsMatch(actual []toolchainv1alpha1.Condition, expected ...toolchainv1alpha1.Condition) bool { - return condition.ConditionsMatch(actual, expected) +// ConditionsMatch returns true if the specified list of conditions (first) is equal to the other specified +// list of conditions (second) ignoring the order of the elements, and comparing the message properties also. +// Note that the alternative ConditionsMatch function in the condition package does *NOT* compare the message properties +func ConditionsMatch(first []toolchainv1alpha1.Condition, second ...toolchainv1alpha1.Condition) bool { + if len(first) != len(second) { + return false + } + for _, c := range first { + if !condition.ContainsConditionWithMessage(second, c) { + return false + } + } + for _, c := range second { + if !condition.ContainsConditionWithMessage(first, c) { + return false + } + } + return true } // ContainsCondition returns true if the specified list of conditions contains the specified condition. // LastTransitionTime is ignored. func ContainsCondition(conditions []toolchainv1alpha1.Condition, contains toolchainv1alpha1.Condition) bool { - return condition.ContainsCondition(conditions, contains) + return condition.ContainsConditionWithMessage(conditions, contains) } From 5f159eae3e3afa0599a1f1ba1189d28363fd2493 Mon Sep 17 00:00:00 2001 From: Shane Bryzak Date: Wed, 8 May 2024 18:52:29 +1000 Subject: [PATCH 6/7] remove second pass over condition arrays --- pkg/condition/condition.go | 5 ----- pkg/test/condition.go | 5 ----- 2 files changed, 10 deletions(-) diff --git a/pkg/condition/condition.go b/pkg/condition/condition.go index e322f88e..289336e1 100644 --- a/pkg/condition/condition.go +++ b/pkg/condition/condition.go @@ -158,11 +158,6 @@ func ConditionsMatch(first, second []toolchainv1alpha1.Condition) bool { return false } } - for _, c := range second { - if !ContainsCondition(first, c) { - return false - } - } return true } diff --git a/pkg/test/condition.go b/pkg/test/condition.go index 77512555..e453a068 100644 --- a/pkg/test/condition.go +++ b/pkg/test/condition.go @@ -66,11 +66,6 @@ func ConditionsMatch(first []toolchainv1alpha1.Condition, second ...toolchainv1a return false } } - for _, c := range second { - if !condition.ContainsConditionWithMessage(first, c) { - return false - } - } return true } From 783e16fb2f02f4cf7e84db76e927f823f627beac Mon Sep 17 00:00:00 2001 From: Shane Bryzak Date: Thu, 9 May 2024 13:52:27 +1000 Subject: [PATCH 7/7] comments, added test --- pkg/condition/condition.go | 6 ++++- pkg/condition/condition_blackbox_test.go | 32 ++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/pkg/condition/condition.go b/pkg/condition/condition.go index 289336e1..8a7a5f60 100644 --- a/pkg/condition/condition.go +++ b/pkg/condition/condition.go @@ -173,12 +173,16 @@ func containsConditions(conditions []toolchainv1alpha1.Condition, contains toolc return false } -// ContainsCondition returns true if the specified list of conditions contains the specified condition and the statuses of the conditions match. +// ContainsCondition returns true if the specified list of conditions contains the specified condition and the statuses +// of the conditions match. This function does not compare the values of the message property. // LastTransitionTime is ignored. func ContainsCondition(conditions []toolchainv1alpha1.Condition, contains toolchainv1alpha1.Condition) bool { return containsConditions(conditions, contains, true) } +// ContainsConditionWithMessage returns true if the specified list of conditions contains the specified condition and the +// statuses and the message property of the conditions match. +// LastTransitionTime is ignored. func ContainsConditionWithMessage(conditions []toolchainv1alpha1.Condition, contains toolchainv1alpha1.Condition) bool { return containsConditions(conditions, contains, false) } diff --git a/pkg/condition/condition_blackbox_test.go b/pkg/condition/condition_blackbox_test.go index 868e70de..f6cc2ce9 100644 --- a/pkg/condition/condition_blackbox_test.go +++ b/pkg/condition/condition_blackbox_test.go @@ -544,6 +544,38 @@ func reverseStatus(status apiv1.ConditionStatus) apiv1.ConditionStatus { } } +func TestContainsConditionWithMessage(t *testing.T) { + conditions1 := []toolchainv1alpha1.Condition{ + { + Type: toolchainv1alpha1.UserSignupComplete, + Status: apiv1.ConditionTrue, + Reason: toolchainv1alpha1.UserSignupApprovedAutomaticallyReason, + Message: "foo", + }, + { + Type: toolchainv1alpha1.UserSignupUserDeactivatingNotificationCreated, + Status: apiv1.ConditionTrue, + Reason: toolchainv1alpha1.UserSignupUserDeactivatingReason, + Message: "bar", + }, + } + require.True(t, condition.ContainsConditionWithMessage(conditions1, + toolchainv1alpha1.Condition{ + Type: toolchainv1alpha1.UserSignupUserDeactivatingNotificationCreated, + Status: apiv1.ConditionTrue, + Reason: toolchainv1alpha1.UserSignupUserDeactivatingReason, + Message: "bar", + })) + + require.False(t, condition.ContainsConditionWithMessage(conditions1, + toolchainv1alpha1.Condition{ + Type: toolchainv1alpha1.UserSignupUserDeactivatingNotificationCreated, + Status: apiv1.ConditionTrue, + Reason: toolchainv1alpha1.UserSignupUserDeactivatingReason, + Message: "foo", + })) +} + func TestConditionsMatch(t *testing.T) { conditions1 := []toolchainv1alpha1.Condition{ {