Fix: more explicit error when addon package hasn't a metadata.yaml (#5298)

* more explicit error when addon package hasn't a metadata.yaml

Signed-off-by: 楚岳 <wangyike.wyk@alibaba-inc.com>

fix checkdiff

Signed-off-by: 楚岳 <wangyike.wyk@alibaba-inc.com>

* fix commets

Signed-off-by: 楚岳 <wangyike.wyk@alibaba-inc.com>

* fix test

Signed-off-by: 楚岳 <wangyike.wyk@alibaba-inc.com>

Signed-off-by: 楚岳 <wangyike.wyk@alibaba-inc.com>

pkg/addon/addon.go

@@ -911,7 +911,6 @@ func NewAddonInstaller(ctx context.Context, cli client.Client, discoveryClient *
 func (h *Installer) enableAddon(addon *InstallPackage) (string, error) {
 	var err error
 	h.addon = addon
-
 	if !h.skipVersionValidate {
 		err = checkAddonVersionMeetRequired(h.ctx, addon.SystemRequirements, h.cli, h.dc)
 		if err != nil {

pkg/addon/helper.go

@@ -58,6 +58,9 @@ func EnableAddon(ctx context.Context, name string, version string, cli client.Cl
 	if err != nil {
 		return "", err
 	}
+	if err := validateAddonPackage(pkg); err != nil {
+		return "", errors.Wrap(err, fmt.Sprintf("failed to enable addon: %s", name))
+	}
 	return h.enableAddon(pkg)
 }
 
@@ -106,6 +109,9 @@ func EnableAddonByLocalDir(ctx context.Context, name string, dir string, cli cli
 	if err != nil {
 		return "", err
 	}
+	if err := validateAddonPackage(pkg); err != nil {
+		return "", errors.Wrap(err, fmt.Sprintf("failed to enable addon by local dir: %s", dir))
+	}
 	h := NewAddonInstaller(ctx, cli, dc, applicator, config, &Registry{Name: LocalAddonRegistryName}, args, nil, nil, opts...)
 	needEnableAddonNames, err := h.checkDependency(pkg)
 	if err != nil {

pkg/addon/utils.go

@@ -21,6 +21,7 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"reflect"
 	"strings"
 
 	"github.com/pkg/errors"
@@ -503,6 +504,19 @@ func checkBondComponentExist(u unstructured.Unstructured, app v1beta1.Applicatio
 	return false
 }
 
+func validateAddonPackage(addonPkg *InstallPackage) error {
+	if reflect.DeepEqual(addonPkg.Meta, Meta{}) {
+		return fmt.Errorf("the addon package doesn't have `metadata.yaml`")
+	}
+	if addonPkg.Name == "" {
+		return fmt.Errorf("`matadata.yaml` must define the name of addon")
+	}
+	if addonPkg.Version == "" {
+		return fmt.Errorf("`matadata.yaml` must define the version of addon")
+	}
+	return nil
+}
+
 // FilterDependencyRegistries will return all registries besides the target registry itself
 func FilterDependencyRegistries(i int, rs []Registry) []Registry {
 	if i >= len(rs) {

pkg/addon/utils_test.go

@@ -17,9 +17,11 @@ limitations under the License.
 package addon
 
 import (
+	"fmt"
 	"net/http/httptest"
 	"os"
 	"path/filepath"
+	"reflect"
 	"strings"
 	"testing"
 
@@ -402,6 +404,30 @@ func TestFilterDependencyRegistries(t *testing.T) {
 	}
 }
 
+func TestCheckAddonPackageValid(t *testing.T) {
+	testCases := []struct {
+		testCase Meta
+		err      error
+	}{{
+		testCase: Meta{},
+		err:      fmt.Errorf("the addon package doesn't have `metadata.yaml`"),
+	}, {
+		testCase: Meta{Version: "v1.4.0"},
+		err:      fmt.Errorf("`matadata.yaml` must define the name of addon"),
+	}, {
+		testCase: Meta{Name: "test-addon"},
+		err:      fmt.Errorf("`matadata.yaml` must define the version of addon"),
+	}, {
+		testCase: Meta{Name: "test-addon", Version: "1.4.5"},
+		err:      nil,
+	},
+	}
+	for _, testCase := range testCases {
+		err := validateAddonPackage(&InstallPackage{Meta: testCase.testCase})
+		assert.Equal(t, reflect.DeepEqual(err, testCase.err), true)
+	}
+}
+
 const (
 	compDefYaml = `
 apiVersion: core.oam.dev/v1beta1

references/cli/addon.go

@@ -185,7 +185,7 @@ func NewAddonEnableCommand(c common.Args, ioStream cmdutil.IOStreams) *cobra.Com
 				if !file.IsDir() {
 					return fmt.Errorf("%s is not addon dir", addonOrDir)
 				}
-				ioStream.Infof("enable addon by local dir: %s \n", addonOrDir)
+				ioStream.Infof(color.New(color.FgYellow).Sprintf("enabling addon by local dir: %s \n", addonOrDir))
 				// args[0] is a local path install with local dir, use base dir name as addonName
 				abs, err := filepath.Abs(addonOrDir)
 				if err != nil {
@@ -314,7 +314,7 @@ non-empty new arg
 				if !file.IsDir() {
 					return fmt.Errorf("%s is not addon dir", addonOrDir)
 				}
-				ioStream.Infof("enable addon by local dir: %s \n", addonOrDir)
+				ioStream.Infof(color.New(color.FgYellow).Sprintf("enabling addon by local dir: %s \n", addonOrDir))
 				// args[0] is a local path install with local dir
 				abs, err := filepath.Abs(addonOrDir)
 				if err != nil {