Fix(addon): show correct owner in definition conflict error (#6903)

* fix(addon): show correct owner in definition conflict error

When enabling an addon, if a definition conflicted with one from another existing addon, the error message would misleadingly cite the addon being installed as the owner, rather than the actual owner of the definition. This made it difficult for users to diagnose the conflict.

This commit corrects the error message generation in `checkConflictDefs` to use the name of the actual owner application. A comprehensive unit test for this function has also been added to verify the corrected behavior and prevent regressions.

Fixes #6898

Signed-off-by: Ashvin Bambhaniya <ashvin.bambhaniya@improwised.com>

* fix(addon): show correct owner name in conflict message

When a definition conflict occurs, the error message attempts to show the addon that owns the existing definition.

However, if the owner is not a KubeVela addon application (i.e., its name doesn't have the 'addon-' prefix), the `AppName2Addon` function returns an empty string. This resulted in a confusing conflict message with a blank owner name, like "already exist in  \n".

This patch fixes the issue by checking if the result of `AppName2Addon` is empty. If it is, it falls back to using the full application name of the owner,
ensuring the conflict message is always clear and actionable.

Signed-off-by: Ashvin Bambhaniya <ashvin.bambhaniya@improwised.com>

* chore(addon): update comment for addon name

- Add this comment to trigger ci

Signed-off-by: Ashvin Bambhaniya <ashvin.bambhaniya@improwised.com>

* fix(addon): improve conflict message for addon definitions

adjust comment placement and logic to ensure correct addon name display in conflict messages

Signed-off-by: Ashvin Bambhaniya <ashvin.bambhaniya@improwised.com>

---------

Signed-off-by: Ashvin Bambhaniya <ashvin.bambhaniya@improwised.com>

pkg/addon/utils.go

@@ -465,7 +465,12 @@ func checkConflictDefs(ctx context.Context, k8sClient client.Client, defs []*uns
 			}
 			if owner.Name != appName {
 				// if addon not belong to an addon or addon name is another one, we should put them in result
-				res[checkDef.GetName()] = fmt.Sprintf("definition: %s in this addon already exist in %s \n", checkDef.GetName(), addon.AppName2Addon(appName))
+				addonName := addon.AppName2Addon(owner.Name)
+				// If owner.Name isn't an addon app name, show the owner's name directly as the addon name
+				if addonName == "" {
+					addonName = owner.Name
+				}
+				res[checkDef.GetName()] = fmt.Sprintf("definition: %s in this addon already exist in %s \n", checkDef.GetName(), addonName)
 			}
 		}
 		if err != nil && !errors2.IsNotFound(err) {

pkg/addon/utils_test.go

@@ -117,6 +117,86 @@ var _ = Describe("Test definition check", func() {
 		Expect(err).Should(BeNil())
 		Expect(len(usedApps)).Should(BeEquivalentTo(4))
 	})
+
+	It("Test checkConflictDefs", func() {
+		const appName = "addon-fluxcd"
+		const otherAppName = "addon-other"
+		isController := true
+
+		// A definition that doesn't exist in the cluster
+		nonExistingDef := &unstructured.Unstructured{}
+		nonExistingDef.SetAPIVersion("core.oam.dev/v1beta1")
+		nonExistingDef.SetKind("ComponentDefinition")
+		nonExistingDef.SetName("non-existing-def")
+		nonExistingDef.SetNamespace(velatypes.DefaultKubeVelaNS)
+
+		// A definition that exists but has no owner
+		defWithNoOwner := &unstructured.Unstructured{}
+		defWithNoOwner.SetAPIVersion("core.oam.dev/v1beta1")
+		defWithNoOwner.SetKind("ComponentDefinition")
+		defWithNoOwner.SetName("def-no-owner")
+		defWithNoOwner.SetNamespace(velatypes.DefaultKubeVelaNS)
+		Expect(k8sClient.Create(ctx, defWithNoOwner)).Should(Succeed())
+		defer func() {
+			Expect(k8sClient.Delete(ctx, defWithNoOwner)).Should(Succeed())
+		}()
+
+		// A definition that is owned by another addon
+		defWithOtherOwner := &unstructured.Unstructured{}
+		defWithOtherOwner.SetAPIVersion("core.oam.dev/v1beta1")
+		defWithOtherOwner.SetKind("ComponentDefinition")
+		defWithOtherOwner.SetName("def-other-owner")
+		defWithOtherOwner.SetNamespace(velatypes.DefaultKubeVelaNS)
+		defWithOtherOwner.SetOwnerReferences([]metav1.OwnerReference{
+			{
+				APIVersion: v1beta1.SchemeGroupVersion.String(),
+				Kind:       v1beta1.ApplicationKind,
+				Name:       otherAppName,
+				Controller: &isController,
+				UID:        "test-uid-other",
+			},
+		})
+		Expect(k8sClient.Create(ctx, defWithOtherOwner)).Should(Succeed())
+		defer func() {
+			Expect(k8sClient.Delete(ctx, defWithOtherOwner)).Should(Succeed())
+		}()
+
+		// A definition that is owned by the same addon
+		defWithSameOwner := &unstructured.Unstructured{}
+		defWithSameOwner.SetAPIVersion("core.oam.dev/v1beta1")
+		defWithSameOwner.SetKind("ComponentDefinition")
+		defWithSameOwner.SetName("def-same-owner")
+		defWithSameOwner.SetNamespace(velatypes.DefaultKubeVelaNS)
+		defWithSameOwner.SetOwnerReferences([]metav1.OwnerReference{
+			{
+				APIVersion: v1beta1.SchemeGroupVersion.String(),
+				Kind:       v1beta1.ApplicationKind,
+				Name:       appName,
+				Controller: &isController,
+				UID:        "test-uid-same",
+			},
+		})
+		Expect(k8sClient.Create(ctx, defWithSameOwner)).Should(Succeed())
+		defer func() {
+			Expect(k8sClient.Delete(ctx, defWithSameOwner)).Should(Succeed())
+		}()
+
+		By("Checking a mix of definitions for conflicts")
+		defs := []*unstructured.Unstructured{
+			nonExistingDef,
+			defWithNoOwner,
+			defWithOtherOwner,
+			defWithSameOwner,
+		}
+
+		conflicts, err := checkConflictDefs(ctx, k8sClient, defs, appName)
+		Expect(err).Should(BeNil())
+		Expect(len(conflicts)).Should(Equal(2))
+		Expect(conflicts).Should(HaveKey(defWithNoOwner.GetName()))
+		Expect(conflicts[defWithNoOwner.GetName()]).Should(ContainSubstring("already exist and not belong to any addon"))
+		Expect(conflicts).Should(HaveKey(defWithOtherOwner.GetName()))
+		Expect(conflicts[defWithOtherOwner.GetName()]).Should(ContainSubstring("already exist in other"))
+	})
 })
 
 func TestMerge2Map(t *testing.T) {