Fix: lint apiserver code, fix panic (#2755)

* lint code

* fix error judge
try

* fix multicluster enable panic

* add err log

* fix can not get parameter

* debug

* try ci

* debug

* debug

* debug

* debugo

Co-authored-by: barnettZQG <576501057@qq.com>

pkg/addon/addon.go

@@ -53,6 +53,7 @@ const (
 	DefinitionsDirName string = "definitions"
 )
 
+// ListOptions contains flags mark what files should be read in an addon directory
 type ListOptions struct {
 	GetDetail     bool
 	GetDefinition bool
@@ -62,17 +63,22 @@ type ListOptions struct {
 }
 
 var (
-	ListLevelOptions   = ListOptions{}
-	GetLevelOptions    = ListOptions{GetDetail: true, GetDefinition: true, GetParameter: true}
+	// GetLevelOptions used when get or list addons
+	GetLevelOptions = ListOptions{GetDetail: true, GetDefinition: true, GetParameter: true}
+
+	// EnableLevelOptions used when enable addon
 	EnableLevelOptions = ListOptions{GetDetail: true, GetDefinition: true, GetResource: true, GetTemplate: true, GetParameter: true}
 )
 
-type AddonErr error
+// aError is internal error type of addon
+type aError error
 
 var (
-	AddonNotExist AddonErr = errors.New("addon not exist")
+	// ErrNotExist means addon not exists
+	ErrNotExist aError = errors.New("addon not exist")
 )
 
+// gitHelper helps get addon's file by git
 type gitHelper struct {
 	Client *github.Client
 	Meta   *utils.Content
@@ -85,14 +91,16 @@ type GitAddonSource struct {
 	Token string `json:"token,omitempty"`
 }
 
-type AddonReader struct {
+// asyncReader helps async read files of addon
+type asyncReader struct {
 	addon   *types.Addon
 	h       *gitHelper
 	item    *github.RepositoryContent
 	errChan chan error
 }
 
-func (r *AddonReader) SetReadContent(content *github.RepositoryContent) {
+// SetReadContent set which file to read
+func (r *asyncReader) SetReadContent(content *github.RepositoryContent) {
 	r.item = content
 }
 
@@ -159,8 +167,11 @@ func getSingleAddonFromGit(baseURL, dir, addonName, token string, opt ListOption
 		return nil, err
 	}
 	_, items, err := gith.readRepo(gith.Meta.Path)
+	if err != nil {
+		return nil, err
+	}
 
-	reader := AddonReader{
+	reader := asyncReader{
 		addon:   &types.Addon{},
 		h:       gith,
 		errChan: make(chan error, 1),
@@ -186,7 +197,7 @@ func getSingleAddonFromGit(baseURL, dir, addonName, token string, opt ListOption
 			wg.Add(1)
 			go readDefinitions(&wg, reader)
 		case ResourcesDirName:
-			if !opt.GetResource {
+			if !opt.GetResource && !opt.GetParameter {
 				break
 			}
 			reader.SetReadContent(item)
@@ -213,7 +224,7 @@ func getSingleAddonFromGit(baseURL, dir, addonName, token string, opt ListOption
 
 }
 
-func readTemplate(wg *sync.WaitGroup, reader AddonReader) {
+func readTemplate(wg *sync.WaitGroup, reader asyncReader) {
 	defer wg.Done()
 	content, _, err := reader.h.readRepo(*reader.item.Path)
 	if err != nil {
@@ -234,7 +245,7 @@ func readTemplate(wg *sync.WaitGroup, reader AddonReader) {
 	}
 }
 
-func readResources(wg *sync.WaitGroup, reader AddonReader) {
+func readResources(wg *sync.WaitGroup, reader asyncReader) {
 	defer wg.Done()
 	dirPath := strings.Split(reader.item.GetPath(), "/")
 	dirPath, err := cutPathUntil(dirPath, ResourcesDirName)
@@ -263,7 +274,7 @@ func readResources(wg *sync.WaitGroup, reader AddonReader) {
 }
 
 // readResFile read single resource file
-func readResFile(wg *sync.WaitGroup, reader AddonReader, dirPath []string) {
+func readResFile(wg *sync.WaitGroup, reader asyncReader, dirPath []string) {
 	defer wg.Done()
 	content, _, err := reader.h.readRepo(*reader.item.Path)
 	if err != nil {
@@ -288,7 +299,7 @@ func readResFile(wg *sync.WaitGroup, reader AddonReader, dirPath []string) {
 	}
 }
 
-func readDefinitions(wg *sync.WaitGroup, reader AddonReader) {
+func readDefinitions(wg *sync.WaitGroup, reader asyncReader) {
 	defer wg.Done()
 	dirPath := strings.Split(reader.item.GetPath(), "/")
 	dirPath, err := cutPathUntil(dirPath, DefinitionsDirName)
@@ -316,7 +327,7 @@ func readDefinitions(wg *sync.WaitGroup, reader AddonReader) {
 }
 
 // readDefFile read single definition file
-func readDefFile(wg *sync.WaitGroup, reader AddonReader, dirPath []string) {
+func readDefFile(wg *sync.WaitGroup, reader asyncReader, dirPath []string) {
 	defer wg.Done()
 	content, _, err := reader.h.readRepo(*reader.item.Path)
 	if err != nil {
@@ -331,7 +342,7 @@ func readDefFile(wg *sync.WaitGroup, reader AddonReader, dirPath []string) {
 	reader.addon.Definitions = append(reader.addon.Definitions, types.AddonElementFile{Data: b, Name: reader.item.GetName(), Path: dirPath})
 }
 
-func readMetadata(wg *sync.WaitGroup, reader AddonReader) {
+func readMetadata(wg *sync.WaitGroup, reader asyncReader) {
 	defer wg.Done()
 	content, _, err := reader.h.readRepo(*reader.item.Path)
 	if err != nil {
@@ -350,7 +361,7 @@ func readMetadata(wg *sync.WaitGroup, reader AddonReader) {
 	}
 }
 
-func readReadme(wg *sync.WaitGroup, reader AddonReader) {
+func readReadme(wg *sync.WaitGroup, reader asyncReader) {
 	defer wg.Done()
 	content, _, err := reader.h.readRepo(*reader.item.Path)
 	if err != nil {
@@ -358,6 +369,10 @@ func readReadme(wg *sync.WaitGroup, reader AddonReader) {
 		return
 	}
 	reader.addon.Detail, err = content.GetContent()
+	if err != nil {
+		reader.errChan <- err
+		return
+	}
 }
 
 func createGitHelper(baseURL, dir, token string) (*gitHelper, error) {
@@ -476,6 +491,9 @@ func RenderApplication(addon *types.Addon, args map[string]string) (*v1beta1.App
 			}
 			defObjs = append(defObjs, obj)
 		}
+		if app.Spec.Workflow == nil {
+			app.Spec.Workflow = &v1beta1.Workflow{Steps: make([]v1beta1.WorkflowStep, 0)}
+		}
 		app.Spec.Workflow.Steps = append(app.Spec.Workflow.Steps,
 			v1beta1.WorkflowStep{
 				Name: "deploy-all",

pkg/addon/error.go

@@ -20,8 +20,8 @@ var (
 
 // WrapErrRateLimit return ErrRateLimit if is the situation, or return error directly
 func WrapErrRateLimit(err error) error {
-	var rateLimit *github.RateLimitError
-	if errors.As(err, &rateLimit) {
+	errRate := &github.RateLimitError{}
+	if errors.As(err, &errRate) {
 		return ErrRateLimit
 	}
 	return err

pkg/apiserver/rest/usecase/addon.go

@@ -51,12 +51,12 @@ func AddonImpl2AddonRes(impl *types.Addon) (*apis.DetailAddonResponse, error) {
 		dec := k8syaml.NewDecodingSerializer(unstructured.UnstructuredJSONScheme)
 		_, _, err := dec.Decode([]byte(def.Data), nil, obj)
 		if err != nil {
-			return nil, errors.New(fmt.Sprintf("convert %s file content to definition fail", def.Name))
+			return nil, fmt.Errorf("convert %s file content to definition fail", def.Name)
 		}
 		defs = append(defs, &apis.AddonDefinition{
-			obj.GetName(),
-			obj.GetKind(),
-			obj.GetAnnotations()["definition.oam.dev/description"],
+			Name:        obj.GetName(),
+			DefType:     obj.GetKind(),
+			Description: obj.GetAnnotations()["definition.oam.dev/description"],
 		})
 	}
 	return &apis.DetailAddonResponse{
@@ -104,23 +104,21 @@ func (u *addonUsecaseImpl) GetAddon(ctx context.Context, name string, registry s
 			if addon, exist = u.tryGetAddonFromCache(r.Name, name); !exist {
 				addon, err = pkgaddon.GetAddon(name, r.Git, pkgaddon.GetLevelOptions)
 			}
-			if err != nil && !errors.Is(err, pkgaddon.AddonNotExist) {
+			if err != nil && !errors.Is(err, pkgaddon.ErrNotExist) {
 				return nil, err
 			}
 			if addon != nil {
 				break
 			}
 		}
-	} else {
-		if addon, exist = u.tryGetAddonFromCache(registry, name); !exist {
-			addonRegistry, err := u.GetAddonRegistry(ctx, registry)
-			if err != nil {
-				return nil, err
-			}
-			addon, err = pkgaddon.GetAddon(name, addonRegistry.Git, pkgaddon.GetLevelOptions)
-			if err != nil && !errors.Is(err, pkgaddon.AddonNotExist) {
-				return nil, err
-			}
+	} else if addon, exist = u.tryGetAddonFromCache(registry, name); !exist {
+		addonRegistry, err := u.GetAddonRegistry(ctx, registry)
+		if err != nil {
+			return nil, err
+		}
+		addon, err = pkgaddon.GetAddon(name, addonRegistry.Git, pkgaddon.GetLevelOptions)
+		if err != nil && !errors.Is(err, pkgaddon.ErrNotExist) {
+			return nil, err
 		}
 	}
 
@@ -194,7 +192,7 @@ func (u *addonUsecaseImpl) ListAddons(ctx context.Context, registry, query strin
 		} else {
 			listAddons, err = pkgaddon.ListAddons(r.Git, pkgaddon.GetLevelOptions)
 			if err != nil {
-				log.Logger.Errorf("fail to get addons from registry %s", r.Name)
+				log.Logger.Errorf("fail to get addons from registry %s, %v", r.Name, err)
 				continue
 			}
 			// if list addons, details will be retrieved later
@@ -332,7 +330,7 @@ func (u *addonUsecaseImpl) EnableAddon(ctx context.Context, name string, args ap
 		if addon, exist = u.tryGetAddonFromCache(r.Name, name); !exist {
 			addon, err = pkgaddon.GetAddon(name, r.Git, pkgaddon.EnableLevelOptions)
 		}
-		if err != nil && !errors.Is(err, pkgaddon.AddonNotExist) {
+		if err != nil && !errors.Is(err, pkgaddon.ErrNotExist) {
 			return bcode.WrapGithubRateLimitErr(err)
 		}
 		if addon == nil {

test/e2e-apiserver-test/addon_test.go

@@ -1,23 +1,22 @@
-package e2e_apiserver
+package e2e_apiserver_test
 
 import (
 	"bytes"
 	"context"
 	"encoding/json"
+	"fmt"
 	"net/http"
 	"os"
 	"time"
 
-	. "github.com/onsi/ginkgo"
-	. "github.com/onsi/gomega"
-	v1 "k8s.io/api/core/v1"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/util/wait"
+	"github.com/pkg/errors"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 
+	"github.com/oam-dev/kubevela/apis/core.oam.dev/v1beta1"
 	"github.com/oam-dev/kubevela/pkg/addon"
 	apis "github.com/oam-dev/kubevela/pkg/apiserver/rest/apis/v1"
-	"github.com/oam-dev/kubevela/pkg/oam/util"
-	"github.com/oam-dev/kubevela/pkg/utils/common"
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
 )
 
 const baseURL = "http://127.0.0.1:8000"
@@ -52,8 +51,8 @@ var _ = Describe("Test addon rest api", func() {
 		By("add registry")
 		createRes := post("/api/v1/addon_registries", createReq)
 		Expect(createRes).ShouldNot(BeNil())
-		Expect(createRes.StatusCode).Should(Equal(200))
 		Expect(createRes.Body).ShouldNot(BeNil())
+		Expect(createRes.StatusCode).Should(Equal(200))
 
 		defer createRes.Body.Close()
 
@@ -77,17 +76,6 @@ var _ = Describe("Test addon rest api", func() {
 	})
 
 	It("should enable and disable an addon", func() {
-		// todo(qiaozp) we should remove this namespace creation. This should be solved with a application template.
-		ns := v1.Namespace{
-			ObjectMeta: metav1.ObjectMeta{
-				Name: "flux-system",
-			},
-		}
-		args := common.Args{}
-		k8sClient, err := args.GetClient()
-		Expect(err).Should(BeNil())
-		Expect(k8sClient.Create(context.Background(), &ns)).Should(SatisfyAny(BeNil(), &util.AlreadyExistMatcher{}))
-
 		defer GinkgoRecover()
 		req := apis.EnableAddonRequest{
 			Args: map[string]string{
@@ -103,25 +91,30 @@ var _ = Describe("Test addon rest api", func() {
 		defer res.Body.Close()
 
 		var statusRes apis.AddonStatusResponse
-		err = json.NewDecoder(res.Body).Decode(&statusRes)
+		err := json.NewDecoder(res.Body).Decode(&statusRes)
 
 		Expect(err).Should(BeNil())
 		Expect(statusRes.Phase).Should(Equal(apis.AddonPhaseEnabling))
 
 		// Wait for addon enabled
 
-		period := 20 * time.Second
-		timeout := 5 * time.Minute
-		err = wait.PollImmediate(period, timeout, func() (done bool, err error) {
+		period := 10 * time.Second
+		timeout := 2 * time.Minute
+		Eventually(func() error {
 			res = get("/api/v1/addons/" + testAddon + "/status")
 			err = json.NewDecoder(res.Body).Decode(&statusRes)
 			Expect(err).Should(BeNil())
 			if statusRes.Phase == apis.AddonPhaseEnabled {
-				return true, nil
+				return nil
 			}
-			return false, nil
-		})
-		Expect(err).Should(BeNil())
+			var app v1beta1.Application
+			err = k8sClient.Get(context.Background(), client.ObjectKey{Name: "addon-example", Namespace: "vela-system"}, &app)
+			Expect(err).Should(BeNil())
+			data, err := json.Marshal(app)
+			Expect(err).Should(BeNil())
+			fmt.Println(data)
+			return errors.New("not ready")
+		}, timeout, period).Should(BeNil())
 
 		res = post("/api/v1/addons/"+testAddon+"/disable", req)
 		Expect(res).ShouldNot(BeNil())