Merge pull request #1382 from devstream-io/refactor-struct-error-msg

refactor: error message and add app validate

Author: aFlyBird0

internal/pkg/configmanager/app.go

@@ -5,6 +5,7 @@ import (
 
 	"github.com/devstream-io/devstream/pkg/util/log"
 	"github.com/devstream-io/devstream/pkg/util/scm/git"
+	"github.com/devstream-io/devstream/pkg/util/validator"
 )
 
 const (
@@ -17,28 +18,32 @@ type repoTemplate struct {
 }
 
 type app struct {
-	Name         string        `yaml:"name" mapstructure:"name"`
-	Spec         *appSpec      `yaml:"spec" mapstructure:"spec"`
-	Repo         *git.RepoInfo `yaml:"repo" mapstructure:"repo"`
+	Name         string        `yaml:"name" mapstructure:"name" validate:"required"`
+	Spec         *appSpec      `yaml:"spec" mapstructure:"spec" validate:"required"`
+	Repo         *git.RepoInfo `yaml:"repo" mapstructure:"repo" validate:"required"`
 	RepoTemplate *repoTemplate `yaml:"repoTemplate" mapstructure:"repoTemplate"`
 	CIRawConfigs []pipelineRaw `yaml:"ci" mapstructure:"ci"`
 	CDRawConfigs []pipelineRaw `yaml:"cd" mapstructure:"cd"`
 }
 
 func (a *app) getTools(vars map[string]any, templateMap map[string]string) (Tools, error) {
-	// 1. set app default field repoInfo and repoTemplateInfo
+	// 1. check app config data is valid
+	if err := validator.CheckStructError(a).Combine(); err != nil {
+		return nil, err
+	}
+	// 2. set app default field repoInfo and repoTemplateInfo
 	if err := a.setDefault(); err != nil {
 		return nil, err
 	}
 
-	// 2. get ci/cd pipelineTemplates
+	// 3. get ci/cd pipelineTemplates
 	appVars := a.Spec.merge(vars)
 	tools, err := a.generateCICDTools(templateMap, appVars)
 	if err != nil {
 		return nil, fmt.Errorf("app[%s] get pipeline tools failed: %w", a.Name, err)
 	}
 
-	// 3. generate app repo and template repo from scmInfo
+	// 4. generate app repo and template repo from scmInfo
 	repoScaffoldingTool := a.generateRepoTemplateTool()
 	if repoScaffoldingTool != nil {
 		tools = append(tools, repoScaffoldingTool)
@@ -58,6 +63,9 @@ func (a *app) generateCICDTools(templateMap map[string]string, appVars map[strin
 		if err != nil {
 			return nil, err
 		}
+		if err := validator.CheckStructError(t).Combine(); err != nil {
+			return nil, err
+		}
 		t.updatePipelineVars(pipelineGlobalVars)
 		pipelineTool, err := t.generatePipelineTool(pipelineGlobalVars)
 		if err != nil {
@@ -90,9 +98,6 @@ func (a *app) generateRepoTemplateTool() *Tool {
 
 // setDefault will set repoName to appName if repo.name field is empty
 func (a *app) setDefault() error {
-	if a.Repo == nil {
-		return fmt.Errorf("configmanager[app] is invalid, repo field must be configured")
-	}
 	if a.Repo.Repo == "" {
 		a.Repo.Repo = a.Name
 	}

internal/pkg/configmanager/app_test.go

@@ -27,7 +27,7 @@ var _ = Describe("app struct", func() {
 			It("should return error", func() {
 				_, err := a.getTools(vars, templateMap)
 				Expect(err).Should(HaveOccurred())
-				Expect(err.Error()).Should(ContainSubstring("configmanager[app] is invalid, repo field must be configured"))
+				Expect(err.Error()).Should(ContainSubstring("field app.repo is required"))
 			})
 		})
 		When("ci/cd template is not valid", func() {
@@ -47,7 +47,8 @@ var _ = Describe("app struct", func() {
 			It("should return error", func() {
 				_, err := a.getTools(vars, templateMap)
 				Expect(err).Should(HaveOccurred())
-				Expect(err.Error()).Should(ContainSubstring("not found in pipelineTemplates"))
+				Expect(err.Error()).Should(ContainSubstring("field app.name is required"))
+				Expect(err.Error()).Should(ContainSubstring("field app.spec is required"))
 			})
 		})
 		When("app repo template is empty", func() {
@@ -57,6 +58,7 @@ var _ = Describe("app struct", func() {
 					Repo: &git.RepoInfo{
 						CloneURL: "http://test.com/test/test_app",
 					},
+					Spec: &appSpec{Language: "go", FrameWork: "gin"},
 				}
 			})
 			It("should return empty tools", func() {
@@ -89,7 +91,7 @@ name: not_valid,
 type: not_valid`}
 				_, err := a.generateCICDTools(templateMap, vars)
 				Expect(err).Should(HaveOccurred())
-				Expect(err.Error()).Should(ContainSubstring("pipeline type [not_valid] not supported for now"))
+				Expect(err.Error()).Should(ContainSubstring("field pipelineTemplate.type must be one of"))
 			})
 		})
 	})

internal/pkg/configmanager/appspec.go

@@ -10,8 +10,8 @@ import (
 // appSpec is app special options
 type appSpec struct {
 	// language config
-	Language  string `yaml:"language" mapstructure:"language"`
-	FrameWork string `yaml:"framework" mapstructure:"framework"`
+	Language  string `yaml:"language" mapstructure:"language" validate:"required"`
+	FrameWork string `yaml:"framework" mapstructure:"framework" validate:"required"`
 }
 
 // merge will merge vars and appSpec

internal/pkg/configmanager/configmanager_test.go

@@ -45,7 +45,7 @@ apps:
     url: github.com/devstream-io/dtm-repo-scaffolding-golang-gin # optional，if url is specified，we can infer scm/owner/org/name from url
   ci:
   - type: template
-    templateName: ci-pipeline-for-gh-actions
+    templateName: ci-pipeline-for-github-actions
     options: # overwrite options in pipelineTemplates
       docker:
         registry:
@@ -76,7 +76,7 @@ tools:
     foo2: [[ foo2 ]]
 
 pipelineTemplates:
-- name: ci-pipeline-for-gh-actions
+- name: ci-pipeline-for-github-actions
   type: github-actions # corresponding to a plugin
   options:
     branch: main # optional, default is main

internal/pkg/configmanager/pipelinetemplate.go

@@ -13,15 +13,15 @@ import (
 type (
 	// pipelineRaw is the raw format of app.ci/app.cd config
 	pipelineRaw struct {
-		Type         string     `yaml:"type" mapstructure:"type"`
-		TemplateName string     `yaml:"templateName" mapstructure:"templateName"`
+		Type         string     `yaml:"type" mapstructure:"type" validate:"required,oneof=template jenkins-pipeline github-actions gitlab-ci argocdapp"`
+		TemplateName string     `yaml:"templateName" mapstructure:"templateName" validate:"required"`
 		Options      RawOptions `yaml:"options" mapstructure:"options"`
 		Vars         RawOptions `yaml:"vars" mapstructure:"vars"`
 	}
 	// pipelineTemplate is valid pipeline format
 	pipelineTemplate struct {
-		Name    string     `yaml:"name"`
-		Type    string     `yaml:"type"`
+		Name    string     `yaml:"name" validate:"required"`
+		Type    string     `yaml:"type" validate:"required,oneof=jenkins-pipeline github-actions gitlab-ci argocdapp"`
 		Options RawOptions `yaml:"options"`
 	}
 	// pipelineGlobalOption is used to pass variable between ci/cd pipeline

internal/pkg/configmanager/tool.go

@@ -5,7 +5,6 @@ import (
 	"runtime"
 	"strings"
 
-	"go.uber.org/multierr"
 	"gopkg.in/yaml.v3"
 
 	"github.com/devstream-io/devstream/internal/pkg/version"
@@ -56,22 +55,13 @@ func (t *Tool) String() string {
 type Tools []*Tool
 
 func (tools Tools) validateAll() error {
-	var errs []error
-	errs = append(errs, tools.validate()...)
-	errs = append(errs, tools.validateDependsOnConfig()...)
-	return multierr.Combine(errs...)
-}
-
-func (tools Tools) validate() (errs []error) {
+	var errs validator.StructFieldErrors
 	for _, tool := range tools {
-		errs = append(errs, tool.validate()...)
+		errs = append(errs, validator.CheckStructError(tool)...)
 	}
+	errs = append(errs, tools.validateDependsOnConfig()...)
 	errs = append(errs, tools.duplicatedCheck()...)
-	return
-}
-
-func (t *Tool) validate() []error {
-	return validator.Struct(t)
+	return errs.Combine()
 }
 
 func (t *Tool) DeepCopy() *Tool {

internal/pkg/configmanager/tool_test.go

@@ -67,20 +67,20 @@ var _ = Describe("validateDependsOnConfig", func() {
 	})
 })
 
-var _ = Describe("Tool Validation", func() {
+var _ = Describe("Tool Validation all", func() {
 	It("should return empty error array if tools all valid", func() {
 		tools := Tools{
 			{Name: "test_tool", InstanceID: "0", DependsOn: []string{}},
 		}
-		errors := tools.validate()
-		Expect(errors).Should(BeEmpty())
+		err := tools.validateAll()
+		Expect(err).ShouldNot(HaveOccurred())
 	})
 	It("should return error if tool not valid", func() {
 		tools := Tools{
 			{Name: "", InstanceID: "", DependsOn: []string{}},
 		}
-		errors := tools.validate()
-		Expect(errors).ShouldNot(BeEmpty())
+		err := tools.validateAll()
+		Expect(err).Should(HaveOccurred())
 	})
 
 })

internal/pkg/develop/plugin/template/validate.go

@@ -18,12 +18,8 @@ func validate(options configmanager.RawOptions) (configmanager.RawOptions, error
 	if err != nil {
 		return nil, err
 	}
-	errs := validator.Struct(opts)
-	if len(errs) > 0 {
-		for _, e := range errs {
-			log.Errorf("Options error: %s.", e)
-		}
-		return nil, fmt.Errorf("opts are illegal")
+	if errs := validator.CheckStructError(opts).Combine(); len(errs) != 0 {
+		return nil, errs.Combine()
 	}
 	return options, nil
 }

internal/pkg/plugin/argocdapp/validate.go

@@ -15,7 +15,7 @@ func validate(options configmanager.RawOptions) (configmanager.RawOptions, error
 	if err != nil {
 		return nil, err
 	}
-	if err = validator.StructAllError(opts); err != nil {
+	if err := validator.CheckStructError(opts).Combine(); err != nil {
 		return nil, err
 	}
 	return options, nil

internal/pkg/plugin/devlakeconfig/validate.go

@@ -1,10 +1,7 @@
 package devlakeconfig
 
 import (
-	"fmt"
-
 	"github.com/devstream-io/devstream/internal/pkg/configmanager"
-	"github.com/devstream-io/devstream/pkg/util/log"
 	"github.com/devstream-io/devstream/pkg/util/validator"
 )
 
@@ -14,12 +11,8 @@ func validate(options configmanager.RawOptions) (configmanager.RawOptions, error
 	if err != nil {
 		return nil, err
 	}
-	errs := validator.Struct(opts)
-	if len(errs) > 0 {
-		for _, e := range errs {
-			log.Errorf("Options error: %s.", e)
-		}
-		return nil, fmt.Errorf("opts are illegal")
+	if err := validator.CheckStructError(opts).Combine(); err != nil {
+		return nil, err
 	}
 	return options, nil
 }