From 909c311754e668bd82c95939b077d68a0aa26838 Mon Sep 17 00:00:00 2001 From: Meng JiaFeng Date: Mon, 26 Dec 2022 13:54:43 +0800 Subject: [PATCH] refactor: error message and add app validate Signed-off-by: Meng JiaFeng --- internal/pkg/configmanager/app.go | 23 ++- internal/pkg/configmanager/app_test.go | 8 +- internal/pkg/configmanager/appspec.go | 4 +- .../pkg/configmanager/configmanager_test.go | 4 +- .../pkg/configmanager/pipelinetemplate.go | 8 +- internal/pkg/configmanager/tool.go | 18 +- internal/pkg/configmanager/tool_test.go | 10 +- .../pkg/develop/plugin/template/validate.go | 8 +- internal/pkg/plugin/argocdapp/validate.go | 2 +- internal/pkg/plugin/devlakeconfig/validate.go | 11 +- .../pkg/plugin/gitlabcedocker/validate.go | 12 +- .../pkg/plugin/helminstaller/helminstaller.go | 3 +- internal/pkg/plugin/helminstaller/validate.go | 10 +- .../plugin/installer/ci/cifile/validate.go | 3 +- internal/pkg/plugin/installer/ci/validate.go | 2 +- .../pkg/plugin/installer/docker/option.go | 8 +- .../pkg/plugin/installer/goclient/validate.go | 11 +- .../pkg/plugin/installer/helm/validate.go | 18 +- .../installer/reposcaffolding/validate.go | 2 +- .../pkg/plugin/jenkinspipeline/jenkins.go | 2 +- .../pkg/plugin/jenkinspipeline/validate.go | 2 +- internal/pkg/plugin/jiragithub/create.go | 9 +- internal/pkg/plugin/jiragithub/delete.go | 11 +- internal/pkg/plugin/jiragithub/read.go | 11 +- internal/pkg/plugin/jiragithub/update.go | 11 +- internal/pkg/plugin/jiragithub/validate.go | 10 -- internal/pkg/plugin/trello/validate.go | 2 +- .../pkg/plugin/trellogithub/trellogithub.go | 8 +- internal/pkg/plugin/trellogithub/validate.go | 9 - pkg/util/helm/validation.go | 22 --- pkg/util/helm/validation_test.go | 35 ---- pkg/util/template/processor_test.go | 2 - pkg/util/validator/validator.go | 59 +++++-- pkg/util/validator/validator_suite_test.go | 13 ++ pkg/util/validator/validator_test.go | 167 +++++++++--------- 35 files changed, 238 insertions(+), 300 deletions(-) delete mode 100644 internal/pkg/plugin/jiragithub/validate.go delete mode 100644 internal/pkg/plugin/trellogithub/validate.go delete mode 100644 pkg/util/helm/validation.go delete mode 100644 pkg/util/helm/validation_test.go create mode 100644 pkg/util/validator/validator_suite_test.go diff --git a/internal/pkg/configmanager/app.go b/internal/pkg/configmanager/app.go index bef66875f..28f6e3897 100644 --- a/internal/pkg/configmanager/app.go +++ b/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 } diff --git a/internal/pkg/configmanager/app_test.go b/internal/pkg/configmanager/app_test.go index 2ccf7189f..5ed6f16b1 100644 --- a/internal/pkg/configmanager/app_test.go +++ b/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")) }) }) }) diff --git a/internal/pkg/configmanager/appspec.go b/internal/pkg/configmanager/appspec.go index da678b41d..8873eba00 100644 --- a/internal/pkg/configmanager/appspec.go +++ b/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 diff --git a/internal/pkg/configmanager/configmanager_test.go b/internal/pkg/configmanager/configmanager_test.go index 00b14132e..563cec8ce 100644 --- a/internal/pkg/configmanager/configmanager_test.go +++ b/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 diff --git a/internal/pkg/configmanager/pipelinetemplate.go b/internal/pkg/configmanager/pipelinetemplate.go index dab9dcea8..0acaf7813 100644 --- a/internal/pkg/configmanager/pipelinetemplate.go +++ b/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 diff --git a/internal/pkg/configmanager/tool.go b/internal/pkg/configmanager/tool.go index 09c2ca4f4..af136ef47 100644 --- a/internal/pkg/configmanager/tool.go +++ b/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 { diff --git a/internal/pkg/configmanager/tool_test.go b/internal/pkg/configmanager/tool_test.go index c6a2232ba..ecf083f8a 100644 --- a/internal/pkg/configmanager/tool_test.go +++ b/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()) }) }) diff --git a/internal/pkg/develop/plugin/template/validate.go b/internal/pkg/develop/plugin/template/validate.go index 94218da45..db17361b9 100644 --- a/internal/pkg/develop/plugin/template/validate.go +++ b/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 } diff --git a/internal/pkg/plugin/argocdapp/validate.go b/internal/pkg/plugin/argocdapp/validate.go index 468f919c4..1d5cdf888 100644 --- a/internal/pkg/plugin/argocdapp/validate.go +++ b/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 diff --git a/internal/pkg/plugin/devlakeconfig/validate.go b/internal/pkg/plugin/devlakeconfig/validate.go index 18c43b6ca..57d8d5ff9 100644 --- a/internal/pkg/plugin/devlakeconfig/validate.go +++ b/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 } diff --git a/internal/pkg/plugin/gitlabcedocker/validate.go b/internal/pkg/plugin/gitlabcedocker/validate.go index 7695d613e..5f7641455 100644 --- a/internal/pkg/plugin/gitlabcedocker/validate.go +++ b/internal/pkg/plugin/gitlabcedocker/validate.go @@ -9,7 +9,6 @@ import ( "github.com/mitchellh/mapstructure" - "github.com/devstream-io/devstream/pkg/util/log" "github.com/devstream-io/devstream/pkg/util/validator" ) @@ -22,16 +21,13 @@ func validateAndDefault(options configmanager.RawOptions) (*Options, error) { opts.Defaults() // validate - errs := validator.Struct(opts) + errs := validator.CheckStructError(opts) // volume directory must be absolute path if !filepath.IsAbs(opts.GitLabHome) { - errs = append(errs, fmt.Errorf("GitLabHome must be an absolute path")) + errs = append(errs, fmt.Errorf("field gitLabHome must be an absolute path")) } - if len(errs) > 0 { - for _, e := range errs { - log.Errorf("Options error: %s.", e) - } - return nil, fmt.Errorf("opts are illegal") + if len(errs) != 0 { + return nil, errs.Combine() } if err := os.MkdirAll(opts.GitLabHome, 0755); err != nil { diff --git a/internal/pkg/plugin/helminstaller/helminstaller.go b/internal/pkg/plugin/helminstaller/helminstaller.go index 0e32b598b..958223f41 100644 --- a/internal/pkg/plugin/helminstaller/helminstaller.go +++ b/internal/pkg/plugin/helminstaller/helminstaller.go @@ -9,6 +9,7 @@ import ( "github.com/devstream-io/devstream/internal/pkg/plugin/installer" "github.com/devstream-io/devstream/internal/pkg/plugin/installer/helm" "github.com/devstream-io/devstream/pkg/util/log" + "github.com/devstream-io/devstream/pkg/util/mapz" "github.com/devstream-io/devstream/pkg/util/types" ) @@ -30,7 +31,7 @@ func renderDefaultConfig(options configmanager.RawOptions) (configmanager.RawOpt helmOptions.FillDefaultValue(defaultHelmOptions) log.Debugf("Options with default config filled: %v.", helmOptions) - return types.EncodeStruct(helmOptions) + return mapz.DecodeStructToMap(helmOptions) } // renderValuesYaml renders options.valuesYaml to options.chart.valuesYaml; diff --git a/internal/pkg/plugin/helminstaller/validate.go b/internal/pkg/plugin/helminstaller/validate.go index 24d22035b..c9430cfff 100644 --- a/internal/pkg/plugin/helminstaller/validate.go +++ b/internal/pkg/plugin/helminstaller/validate.go @@ -3,9 +3,17 @@ package helminstaller import ( "github.com/devstream-io/devstream/internal/pkg/configmanager" "github.com/devstream-io/devstream/internal/pkg/plugin/installer/helm" + "github.com/devstream-io/devstream/pkg/util/validator" ) // validate validates the options provided by the core. func validate(options configmanager.RawOptions) (configmanager.RawOptions, error) { - return helm.Validate(options) + helmOptions, err := helm.NewOptions(options) + if err != nil { + return nil, err + } + if err := validator.CheckStructError(helmOptions).Combine(); err != nil { + return nil, err + } + return options, nil } diff --git a/internal/pkg/plugin/installer/ci/cifile/validate.go b/internal/pkg/plugin/installer/ci/cifile/validate.go index 5c85d595c..2abcff1f5 100644 --- a/internal/pkg/plugin/installer/ci/cifile/validate.go +++ b/internal/pkg/plugin/installer/ci/cifile/validate.go @@ -13,8 +13,7 @@ func Validate(options configmanager.RawOptions) (configmanager.RawOptions, error if err != nil { return nil, err } - fieldErr := validator.StructAllError(opts) - if fieldErr != nil { + if fieldErr := validator.CheckStructError(opts).Combine(); fieldErr != nil { return nil, fieldErr } return options, nil diff --git a/internal/pkg/plugin/installer/ci/validate.go b/internal/pkg/plugin/installer/ci/validate.go index 56cab6916..2039284a2 100644 --- a/internal/pkg/plugin/installer/ci/validate.go +++ b/internal/pkg/plugin/installer/ci/validate.go @@ -13,7 +13,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 diff --git a/internal/pkg/plugin/installer/docker/option.go b/internal/pkg/plugin/installer/docker/option.go index 3735d5db0..b7b4db9f7 100644 --- a/internal/pkg/plugin/installer/docker/option.go +++ b/internal/pkg/plugin/installer/docker/option.go @@ -54,12 +54,8 @@ func Validate(options configmanager.RawOptions) (configmanager.RawOptions, error return nil, err } log.Debugf("Options: %v.", opts) - 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 } diff --git a/internal/pkg/plugin/installer/goclient/validate.go b/internal/pkg/plugin/installer/goclient/validate.go index 253194274..20a6b170c 100644 --- a/internal/pkg/plugin/installer/goclient/validate.go +++ b/internal/pkg/plugin/installer/goclient/validate.go @@ -1,10 +1,7 @@ package goclient 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 } diff --git a/internal/pkg/plugin/installer/helm/validate.go b/internal/pkg/plugin/installer/helm/validate.go index 491e060ea..60bfee403 100644 --- a/internal/pkg/plugin/installer/helm/validate.go +++ b/internal/pkg/plugin/installer/helm/validate.go @@ -5,9 +5,9 @@ import ( "github.com/devstream-io/devstream/internal/pkg/configmanager" "github.com/devstream-io/devstream/internal/pkg/plugin/installer" - "github.com/devstream-io/devstream/pkg/util/helm" "github.com/devstream-io/devstream/pkg/util/log" "github.com/devstream-io/devstream/pkg/util/types" + "github.com/devstream-io/devstream/pkg/util/validator" ) // Validate validates the options provided by the dtm-core. @@ -16,14 +16,16 @@ func Validate(options configmanager.RawOptions) (configmanager.RawOptions, error if err != nil { return nil, err } - errs := helm.Validate(opts.GetHelmParam()) - if len(errs) > 0 { - for _, e := range errs { - log.Errorf("Options error: %s.", e) - } - return nil, fmt.Errorf("opts are illegal") + + var structErrs = validator.CheckStructError(opts) + + if opts.Chart.ChartPath == "" && (opts.Repo.Name == "" || opts.Repo.URL == "" || opts.Chart.ChartName == "") { + log.Debugf("Repo.Name: %s, Repo.URL: %s, Chart.ChartName: %s", opts.Repo.Name, opts.Repo.URL, opts.Chart.ChartName) + err := fmt.Errorf("if chartPath == \"\", then the repo.Name & repo.URL & chart.chartName must be set") + structErrs = append(structErrs, err) } - return options, nil + + return options, structErrs.Combine() } // SetDefaultConfig will update options empty values base on import options diff --git a/internal/pkg/plugin/installer/reposcaffolding/validate.go b/internal/pkg/plugin/installer/reposcaffolding/validate.go index 17c2d6627..0a16c67e5 100644 --- a/internal/pkg/plugin/installer/reposcaffolding/validate.go +++ b/internal/pkg/plugin/installer/reposcaffolding/validate.go @@ -11,7 +11,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 diff --git a/internal/pkg/plugin/jenkinspipeline/jenkins.go b/internal/pkg/plugin/jenkinspipeline/jenkins.go index 4d0d7ce45..fa9433c85 100644 --- a/internal/pkg/plugin/jenkinspipeline/jenkins.go +++ b/internal/pkg/plugin/jenkinspipeline/jenkins.go @@ -51,7 +51,7 @@ func (j *jenkinsOption) getBasicAuth() (*jenkins.BasicAuth, error) { } // 2. if not set, get user and password from secret k8sClient, err := k8s.NewClient() - if err != nil { + if err == nil { secretAuth := getAuthFromSecret(k8sClient, j.Namespace) if secretAuth != nil && secretAuth.CheckNameMatch(j.User) { log.Debugf("jenkins get auth token from secret") diff --git a/internal/pkg/plugin/jenkinspipeline/validate.go b/internal/pkg/plugin/jenkinspipeline/validate.go index f90626348..7a419e47f 100644 --- a/internal/pkg/plugin/jenkinspipeline/validate.go +++ b/internal/pkg/plugin/jenkinspipeline/validate.go @@ -54,7 +54,7 @@ func validateJenkins(options configmanager.RawOptions) (configmanager.RawOptions return nil, err } - if err = validator.StructAllError(opts); err != nil { + if err = validator.CheckStructError(opts).Combine(); err != nil { return nil, err } diff --git a/internal/pkg/plugin/jiragithub/create.go b/internal/pkg/plugin/jiragithub/create.go index 9b78b11d3..4800072c1 100644 --- a/internal/pkg/plugin/jiragithub/create.go +++ b/internal/pkg/plugin/jiragithub/create.go @@ -7,9 +7,9 @@ import ( "github.com/devstream-io/devstream/internal/pkg/configmanager" "github.com/devstream-io/devstream/internal/pkg/statemanager" - "github.com/devstream-io/devstream/pkg/util/log" "github.com/devstream-io/devstream/pkg/util/scm/git" "github.com/devstream-io/devstream/pkg/util/scm/github" + "github.com/devstream-io/devstream/pkg/util/validator" ) // Create sets up jira-github-integ workflows. @@ -20,11 +20,8 @@ func Create(options configmanager.RawOptions) (statemanager.ResourceStatus, erro return nil, err } - if errs := validate(&opts); len(errs) != 0 { - for _, e := range errs { - log.Errorf("Options error: %s.", e) - } - return nil, fmt.Errorf("options are illegal") + if err := validator.CheckStructError(&opts).Combine(); err != nil { + return nil, err } ghOptions := &git.RepoInfo{ diff --git a/internal/pkg/plugin/jiragithub/delete.go b/internal/pkg/plugin/jiragithub/delete.go index 1cee7cffa..fd549538e 100644 --- a/internal/pkg/plugin/jiragithub/delete.go +++ b/internal/pkg/plugin/jiragithub/delete.go @@ -1,14 +1,12 @@ package jiragithub import ( - "fmt" - "github.com/mitchellh/mapstructure" "github.com/devstream-io/devstream/internal/pkg/configmanager" - "github.com/devstream-io/devstream/pkg/util/log" "github.com/devstream-io/devstream/pkg/util/scm/git" "github.com/devstream-io/devstream/pkg/util/scm/github" + "github.com/devstream-io/devstream/pkg/util/validator" ) // Delete remove jira-github-integ workflows. @@ -19,11 +17,8 @@ func Delete(options configmanager.RawOptions) (bool, error) { return false, err } - if errs := validate(&opts); len(errs) != 0 { - for _, e := range errs { - log.Errorf("Options error: %s.", e) - } - return false, fmt.Errorf("options are illegal") + if err := validator.CheckStructError(&opts).Combine(); err != nil { + return false, err } ghOptions := &git.RepoInfo{ diff --git a/internal/pkg/plugin/jiragithub/read.go b/internal/pkg/plugin/jiragithub/read.go index a5a660078..781f2d045 100644 --- a/internal/pkg/plugin/jiragithub/read.go +++ b/internal/pkg/plugin/jiragithub/read.go @@ -1,15 +1,13 @@ package jiragithub import ( - "fmt" - "github.com/mitchellh/mapstructure" "github.com/devstream-io/devstream/internal/pkg/configmanager" "github.com/devstream-io/devstream/internal/pkg/statemanager" - "github.com/devstream-io/devstream/pkg/util/log" "github.com/devstream-io/devstream/pkg/util/scm/git" "github.com/devstream-io/devstream/pkg/util/scm/github" + "github.com/devstream-io/devstream/pkg/util/validator" ) // Read get jira-github-integ workflows. @@ -20,11 +18,8 @@ func Read(options configmanager.RawOptions) (statemanager.ResourceStatus, error) return nil, err } - if errs := validate(&opts); len(errs) != 0 { - for _, e := range errs { - log.Errorf("Options error: %s.", e) - } - return nil, fmt.Errorf("options are illegal") + if err := validator.CheckStructError(&opts).Combine(); err != nil { + return nil, err } ghOptions := &git.RepoInfo{ diff --git a/internal/pkg/plugin/jiragithub/update.go b/internal/pkg/plugin/jiragithub/update.go index 425978bc1..9317f168e 100644 --- a/internal/pkg/plugin/jiragithub/update.go +++ b/internal/pkg/plugin/jiragithub/update.go @@ -1,15 +1,13 @@ package jiragithub import ( - "fmt" - "github.com/mitchellh/mapstructure" "github.com/devstream-io/devstream/internal/pkg/configmanager" "github.com/devstream-io/devstream/internal/pkg/statemanager" - "github.com/devstream-io/devstream/pkg/util/log" "github.com/devstream-io/devstream/pkg/util/scm/git" "github.com/devstream-io/devstream/pkg/util/scm/github" + "github.com/devstream-io/devstream/pkg/util/validator" ) // Update remove and set up jira-github-integ workflows. @@ -20,11 +18,8 @@ func Update(options configmanager.RawOptions) (statemanager.ResourceStatus, erro return nil, err } - if errs := validate(&opts); len(errs) != 0 { - for _, e := range errs { - log.Errorf("Options error: %s.", e) - } - return nil, fmt.Errorf("options are illegal") + if err := validator.CheckStructError(&opts).Combine(); err != nil { + return nil, err } ghOptions := &git.RepoInfo{ diff --git a/internal/pkg/plugin/jiragithub/validate.go b/internal/pkg/plugin/jiragithub/validate.go deleted file mode 100644 index a74c1fbce..000000000 --- a/internal/pkg/plugin/jiragithub/validate.go +++ /dev/null @@ -1,10 +0,0 @@ -package jiragithub - -import ( - "github.com/devstream-io/devstream/pkg/util/validator" -) - -// validate validates the options provided by the core. -func validate(opts *Options) []error { - return validator.Struct(opts) -} diff --git a/internal/pkg/plugin/trello/validate.go b/internal/pkg/plugin/trello/validate.go index 66b7d9097..c2b9f827a 100644 --- a/internal/pkg/plugin/trello/validate.go +++ b/internal/pkg/plugin/trello/validate.go @@ -21,7 +21,7 @@ func validate(rawOptions configmanager.RawOptions) (configmanager.RawOptions, er 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 rawOptions, nil diff --git a/internal/pkg/plugin/trellogithub/trellogithub.go b/internal/pkg/plugin/trellogithub/trellogithub.go index 72cf83536..58a462f27 100644 --- a/internal/pkg/plugin/trellogithub/trellogithub.go +++ b/internal/pkg/plugin/trellogithub/trellogithub.go @@ -13,6 +13,7 @@ import ( "github.com/devstream-io/devstream/pkg/util/mapz" "github.com/devstream-io/devstream/pkg/util/scm/git" "github.com/devstream-io/devstream/pkg/util/scm/github" + "github.com/devstream-io/devstream/pkg/util/validator" ) type TrelloGithub struct { @@ -37,11 +38,8 @@ func NewTrelloGithub(options configmanager.RawOptions) (*TrelloGithub, error) { return nil, err } - if errs := validate(&opts); len(errs) != 0 { - for _, e := range errs { - log.Errorf("Param error: %s.", e) - } - return nil, fmt.Errorf("params are illegal") + if err := validator.CheckStructError(&opts).Combine(); err != nil { + return nil, err } ghOptions := &git.RepoInfo{ diff --git a/internal/pkg/plugin/trellogithub/validate.go b/internal/pkg/plugin/trellogithub/validate.go deleted file mode 100644 index a1eb826c0..000000000 --- a/internal/pkg/plugin/trellogithub/validate.go +++ /dev/null @@ -1,9 +0,0 @@ -package trellogithub - -import ( - "github.com/devstream-io/devstream/pkg/util/validator" -) - -func validate(opts *Options) []error { - return validator.Struct(opts) -} diff --git a/pkg/util/helm/validation.go b/pkg/util/helm/validation.go deleted file mode 100644 index c00ac8ba9..000000000 --- a/pkg/util/helm/validation.go +++ /dev/null @@ -1,22 +0,0 @@ -package helm - -import ( - "fmt" - - "github.com/devstream-io/devstream/pkg/util/log" - - "github.com/devstream-io/devstream/pkg/util/validator" -) - -// Validate validates helm param -func Validate(param *HelmParam) []error { - var retErrs = validator.Struct(param) - - if param.Chart.ChartPath == "" && (param.Repo.Name == "" || param.Repo.URL == "" || param.Chart.ChartName == "") { - log.Debugf("Repo.Name: %s, Repo.URL: %s, Chart.ChartName: %s", param.Repo.Name, param.Repo.URL, param.Chart.ChartName) - err := fmt.Errorf("if chartPath == \"\", then the repo.Name & repo.URL & chart.chartName must be set") - retErrs = append(retErrs, err) - } - - return retErrs -} diff --git a/pkg/util/helm/validation_test.go b/pkg/util/helm/validation_test.go deleted file mode 100644 index bdae2b424..000000000 --- a/pkg/util/helm/validation_test.go +++ /dev/null @@ -1,35 +0,0 @@ -package helm - -import ( - "testing" - - "github.com/stretchr/testify/require" -) - -func Test_validate(t *testing.T) { - type args struct { - param *HelmParam - } - tests := []struct { - name string - args args - want int // error count - }{ - // TODO: Add test cases. - {"base", args{&HelmParam{ - Repo{Name: "argo", URL: "https://argoproj.github.io/argo-helm"}, - Chart{ChartName: "argo/argo-cd"}, - }}, 0}, - {"one required field validation error", args{&HelmParam{ - Repo{Name: "argo", URL: "https://argoproj.github.io/argo-helm"}, - Chart{ChartName: ""}, - }}, 1}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got := Validate(tt.args.param) - - require.Lenf(t, got, tt.want, "validate() = %v, want %v", got, tt.want) - }) - } -} diff --git a/pkg/util/template/processor_test.go b/pkg/util/template/processor_test.go index 46ed94532..7a8161c66 100644 --- a/pkg/util/template/processor_test.go +++ b/pkg/util/template/processor_test.go @@ -4,7 +4,6 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" - "github.com/devstream-io/devstream/pkg/util/log" "github.com/devstream-io/devstream/pkg/util/template" ) @@ -145,7 +144,6 @@ var _ = Describe("AddQuoteForVariablesInConfigProcessor", func() { }) It("should do nothing", func() { - log.Infof("------> %s [%s]", string(gotten), expected) Expect(string(gotten)).To(Equal(expected)) }) }) diff --git a/pkg/util/validator/validator.go b/pkg/util/validator/validator.go index af7dac6da..357d42f25 100644 --- a/pkg/util/validator/validator.go +++ b/pkg/util/validator/validator.go @@ -1,8 +1,10 @@ package validator import ( - "errors" + "fmt" "log" + "reflect" + "strings" "gopkg.in/yaml.v3" @@ -12,6 +14,19 @@ import ( var v *validator.Validate +type StructFieldErrors []error + +func (errs StructFieldErrors) Combine() error { + if len(errs) == 0 { + return nil + } + var totalErr = "config options are not valid:\n" + for _, fieldErr := range errs { + totalErr = fmt.Sprintf("%s %s\n", totalErr, fieldErr) + } + return fmt.Errorf(strings.TrimSpace(totalErr)) +} + func init() { v = validator.New() @@ -28,21 +43,29 @@ func init() { log.Fatal(err) } } + v.RegisterTagNameFunc(getMapstructureOrYamlTagName) } -func Struct(s interface{}) []error { +// CheckStructError will check s, and return StructValidationError if this struct is not valid +func CheckStructError(s interface{}) StructFieldErrors { + fieldErrs := make(StructFieldErrors, 0) if err := v.Struct(s); err != nil { - errs := make([]error, 0) - for _, e := range err.(validator.ValidationErrors) { - errs = append(errs, errors.New(e.Error())) + for _, fieldErr := range err.(validator.ValidationErrors) { + var fieldCustomErr error + switch fieldErr.Tag() { + case "required": + fieldCustomErr = fmt.Errorf("field %s is required", fieldErr.Namespace()) + case "url": + fieldCustomErr = fmt.Errorf("field %s is a not valid url", fieldErr.Namespace()) + case "oneof": + fieldCustomErr = fmt.Errorf("field %s must be one of [%s]", fieldErr.Namespace(), fieldErr.Param()) + default: + fieldCustomErr = fmt.Errorf("field %s validation failed on the '%s' tag", fieldErr.Namespace(), fieldErr.Tag()) + } + fieldErrs = append(fieldErrs, fieldCustomErr) } - return errs } - return nil -} - -func StructAllError(s interface{}) error { - return v.Struct(s) + return fieldErrs } func dns1123SubDomain(fl validator.FieldLevel) bool { @@ -52,3 +75,17 @@ func dns1123SubDomain(fl validator.FieldLevel) bool { func isYaml(fl validator.FieldLevel) bool { return yaml.Unmarshal([]byte(fl.Field().String()), &struct{}{}) == nil } + +func getMapstructureOrYamlTagName(fld reflect.StructField) string { + // 1. get tag name from mapstructure or yaml + tagName := fld.Tag.Get("mapstructure") + if tagName == "" { + tagName = fld.Tag.Get("yaml") + } + // 2. else get yaml tag name + name := strings.SplitN(tagName, ",", 2)[0] + if name == "-" { + return "" + } + return name +} diff --git a/pkg/util/validator/validator_suite_test.go b/pkg/util/validator/validator_suite_test.go new file mode 100644 index 000000000..4598b7fbf --- /dev/null +++ b/pkg/util/validator/validator_suite_test.go @@ -0,0 +1,13 @@ +package validator_test + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestPlanmanager(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Pkg Util Validator Suite") +} diff --git a/pkg/util/validator/validator_test.go b/pkg/util/validator/validator_test.go index b38d53ea0..4b1ed3f08 100644 --- a/pkg/util/validator/validator_test.go +++ b/pkg/util/validator/validator_test.go @@ -2,108 +2,113 @@ package validator import ( "reflect" - "testing" "github.com/go-playground/validator/v10" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" ) -type Tool struct { - Name string `yaml:"name" validate:"required"` - InstanceID string `yaml:"instanceID" validate:"required,dns1123subdomain"` - DependsOn []string `yaml:"dependsOn"` - Options map[string]interface{} `yaml:"options"` +type mockTool struct { + Name string `validate:"required"` + InstanceID string `validate:"required,dns1123subdomain"` + URL string `validate:"url"` + TestOne string `validate:"required_without=TestTwo"` + TestTwo string `validate:"required_without=TestOne"` } -func TestStruct(t *testing.T) { - tests := []struct { - name string - s interface{} - wantErrCount int - }{ - // TODO: Add test cases. - {"base", struct{}{}, 0}, - {"base Tool instance", Tool{}, 2}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if got := Struct(tt.s); len(got) != tt.wantErrCount { - t.Errorf("Struct() = %v\n, got err count: %d\n, want err count:%d", got, len(got), tt.wantErrCount) - } - }) - } -} - -type FakerFieldLeveler struct { +type mockFieldLeveler struct { validator.FieldLevel field string } -func (fl *FakerFieldLeveler) Field() reflect.Value { +func (fl *mockFieldLeveler) Field() reflect.Value { return reflect.ValueOf(fl.field) } -func Test_dns1123SubDomain(t *testing.T) { - goodValues, badValues := "a", "" - tests := []struct { - name string - fl validator.FieldLevel - want bool - }{ - // TODO: Add test cases. - {"base", &FakerFieldLeveler{field: goodValues}, true}, - {"base", &FakerFieldLeveler{field: badValues}, false}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if got := dns1123SubDomain(tt.fl); got != tt.want { - t.Errorf("dns1123SubDomain() = %v, want %v", got, tt.want) +var _ = Describe("Check func", func() { + var ( + mockTest *mockTool + ) + When("all field is empty", func() { + BeforeEach(func() { + mockTest = &mockTool{} + }) + It("should return all field error", func() { + errs := CheckStructError(mockTest) + Expect(len(errs)).Should(Equal(5)) + Expect(errs.Combine().Error()).Should(Equal("config options are not valid:\n field mockTool.Name is required\n field mockTool.InstanceID is required\n field mockTool.URL is a not valid url\n field mockTool.TestOne validation failed on the 'required_without' tag\n field mockTool.TestTwo validation failed on the 'required_without' tag")) + }) + }) + When("all field is valid", func() { + BeforeEach(func() { + mockTest = &mockTool{ + Name: "test", + InstanceID: "test", + URL: "http://www.com", + TestOne: "without TestTwo", } }) - } -} + It("should return empty", func() { + errs := CheckStructError(mockTest) + Expect(errs).Should(BeEmpty()) + }) + }) +}) -func Test_isYaml(t *testing.T) { - goodValues := ` ---- -# An employee record -name: Martin D'vloper -foods: - - Apple -languages: - perl: Elite -education: | - 4 GCSEs - 3 A-Levels - BSc in the Internet of Things -` - badValues := ` +var _ = Describe("dns1123SubDomain func", func() { + var ( + testVal *mockFieldLeveler + ) + When("value is valid", func() { + BeforeEach(func() { + testVal = &mockFieldLeveler{field: "a"} + }) + It("should return true", func() { + Expect(dns1123SubDomain(testVal)).Should(BeTrue()) + }) + }) + When("valid is invalid", func() { + BeforeEach(func() { + testVal = &mockFieldLeveler{field: ""} + }) + It("should return false", func() { + Expect(dns1123SubDomain(testVal)).Should(BeFalse()) + }) + }) +}) + +var _ = Describe("isYaml func", func() { + var ( + testVal *mockFieldLeveler + ) + When("value is valid", func() { + BeforeEach(func() { + testVal = &mockFieldLeveler{field: ` +name: Martin D'vloper`} + }) + It("should return true", func() { + Expect(isYaml(testVal)).Should(BeTrue()) + }) + }) + When("valid is invalid", func() { + BeforeEach(func() { + testVal = &mockFieldLeveler{field: ` --- # An employee record job: Developer skill:Elite foods: - - Apple - - Orange +- Apple +- Orange languages: - perl: Elite - python: Elite +perl: Elite +python: Elite education: || - 4 GCSEs -` - tests := []struct { - name string - fl validator.FieldLevel - want bool - }{ - // TODO: Add test cases. - {"base", &FakerFieldLeveler{field: goodValues}, true}, - {"base", &FakerFieldLeveler{field: badValues}, false}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if got := isYaml(tt.fl); got != tt.want { - t.Errorf("isYaml() = %v, want %v", got, tt.want) - } +4 GCSEs +`} }) - } -} + It("should return false", func() { + Expect(isYaml(testVal)).Should(BeFalse()) + }) + }) +})