fix(traits): use Comparable matches

Reverting apache#4512 which introduced a function diverging the match from the original design

docs/modules/ROOT/pages/architecture/traits.adoc

@@ -49,6 +49,15 @@ type Trait interface {
 	IsAllowedInProfile(v1.TraitProfile) bool
 	Order() int
 }
+
+type Comparable interface {
+	Matches(trait Trait) bool
+}
+
+type ComparableTrait interface {
+	Trait
+	Comparable
+}
 ----
 
 Each trait will implement this interface. The most important methods that will be invoked by the xref:architecture/operator.adoc[Operator] are `Configure()` and `Apply()`. Basically, the `Configure()` method will set those inputs aforementioned (each trait has its own). The method is in charge to verify also the correctness of those expected parameters, where it makes sense (i.e., a well expected `Kubernetes` resource name). The function can return a `TraitCondition` object containing any informative or warning condition to be attached to the resulting Integration (for instance, traits forcefully altered by the platform, or deprecation notices).
@@ -59,6 +68,6 @@ The `Order()` method helps in resolving the order of execution of different trai
 
 The `InfluencesKit()`, `IsPlatformTrait()` and `RequiresIntegrationPlatform()` methods are easy to understand. They are used to determine if a trait has to influence an `IntegrationKit` build/initialization, if it's a platform trait (ie, needed by the platform itself) or are requiring the presence of an `IntegrationPlatform`.
 
-The presence of `InfluencesBuild()` will let specify the level of granularity of a trait down to its properties for a rebuild. So, if you need, you can compare the traits properties coming from the `prev` (previous) Integration to decide if it is worth to rebuild an Integration or the trait can reuse the one already provided in `this` version.
+For those traits that `InfluencesKit()` you may need to provide a `Matches(trait Trait)` func in order to specify those trait parameters that influences a build. This is required by the platform to decide if it is worth to rebuild an Integration or the trait can reuse the one already provided.
 
 Finally, through the `IsAllowedInProfile()` method we can override the default behavior (allow the trait for any profile). We must specify the profile we expect for this trait to be executed properly.

e2e/common/misc/pipe_test.go

@@ -63,7 +63,7 @@ func TestPipe(t *testing.T) {
 					"--name", "throw-error-binding",
 				).Execute()).To(Succeed())
 
-				g.Eventually(IntegrationPodPhase(t, ns, "throw-error-binding"), TestTimeoutLong).Should(Equal(corev1.PodRunning))
+				g.Eventually(IntegrationPodPhase(t, ns, "throw-error-binding"), TestTimeoutMedium).Should(Equal(corev1.PodRunning))
 				g.Eventually(IntegrationLogs(t, ns, "throw-error-binding"), TestTimeoutShort).Should(ContainSubstring("[kameletErrorHandler] (Camel (camel-1) thread #1 - timer://tick)"))
 				g.Eventually(IntegrationLogs(t, ns, "throw-error-binding"), TestTimeoutShort).ShouldNot(ContainSubstring("[integrationLogger] (Camel (camel-1) thread #1 - timer://tick)"))
 
@@ -82,7 +82,7 @@ func TestPipe(t *testing.T) {
 					"--name", "no-error-binding",
 				).Execute()).To(Succeed())
 
-				g.Eventually(IntegrationPodPhase(t, ns, "no-error-binding"), TestTimeoutLong).Should(Equal(corev1.PodRunning))
+				g.Eventually(IntegrationPodPhase(t, ns, "no-error-binding"), TestTimeoutMedium).Should(Equal(corev1.PodRunning))
 				g.Eventually(IntegrationLogs(t, ns, "no-error-binding"), TestTimeoutShort).ShouldNot(ContainSubstring("[kameletErrorHandler] (Camel (camel-1) thread #1 - timer://tick)"))
 				g.Eventually(IntegrationLogs(t, ns, "no-error-binding"), TestTimeoutShort).Should(ContainSubstring("[integrationLogger] (Camel (camel-1) thread #1 - timer://tick)"))
 
