From fefca7d9e2debc7db25ad72a5620ed0243ead27d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=A5=9A=E5=B2=B3?= Date: Thu, 28 Jul 2022 16:38:05 +0800 Subject: [PATCH] fix comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: 楚岳 --- pkg/addon/addon.go | 4 ++-- pkg/addon/addon_suite_test.go | 10 +++++----- pkg/addon/addon_test.go | 17 +++++++++++++++++ pkg/addon/utils.go | 25 +++++++++++++++++++++---- references/cli/addon.go | 4 ++-- 5 files changed, 47 insertions(+), 13 deletions(-) diff --git a/pkg/addon/addon.go b/pkg/addon/addon.go index fc9e7d5e9..7ee65829f 100644 --- a/pkg/addon/addon.go +++ b/pkg/addon/addon.go @@ -1071,12 +1071,12 @@ func (h *Installer) dispatchAddonResource(addon *InstallPackage) error { } if !h.overrideDefs { - existDefs, err := checkConflictDefs(h.ctx, h.cli, defs, *app) + existDefs, err := checkConflictDefs(h.ctx, h.cli, defs, app.Name) if err != nil { return err } if len(existDefs) != 0 { - return fmt.Errorf(`definitions: %s in this addon already exist, if you want override them, Please use add "--override-defs" to re-enable or re-upgrade`, existDefs) + return produceDefConflictError(existDefs) } } diff --git a/pkg/addon/addon_suite_test.go b/pkg/addon/addon_suite_test.go index a3409e226..b996ca051 100644 --- a/pkg/addon/addon_suite_test.go +++ b/pkg/addon/addon_suite_test.go @@ -418,17 +418,17 @@ var _ = Describe("test override defs of addon", func() { u := unstructured.Unstructured{Object: compUnstructured} u.SetAPIVersion(v1beta1.SchemeGroupVersion.String()) u.SetKind(v1beta1.ComponentDefinitionKind) - c, err := checkConflictDefs(ctx, k8sClient, []*unstructured.Unstructured{&u}, app) + c, err := checkConflictDefs(ctx, k8sClient, []*unstructured.Unstructured{&u}, app.GetName()) Expect(err).Should(BeNil()) Expect(len(c)).Should(BeEquivalentTo(1)) u.SetName("rollout") - c, err = checkConflictDefs(ctx, k8sClient, []*unstructured.Unstructured{&u}, app) + c, err = checkConflictDefs(ctx, k8sClient, []*unstructured.Unstructured{&u}, app.GetName()) Expect(err).Should(BeNil()) Expect(len(c)).Should(BeEquivalentTo(0)) u.SetKind("NotExistKind") - _, err = checkConflictDefs(ctx, k8sClient, []*unstructured.Unstructured{&u}, app) + _, err = checkConflictDefs(ctx, k8sClient, []*unstructured.Unstructured{&u}, app.GetName()) Expect(err).ShouldNot(BeNil()) compUnstructured2, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&comp2) @@ -436,7 +436,7 @@ var _ = Describe("test override defs of addon", func() { u2 := &unstructured.Unstructured{Object: compUnstructured2} u2.SetAPIVersion(v1beta1.SchemeGroupVersion.String()) u2.SetKind(v1beta1.ComponentDefinitionKind) - c, err = checkConflictDefs(ctx, k8sClient, []*unstructured.Unstructured{u2}, app) + c, err = checkConflictDefs(ctx, k8sClient, []*unstructured.Unstructured{u2}, app.GetName()) Expect(err).Should(BeNil()) Expect(len(c)).Should(BeEquivalentTo(1)) @@ -445,7 +445,7 @@ var _ = Describe("test override defs of addon", func() { u3 := &unstructured.Unstructured{Object: compUnstructured3} u3.SetAPIVersion(v1beta1.SchemeGroupVersion.String()) u3.SetKind(v1beta1.ComponentDefinitionKind) - c, err = checkConflictDefs(ctx, k8sClient, []*unstructured.Unstructured{u3}, app) + c, err = checkConflictDefs(ctx, k8sClient, []*unstructured.Unstructured{u3}, app.GetName()) Expect(err).Should(BeNil()) Expect(len(c)).Should(BeEquivalentTo(0)) }) diff --git a/pkg/addon/addon_test.go b/pkg/addon/addon_test.go index aa34b633b..01257e518 100644 --- a/pkg/addon/addon_test.go +++ b/pkg/addon/addon_test.go @@ -950,6 +950,11 @@ func TestCheckSemVer(t *testing.T) { require: ">=1.5.0", res: false, }, + { + actual: "1.5.0-alpha.2", + require: ">=1.5.0", + res: false, + }, } for _, testCase := range testCases { result, err := checkSemVer(testCase.actual, testCase.require) @@ -1311,3 +1316,15 @@ func TestMergeAddonInstallArgs(t *testing.T) { } } + +func TestGenerateConflictError(t *testing.T) { + confictAddon := map[string]string{ + "helm": "definition: helm already exist and not belong to any addon \n", + "kustomize": "definition: %s in this addon already exist in fluxcd \n", + } + err := produceDefConflictError(confictAddon) + assert.Error(t, err) + strings.Contains(err.Error(), "in this addon already exist in fluxcd") + + assert.NoError(t, produceDefConflictError(map[string]string{})) +} diff --git a/pkg/addon/utils.go b/pkg/addon/utils.go index b56751521..4d062c09c 100644 --- a/pkg/addon/utils.go +++ b/pkg/addon/utils.go @@ -38,6 +38,7 @@ import ( "github.com/oam-dev/kubevela/pkg/definition" "github.com/oam-dev/kubevela/pkg/oam" "github.com/oam-dev/kubevela/pkg/oam/util" + "github.com/oam-dev/kubevela/pkg/utils/addon" "github.com/oam-dev/kubevela/pkg/utils/common" ) @@ -440,15 +441,19 @@ func isErrorCueRenderPathNotFound(err error, path string) bool { return err.Error() == fmt.Sprintf("var(path=%s) not exist", path) } -func checkConflictDefs(ctx context.Context, k8sClient client.Client, defs []*unstructured.Unstructured, app v1beta1.Application) ([]string, error) { - var res []string +func checkConflictDefs(ctx context.Context, k8sClient client.Client, defs []*unstructured.Unstructured, appName string) (map[string]string, error) { + res := map[string]string{} for _, def := range defs { err := k8sClient.Get(ctx, client.ObjectKeyFromObject(def), def) if err == nil { owner := metav1.GetControllerOf(def) - if owner == nil || owner.Kind != v1beta1.ApplicationKind || owner.Name != app.Name { + if owner == nil || owner.Kind != v1beta1.ApplicationKind { + res[def.GetName()] = fmt.Sprintf("definition: %s already exist and not belong to any addon \n", def.GetName()) + continue + } + if owner.Name != appName { // if addon not belong to an addon or addon name is another one, we should put them in result - res = append(res, def.GetName()) + res[def.GetName()] = fmt.Sprintf("definition: %s in this addon already exist in %s \n", def.GetName(), addon.AppName2Addon(appName)) } } if err != nil && !errors2.IsNotFound(err) { @@ -457,3 +462,15 @@ func checkConflictDefs(ctx context.Context, k8sClient client.Client, defs []*uns } return res, nil } + +func produceDefConflictError(conflictDefs map[string]string) error { + if len(conflictDefs) == 0 { + return nil + } + var errorInfo string + for _, s := range conflictDefs { + errorInfo += s + } + errorInfo += "if you want override them, Please use arguments '--override' to enable \n" + return errors.New(errorInfo) +} diff --git a/references/cli/addon.go b/references/cli/addon.go index 2f4c920ee..f1a583de5 100644 --- a/references/cli/addon.go +++ b/references/cli/addon.go @@ -205,7 +205,7 @@ func NewAddonEnableCommand(c common.Args, ioStream cmdutil.IOStreams) *cobra.Com cmd.Flags().StringVarP(&addonVersion, "version", "v", "", "specify the addon version to enable") cmd.Flags().StringVarP(&addonClusters, types.ClustersArg, "c", "", "specify the runtime-clusters to enable") cmd.Flags().BoolVarP(&skipValidate, "skip-version-validating", "s", false, "skip validating system version requirement") - cmd.Flags().BoolVarP(&overrideDefs, "override-defs", "o", false, "override already exist definitions within addon") + cmd.Flags().BoolVarP(&overrideDefs, "override-definitions", "", false, "override existing definitions if conflict with those contained in this addon") return cmd } @@ -326,7 +326,7 @@ non-empty new arg } cmd.Flags().StringVarP(&addonVersion, "version", "v", "", "specify the addon version to upgrade") cmd.Flags().BoolVarP(&skipValidate, "skip-version-validating", "s", false, "skip validating system version requirement") - cmd.Flags().BoolVarP(&overrideDefs, "override-defs", "o", false, "override already exist definitions within addon") + cmd.Flags().BoolVarP(&overrideDefs, "override-definitions", "", false, "override existing definitions if conflict with those contained in this addon") return cmd }