@@ -103,7 +103,7 @@ func TestPipe(t *testing.T) {
 				"--name", "kb-with-traits",
 			).Execute()).To(Succeed())
 
-			g.Eventually(IntegrationPodPhase(t, ns, "kb-with-traits"), TestTimeoutLong).Should(Equal(corev1.PodRunning))
+			g.Eventually(IntegrationPodPhase(t, ns, "kb-with-traits"), TestTimeoutMedium).Should(Equal(corev1.PodRunning))
 			g.Eventually(IntegrationLogs(t, ns, "kb-with-traits"), TestTimeoutShort).Should(ContainSubstring("hello from test"))
 			g.Eventually(IntegrationLogs(t, ns, "kb-with-traits"), TestTimeoutShort).Should(ContainSubstring("integrationLogger"))
 		})

e2e/common/traits/builder_test.go

@@ -30,7 +30,6 @@ import (
 
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 
 	. "github.com/apache/camel-k/v2/e2e/support"
 	v1 "github.com/apache/camel-k/v2/pkg/apis/camel/v1"
@@ -78,6 +77,8 @@ func TestBuilderTrait(t *testing.T) {
 			name := RandomizedSuffixName("java-dependencies-strategy")
 			g.Expect(KamelRunWithID(t, operatorID, ns, "files/Java.java",
 				"--name", name,
+				// This is required in order to avoid reusing a Kit already existing (which is the default behavior)
+				"--build-property", "strategy=dependencies",
 				"-t", "builder.order-strategy=dependencies").Execute()).To(Succeed())
 
 			g.Eventually(IntegrationPodPhase(t, ns, name), TestTimeoutLong).Should(Equal(corev1.PodRunning))
@@ -94,24 +95,17 @@ func TestBuilderTrait(t *testing.T) {
 			g.Eventually(BuildConfig(t, integrationKitNamespace, integrationKitName)().LimitCPU, TestTimeoutShort).Should(Equal(""))
 			g.Eventually(BuildConfig(t, integrationKitNamespace, integrationKitName)().RequestMemory, TestTimeoutShort).Should(Equal(""))
 			g.Eventually(BuildConfig(t, integrationKitNamespace, integrationKitName)().LimitMemory, TestTimeoutShort).Should(Equal(""))
-
 			g.Eventually(BuilderPod(t, integrationKitNamespace, builderKitName), TestTimeoutShort).Should(BeNil())
 
-			// check integration schema does not contains unwanted default trait value.
-			g.Eventually(UnstructuredIntegration(t, ns, name)).ShouldNot(BeNil())
-			unstructuredIntegration := UnstructuredIntegration(t, ns, name)()
-			builderTrait, _, _ := unstructured.NestedMap(unstructuredIntegration.Object, "spec", "traits", "builder")
-			g.Expect(builderTrait).NotTo(BeNil())
-			g.Expect(len(builderTrait)).To(Equal(1))
-			g.Expect(builderTrait["orderStrategy"]).To(Equal("dependencies"))
-
 			g.Expect(Kamel(t, "delete", "--all", "-n", ns).Execute()).To(Succeed())
 		})
 
 		t.Run("Run build order strategy fifo", func(t *testing.T) {
 			name := RandomizedSuffixName("java-fifo-strategy")
 			g.Expect(KamelRunWithID(t, operatorID, ns, "files/Java.java",
 				"--name", name,
+				// This is required in order to avoid reusing a Kit already existing (which is the default behavior)
+				"--build-property", "strategy=fifo",
 				"-t", "builder.order-strategy=fifo").Execute()).To(Succeed())
 
 			g.Eventually(IntegrationPodPhase(t, ns, name), TestTimeoutLong).Should(Equal(corev1.PodRunning))
@@ -138,6 +132,8 @@ func TestBuilderTrait(t *testing.T) {
 			name := RandomizedSuffixName("java-resource-config")
 			g.Expect(KamelRunWithID(t, operatorID, ns, "files/Java.java",
 				"--name", name,
+				// This is required in order to avoid reusing a Kit already existing (which is the default behavior)
+				"--build-property", "resources=new-build",
 				"-t", "builder.tasks-request-cpu=builder:500m",
 				"-t", "builder.tasks-limit-cpu=builder:1000m",
 				"-t", "builder.tasks-request-memory=builder:2Gi",

pkg/controller/integration/kits.go

@@ -19,8 +19,6 @@ package integration
 
 import (
 	"context"
-	"fmt"
-	"reflect"
 
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/labels"
@@ -132,7 +130,7 @@ func integrationMatches(ctx context.Context, c client.Client, integration *v1.In
 		return false, err
 	}
 
-	if match, err := hasMatchingTraits(itc, ikc); !match || err != nil {
+	if match, err := trait.HasMatchingTraits(itc, ikc); !match || err != nil {
 		ilog.Debug("Integration and integration-kit traits do not match", "integration", integration.Name, "integration-kit", kit.Name, "namespace", integration.Namespace)
 		return false, err
 	}
@@ -195,7 +193,7 @@ func kitMatches(kit *v1.IntegrationKit, target *v1.IntegrationKit) (bool, error)
 		return false, err
 	}
 
-	if match, err := hasMatchingTraits(c1, c2); !match || err != nil {
+	if match, err := trait.HasMatchingTraits(c1, c2); !match || err != nil {
 		return false, err
 	}
 	if !util.StringSliceContains(kit.Spec.Dependencies, target.Spec.Dependencies) {
@@ -205,67 +203,6 @@ func kitMatches(kit *v1.IntegrationKit, target *v1.IntegrationKit) (bool, error)
 	return true, nil
 }
 
-func hasMatchingTraits(traitMap trait.Options, kitTraitMap trait.Options) (bool, error) {
-	catalog := trait.NewCatalog(nil)
-
-	for _, t := range catalog.AllTraits() {
-		if t == nil {
-			continue
-		}
-
-		id := string(t.ID())
-		it, ok1 := traitMap.Get(id)
-		kt, ok2 := kitTraitMap.Get(id)
-
-		if !ok1 && !ok2 {
-			continue
-		}
-
-		if t.InfluencesKit() && t.InfluencesBuild(it, kt) {
-			if ct, ok := t.(trait.ComparableTrait); ok {
-				// if it's match trait use its matches method to determine the match
-				if match, err := matchesComparableTrait(ct, it, kt); !match || err != nil {
-					return false, err
-				}
-			} else {
-				if !matchesTrait(it, kt) {
-					return false, nil
-				}
-			}
-		}
-	}
-
-	return true, nil
-}
-
-func matchesComparableTrait(ct trait.ComparableTrait, it map[string]interface{}, kt map[string]interface{}) (bool, error) {
-	t1 := reflect.New(reflect.TypeOf(ct).Elem()).Interface()
-	if err := trait.ToTrait(it, &t1); err != nil {
-		return false, err
-	}
-
-	t2 := reflect.New(reflect.TypeOf(ct).Elem()).Interface()
-	if err := trait.ToTrait(kt, &t2); err != nil {
-		return false, err
-	}
-
-	ct2, ok := t2.(trait.ComparableTrait)
-	if !ok {
-		return false, fmt.Errorf("type assertion failed: %v", t2)
-	}
-	tt1, ok := t1.(trait.Trait)
-	if !ok {
-		return false, fmt.Errorf("type assertion failed: %v", t1)
-	}
-
-	return ct2.Matches(tt1), nil
-}
-
-func matchesTrait(it map[string]interface{}, kt map[string]interface{}) bool {
-	// perform exact match on the two trait maps
-	return reflect.DeepEqual(it, kt)
-}
-
 func hasMatchingSourcesForNative(it *v1.Integration, kit *v1.IntegrationKit) bool {
 	if len(it.UserDefinedSources()) != len(kit.Spec.Sources) {
 		return false

pkg/controller/integration/kits_test.go

@@ -27,7 +27,6 @@ import (
 	traitv1 "github.com/apache/camel-k/v2/pkg/apis/camel/v1/trait"
 
 	"github.com/apache/camel-k/v2/pkg/trait"
-	"github.com/apache/camel-k/v2/pkg/util/log"
 	"github.com/apache/camel-k/v2/pkg/util/test"
 
 	"github.com/stretchr/testify/assert"
@@ -94,10 +93,6 @@ func TestLookupKitForIntegration_DiscardKitsInError(t *testing.T) {
 
 	require.NoError(t, err)
 
-	a := buildKitAction{}
-	a.InjectLogger(log.Log)
-	a.InjectClient(c)
-
 	kits, err := lookupKitsForIntegration(context.TODO(), c, &v1.Integration{
 		TypeMeta: metav1.TypeMeta{
 			APIVersion: v1.SchemeGroupVersion.String(),
@@ -221,10 +216,6 @@ func TestLookupKitForIntegration_DiscardKitsWithIncompatibleTraits(t *testing.T)
 
 	require.NoError(t, err)
 
-	a := buildKitAction{}
-	a.InjectLogger(log.Log)
-	a.InjectClient(c)
-
 	kits, err := lookupKitsForIntegration(context.TODO(), c, &v1.Integration{
 		TypeMeta: metav1.TypeMeta{
 			APIVersion: v1.SchemeGroupVersion.String(),
@@ -288,9 +279,6 @@ func TestHasMatchingTraits_KitNoTraitShouldNotBePicked(t *testing.T) {
 		},
 	}
 
-	a := buildKitAction{}
-	a.InjectLogger(log.Log)
-
 	ok, err := integrationAndKitHaveSameTraits(integration, kit)
 	require.NoError(t, err)
 	assert.False(t, ok)
@@ -339,9 +327,6 @@ func TestHasMatchingTraits_KitSameTraitShouldBePicked(t *testing.T) {
 		},
 	}
 
-	a := buildKitAction{}
-	a.InjectLogger(log.Log)
-
 	ok, err := integrationAndKitHaveSameTraits(integration, kit)
 	require.NoError(t, err)
 	assert.True(t, ok)

pkg/controller/integration/monitor.go

@@ -237,7 +237,7 @@ func (action *monitorAction) checkDigestAndRebuild(ctx context.Context, integrat
 	}
 
 	if hash != integration.Status.Digest {
-		action.L.Info("Integration %s digest has changed: resetting its status. Will check if it needs to be rebuilt and restarted.", integration.Name)
+		action.L.Infof("Integration %s digest has changed: resetting its status. Will check if it needs to be rebuilt and restarted.", integration.Name)
 		if isIntegrationKitResetRequired(integration, kit) {
 			integration.SetIntegrationKit(nil)
 		}

pkg/trait/builder.go

@@ -20,6 +20,7 @@ package trait
 import (
 	"fmt"
 	"regexp"
+	"slices"
 	"sort"
 	"strconv"
 	"strings"
@@ -56,9 +57,33 @@ func (t *builderTrait) InfluencesKit() bool {
 	return true
 }
 
-// InfluencesBuild overrides base class method.
-func (t *builderTrait) InfluencesBuild(this, prev map[string]interface{}) bool {
-	return true
+func (t *builderTrait) Matches(trait Trait) bool {
+	otherTrait, ok := trait.(*builderTrait)
+	if !ok {
+		return false
+	}
+	if t.BaseImage != otherTrait.BaseImage || len(t.Properties) != len(otherTrait.Properties) || len(t.Tasks) != len(otherTrait.Tasks) {
+		return false
+	}
+	// More sofisticated check if len is the same. Sort and compare via slices equal func.
+	// Although the Matches func is used as a support for comparison, it makes sense
+	// to copy the properties and avoid possible inconsistencies caused by the sorting operation.
+	srtThisProps := make([]string, len(t.Properties))
+	srtOtheProps := make([]string, len(otherTrait.Properties))
+	copy(srtThisProps, t.Properties)
+	copy(srtOtheProps, otherTrait.Properties)
+	slices.Sort(srtThisProps)
+	slices.Sort(srtOtheProps)
+	if !slices.Equal(srtThisProps, srtOtheProps) {
+		return false
+	}
+	srtThisTasks := make([]string, len(t.Tasks))
+	srtOtheTasks := make([]string, len(otherTrait.Tasks))
+	copy(srtThisTasks, t.Tasks)
+	copy(srtOtheTasks, otherTrait.Tasks)
+	slices.Sort(srtThisTasks)
+	slices.Sort(srtOtheTasks)
+	return slices.Equal(srtThisTasks, srtOtheTasks)
 }
 
 func (t *builderTrait) Configure(e *Environment) (bool, *TraitCondition, error) {

pkg/trait/builder_test.go

@@ -28,6 +28,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	v1 "github.com/apache/camel-k/v2/pkg/apis/camel/v1"
+	traitv1 "github.com/apache/camel-k/v2/pkg/apis/camel/v1/trait"
 	"github.com/apache/camel-k/v2/pkg/util/camel"
 	"github.com/apache/camel-k/v2/pkg/util/defaults"
 	"github.com/apache/camel-k/v2/pkg/util/kubernetes"
@@ -603,3 +604,53 @@ func tasksByName(tasks []v1.Task) []string {
 	}
 	return pipelineTasks
 }
+
+func TestBuilderMatches(t *testing.T) {
+	t1 := builderTrait{
+		BasePlatformTrait: NewBasePlatformTrait("builder", 600),
+		BuilderTrait: traitv1.BuilderTrait{
+			OrderStrategy: "dependencies",
+		},
+	}
+	t2 := builderTrait{
+		BasePlatformTrait: NewBasePlatformTrait("builder", 600),
+		BuilderTrait: traitv1.BuilderTrait{
+			OrderStrategy: "dependencies",
+		},
+	}
+	assert.True(t, t1.Matches(&t2))
+	// This is a property that does not influence the build
+	t2.OrderStrategy = "fifo"
+	assert.True(t, t1.Matches(&t2))
+	// Changing properties which influences build
+	t1.Properties = []string{"hello=world"}
+	assert.False(t, t1.Matches(&t2))
+	t2.Properties = []string{"hello=world"}
+	assert.True(t, t1.Matches(&t2))
+	t1.Properties = []string{"hello=world", "weare=theworld"}
+	assert.False(t, t1.Matches(&t2))
+	// should detect swap
+	t2.Properties = []string{"weare=theworld", "hello=world"}
+	assert.True(t, t1.Matches(&t2))
+}
+
+func TestBuilderMatchesTasks(t *testing.T) {
+	t1 := builderTrait{
+		BasePlatformTrait: NewBasePlatformTrait("builder", 600),
+		BuilderTrait:      traitv1.BuilderTrait{},
+	}
+	t2 := builderTrait{
+		BasePlatformTrait: NewBasePlatformTrait("builder", 600),
+		BuilderTrait: traitv1.BuilderTrait{
+			Tasks: []string{"task1;my-task;do-something"},
+		},
+	}
+	t3 := builderTrait{
+		BasePlatformTrait: NewBasePlatformTrait("builder", 600),
+		BuilderTrait: traitv1.BuilderTrait{
+			Tasks: []string{"task1;my-task;do-something-else"},
+		},
+	}
+	assert.False(t, t1.Matches(&t2))
+	assert.False(t, t2.Matches(&t3))
+}

pkg/trait/camel.go

@@ -53,9 +53,13 @@ func (t *camelTrait) InfluencesKit() bool {
 	return true
 }
 
-// InfluencesBuild only when the runtime has changed.
-func (t *camelTrait) InfluencesBuild(this, prev map[string]interface{}) bool {
-	return this["runtimeVersion"] != prev["runtimeVersion"]
+func (t *camelTrait) Matches(trait Trait) bool {
+	otherTrait, ok := trait.(*camelTrait)
+	if !ok {
+		return false
+	}
+
+	return otherTrait.RuntimeVersion == t.RuntimeVersion
 }
 
 func (t *camelTrait) Configure(e *Environment) (bool, *TraitCondition, error) {