From 5941c7d5ddf25557875f867325df46fb22316542 Mon Sep 17 00:00:00 2001 From: medmes Date: Fri, 22 Nov 2024 12:27:53 +0100 Subject: [PATCH 01/18] Resolve conflict --- .../service/componentdescriptor/gitsources.go | 2 +- .../service/contentprovider/moduleconfig.go | 12 ++++---- .../service/contentprovider/securityconfig.go | 8 ++--- internal/service/crdparser/crdparser.go | 2 +- internal/service/create/create.go | 30 +++++++++---------- internal/service/create/options.go | 12 ++++---- .../service/filegenerator/filegenerator.go | 6 ++-- internal/service/fileresolver/fileresolver.go | 4 +-- .../generator/moduleconfig_generator.go | 4 +-- .../reader/moduleconfig_reader.go | 2 +- 10 files changed, 41 insertions(+), 41 deletions(-) diff --git a/internal/service/componentdescriptor/gitsources.go b/internal/service/componentdescriptor/gitsources.go index 9c2f8c85..3b2c2319 100644 --- a/internal/service/componentdescriptor/gitsources.go +++ b/internal/service/componentdescriptor/gitsources.go @@ -23,7 +23,7 @@ type GitSourcesService struct { func NewGitSourcesService(gitService GitService) (*GitSourcesService, error) { if gitService == nil { - return nil, fmt.Errorf("%w: gitService must not be nil", commonerrors.ErrInvalidArg) + return nil, fmt.Errorf("gitService must not be nil: %w", commonerrors.ErrInvalidArg) } return &GitSourcesService{ diff --git a/internal/service/contentprovider/moduleconfig.go b/internal/service/contentprovider/moduleconfig.go index fbff00c1..3c5decfc 100644 --- a/internal/service/contentprovider/moduleconfig.go +++ b/internal/service/contentprovider/moduleconfig.go @@ -18,7 +18,7 @@ type ModuleConfigProvider struct { func NewModuleConfigProvider(yamlConverter ObjectToYAMLConverter) (*ModuleConfigProvider, error) { if yamlConverter == nil { - return nil, fmt.Errorf("%w: yamlConverter must not be nil", commonerrors.ErrInvalidArg) + return nil, fmt.Errorf("yamlConverter must not be nil: %w", commonerrors.ErrInvalidArg) } return &ModuleConfigProvider{ @@ -48,19 +48,19 @@ func (s *ModuleConfigProvider) getModuleConfig(args types.KeyValueArgs) ModuleCo func (s *ModuleConfigProvider) validateArgs(args types.KeyValueArgs) error { if args == nil { - return fmt.Errorf("%w: args must not be nil", ErrInvalidArg) + return fmt.Errorf("args must not be nil: %w", ErrInvalidArg) } if value, ok := args[ArgModuleName]; !ok { - return fmt.Errorf("%w: %s", ErrMissingArg, ArgModuleName) + return fmt.Errorf("%s: %w", ArgModuleName, ErrMissingArg) } else if value == "" { - return fmt.Errorf("%w: %s must not be empty", ErrInvalidArg, ArgModuleName) + return fmt.Errorf("%s: %w ,must not be empty", ArgModuleName, ErrInvalidArg) } if value, ok := args[ArgModuleVersion]; !ok { - return fmt.Errorf("%w: %s", ErrMissingArg, ArgModuleVersion) + return fmt.Errorf("%s: %w", ArgModuleVersion, ErrMissingArg) } else if value == "" { - return fmt.Errorf("%w: %s must not be empty", ErrInvalidArg, ArgModuleVersion) + return fmt.Errorf("%s: %w ,must not be empty", ArgModuleVersion, ErrInvalidArg) } return nil diff --git a/internal/service/contentprovider/securityconfig.go b/internal/service/contentprovider/securityconfig.go index 10cb74bb..dd3c3220 100644 --- a/internal/service/contentprovider/securityconfig.go +++ b/internal/service/contentprovider/securityconfig.go @@ -13,7 +13,7 @@ type SecurityConfig struct { func NewSecurityConfig(yamlConverter ObjectToYAMLConverter) (*SecurityConfig, error) { if yamlConverter == nil { - return nil, fmt.Errorf("%w: yamlConverter must not be nil", commonerrors.ErrInvalidArg) + return nil, fmt.Errorf("yamlConverter must not be nil: %w", commonerrors.ErrInvalidArg) } return &SecurityConfig{ @@ -31,16 +31,16 @@ func (s *SecurityConfig) GetDefaultContent(args types.KeyValueArgs) (string, err func (s *SecurityConfig) validateArgs(args types.KeyValueArgs) error { if args == nil { - return fmt.Errorf("%w: args must not be nil", ErrInvalidArg) + return fmt.Errorf("args must not be nil: %w", ErrInvalidArg) } value, ok := args[ArgModuleName] if !ok { - return fmt.Errorf("%w: %s", ErrMissingArg, ArgModuleName) + return fmt.Errorf("%s: %v", ArgModuleName, ErrMissingArg) } if value == "" { - return fmt.Errorf("%w: %s must not be empty", ErrInvalidArg, ArgModuleName) + return fmt.Errorf("%s: %w must not be empty", ArgModuleName, ErrInvalidArg) } return nil diff --git a/internal/service/crdparser/crdparser.go b/internal/service/crdparser/crdparser.go index aaa1cc6b..ccf3c6bc 100644 --- a/internal/service/crdparser/crdparser.go +++ b/internal/service/crdparser/crdparser.go @@ -21,7 +21,7 @@ type Service struct { func NewService(fileSystem FileSystem) (*Service, error) { if fileSystem == nil { - return nil, fmt.Errorf("%w: fileSystem must not be nil", commonerrors.ErrInvalidArg) + return nil, fmt.Errorf("fileSystem must not be nil: %w", commonerrors.ErrInvalidArg) } return &Service{ diff --git a/internal/service/create/create.go b/internal/service/create/create.go index 5765d382..3745e4eb 100644 --- a/internal/service/create/create.go +++ b/internal/service/create/create.go @@ -86,43 +86,43 @@ func NewService(moduleConfigService ModuleConfigService, fileSystem FileSystem, ) (*Service, error) { if moduleConfigService == nil { - return nil, fmt.Errorf("%w: moduleConfigService must not be nil", commonerrors.ErrInvalidArg) + return nil, fmt.Errorf("moduleConfigService must not be nil: %w", commonerrors.ErrInvalidArg) } if gitSourcesService == nil { - return nil, fmt.Errorf("%w: gitSourcesService must not be nil", commonerrors.ErrInvalidArg) + return nil, fmt.Errorf("gitSourcesService must not be nil: %w", commonerrors.ErrInvalidArg) } if securityConfigService == nil { - return nil, fmt.Errorf("%w: securityConfigService must not be nil", commonerrors.ErrInvalidArg) + return nil, fmt.Errorf("securityConfigService must not be nil: %w", commonerrors.ErrInvalidArg) } if componentArchiveService == nil { - return nil, fmt.Errorf("%w: componentArchiveService must not be nil", commonerrors.ErrInvalidArg) + return nil, fmt.Errorf("componentArchiveService must not be nil: %w", commonerrors.ErrInvalidArg) } if registryService == nil { - return nil, fmt.Errorf("%w: registryService must not be nil", commonerrors.ErrInvalidArg) + return nil, fmt.Errorf("registryService must not be nil: %w", commonerrors.ErrInvalidArg) } if moduleTemplateService == nil { - return nil, fmt.Errorf("%w: moduleTemplateService must not be nil", commonerrors.ErrInvalidArg) + return nil, fmt.Errorf("moduleTemplateService must not be nil: %w", commonerrors.ErrInvalidArg) } if crdParserService == nil { - return nil, fmt.Errorf("%w: crdParserService must not be nil", commonerrors.ErrInvalidArg) + return nil, fmt.Errorf("crdParserService must not be nil: %w", commonerrors.ErrInvalidArg) } if manifestFileResolver == nil { - return nil, fmt.Errorf("%w: manifestFileResolver must not be nil", commonerrors.ErrInvalidArg) + return nil, fmt.Errorf("manifestFileResolver must not be nil: %w", commonerrors.ErrInvalidArg) } if defaultCRFileResolver == nil { - return nil, fmt.Errorf("%w: defaultCRFileResolver must not be nil", commonerrors.ErrInvalidArg) + return nil, fmt.Errorf("defaultCRFileResolver must not be nil: %w", commonerrors.ErrInvalidArg) } if fileSystem == nil { - return nil, fmt.Errorf("%w: fileSystem must not be nil", commonerrors.ErrInvalidArg) + return nil, fmt.Errorf("fileSystem must not be nil: %w", commonerrors.ErrInvalidArg) } return &Service{ @@ -219,19 +219,19 @@ func (s *Service) pushImgAndCreateTemplate(archive *comparch.ComponentArchive, m if err := s.registryService.PushComponentVersion(archive, opts.Insecure, opts.Credentials, opts.RegistryURL); err != nil { - return fmt.Errorf("%w: failed to push component archive", err) + return fmt.Errorf("failed to push component archive: %w", err) } componentVersionAccess, err := s.registryService.GetComponentVersion(archive, opts.Insecure, opts.Credentials, opts.RegistryURL) if err != nil { - return fmt.Errorf("%w: failed to get component version", err) + return fmt.Errorf("failed to get component version: %w", err) } var crData []byte if defaultCRFilePath != "" { crData, err = s.fileSystem.ReadFile(defaultCRFilePath) if err != nil { - return fmt.Errorf("%w: failed to get default CR data", err) + return fmt.Errorf("failed to get default CR data: %w", err) } } @@ -239,7 +239,7 @@ func (s *Service) pushImgAndCreateTemplate(archive *comparch.ComponentArchive, m descriptor := componentVersionAccess.GetDescriptor() if err = s.moduleTemplateService.GenerateModuleTemplate(moduleConfig, descriptor, crData, isCRDClusterScoped, opts.TemplateOutput); err != nil { - return fmt.Errorf("%w: failed to generate module template", err) + return fmt.Errorf("failed to generate module template: %w", err) } return nil } @@ -252,7 +252,7 @@ func (s *Service) configureSecScannerConf(descriptor *compdesc.ComponentDescript } if err = s.securityConfigService.AppendSecurityScanConfig(descriptor, *securityConfig); err != nil { - return fmt.Errorf("%w: failed to append security scan config", err) + return fmt.Errorf("failed to append security scan config: %w", err) } return nil } diff --git a/internal/service/create/options.go b/internal/service/create/options.go index f54815c7..97357979 100644 --- a/internal/service/create/options.go +++ b/internal/service/create/options.go @@ -21,28 +21,28 @@ type Options struct { func (opts Options) Validate() error { if opts.Out == nil { - return fmt.Errorf("%w: opts.Out must not be nil", commonerrors.ErrInvalidOption) + return fmt.Errorf("opts.Out must not be nil: %w", commonerrors.ErrInvalidOption) } if opts.ConfigFile == "" { - return fmt.Errorf("%w: opts.ConfigFile must not be empty", commonerrors.ErrInvalidOption) + return fmt.Errorf("opts.ConfigFile must not be empty: %w", commonerrors.ErrInvalidOption) } if opts.Credentials != "" { matched, err := regexp.MatchString("(.+):(.+)", opts.Credentials) if err != nil { - return fmt.Errorf("%w: opts.Credentials could not be parsed: %w", commonerrors.ErrInvalidOption, err) + return fmt.Errorf("opts.Credentials could not be parsed: %w", commonerrors.ErrInvalidOption, err) } else if !matched { - return fmt.Errorf("%w: opts.Credentials is in invalid format", commonerrors.ErrInvalidOption) + return fmt.Errorf("opts.Credentials is in invalid format: %w", commonerrors.ErrInvalidOption) } } if opts.TemplateOutput == "" { - return fmt.Errorf("%w: opts.TemplateOutput must not be empty", commonerrors.ErrInvalidOption) + return fmt.Errorf("opts.TemplateOutput must not be empty: %w", commonerrors.ErrInvalidOption) } if opts.RegistryURL != "" && !strings.HasPrefix(opts.RegistryURL, "http") { - return fmt.Errorf("%w: opts.RegistryURL does not start with http(s)", commonerrors.ErrInvalidOption) + return fmt.Errorf("opts.RegistryURL does not start with http(s): %w", commonerrors.ErrInvalidOption) } return nil diff --git a/internal/service/filegenerator/filegenerator.go b/internal/service/filegenerator/filegenerator.go index 5d1967b0..e04d5e61 100644 --- a/internal/service/filegenerator/filegenerator.go +++ b/internal/service/filegenerator/filegenerator.go @@ -25,15 +25,15 @@ type Service struct { func NewService(kind string, fileSystem FileWriter, defaultContentProvider DefaultContentProvider) (*Service, error) { if kind == "" { - return nil, fmt.Errorf("%w: kind must not be empty", commonerrors.ErrInvalidArg) + return nil, fmt.Errorf("kind must not be empty: %w", commonerrors.ErrInvalidArg) } if fileSystem == nil { - return nil, fmt.Errorf("%w: fileSystem must not be nil", commonerrors.ErrInvalidArg) + return nil, fmt.Errorf("fileSystem must not be nil: %w", commonerrors.ErrInvalidArg) } if defaultContentProvider == nil { - return nil, fmt.Errorf("%w: defaultContentProvider must not be nil", commonerrors.ErrInvalidArg) + return nil, fmt.Errorf("defaultContentProvider must not be nil: %w", commonerrors.ErrInvalidArg) } return &Service{ diff --git a/internal/service/fileresolver/fileresolver.go b/internal/service/fileresolver/fileresolver.go index ff3263a7..340cd4bd 100644 --- a/internal/service/fileresolver/fileresolver.go +++ b/internal/service/fileresolver/fileresolver.go @@ -19,11 +19,11 @@ type FileResolver struct { func NewFileResolver(filePattern string, tempFileSystem TempFileSystem) (*FileResolver, error) { if filePattern == "" { - return nil, fmt.Errorf("%w: filePattern must not be empty", commonerrors.ErrInvalidArg) + return nil, fmt.Errorf("filePattern must not be empty: %w", commonerrors.ErrInvalidArg) } if tempFileSystem == nil { - return nil, fmt.Errorf("%w: tempFileSystem must not be nil", commonerrors.ErrInvalidArg) + return nil, fmt.Errorf("tempFileSystem must not be nil: %w", commonerrors.ErrInvalidArg) } return &FileResolver{ diff --git a/internal/service/moduleconfig/generator/moduleconfig_generator.go b/internal/service/moduleconfig/generator/moduleconfig_generator.go index 6b51d225..8dda438d 100644 --- a/internal/service/moduleconfig/generator/moduleconfig_generator.go +++ b/internal/service/moduleconfig/generator/moduleconfig_generator.go @@ -25,11 +25,11 @@ type Service struct { func NewService(fileSystem FileSystem, fileGenerator FileGenerator) (*Service, error) { if fileSystem == nil { - return nil, fmt.Errorf("%w: fileSystem must not be nil", commonerrors.ErrInvalidArg) + return nil, fmt.Errorf("fileSystem must not be nil: %w", commonerrors.ErrInvalidArg) } if fileGenerator == nil { - return nil, fmt.Errorf("%w: fileGenerator must not be nil", commonerrors.ErrInvalidArg) + return nil, fmt.Errorf("fileGenerator must not be nil: %w", commonerrors.ErrInvalidArg) } return &Service{ diff --git a/internal/service/moduleconfig/reader/moduleconfig_reader.go b/internal/service/moduleconfig/reader/moduleconfig_reader.go index df32ab14..3b93f192 100644 --- a/internal/service/moduleconfig/reader/moduleconfig_reader.go +++ b/internal/service/moduleconfig/reader/moduleconfig_reader.go @@ -69,7 +69,7 @@ func ValidateModuleConfig(moduleConfig *contentprovider.ModuleConfig) error { } if len(moduleConfig.Icons) == 0 { - return fmt.Errorf("failed to validate module icons: %w: must contain at least one icon", + return fmt.Errorf("failed to validate module icons: %v: must contain at least one icon", commonerrors.ErrInvalidOption) } From 2a59f08bf94a13ff272b902df4ffae7397f87a3f Mon Sep 17 00:00:00 2001 From: medmes Date: Fri, 22 Nov 2024 16:32:59 +0100 Subject: [PATCH 02/18] Added error wrapping changes ##105 --- cmd/modulectl/create/cmd.go | 5 ++--- cmd/modulectl/scaffold/cmd.go | 5 ++--- internal/common/validation/validation.go | 16 ++++++++-------- .../service/contentprovider/securityconfig.go | 2 +- 4 files changed, 13 insertions(+), 15 deletions(-) diff --git a/cmd/modulectl/create/cmd.go b/cmd/modulectl/create/cmd.go index 549b166a..a5ac6b52 100644 --- a/cmd/modulectl/create/cmd.go +++ b/cmd/modulectl/create/cmd.go @@ -1,6 +1,7 @@ package create import ( + _ "embed" "fmt" "github.com/spf13/cobra" @@ -8,8 +9,6 @@ import ( commonerrors "github.com/kyma-project/modulectl/internal/common/errors" "github.com/kyma-project/modulectl/internal/service/create" iotools "github.com/kyma-project/modulectl/tools/io" - - _ "embed" ) //go:embed use.txt @@ -30,7 +29,7 @@ type Service interface { func NewCmd(service Service) (*cobra.Command, error) { if service == nil { - return nil, fmt.Errorf("%w: service must not be nil", commonerrors.ErrInvalidArg) + return nil, fmt.Errorf("service must not be nil: %w", commonerrors.ErrInvalidArg) } opts := create.Options{} diff --git a/cmd/modulectl/scaffold/cmd.go b/cmd/modulectl/scaffold/cmd.go index 2ec3c380..6bede5b1 100644 --- a/cmd/modulectl/scaffold/cmd.go +++ b/cmd/modulectl/scaffold/cmd.go @@ -1,6 +1,7 @@ package scaffold import ( + _ "embed" "fmt" "github.com/spf13/cobra" @@ -8,8 +9,6 @@ import ( commonerrors "github.com/kyma-project/modulectl/internal/common/errors" "github.com/kyma-project/modulectl/internal/service/scaffold" iotools "github.com/kyma-project/modulectl/tools/io" - - _ "embed" ) //go:embed use.txt @@ -30,7 +29,7 @@ type Service interface { func NewCmd(service Service) (*cobra.Command, error) { if service == nil { - return nil, fmt.Errorf("%w: service must not be nil", commonerrors.ErrInvalidArg) + return nil, fmt.Errorf("service must not be nil: %w", commonerrors.ErrInvalidArg) } opts := scaffold.Options{} diff --git a/internal/common/validation/validation.go b/internal/common/validation/validation.go index df1ca095..66dc0f24 100644 --- a/internal/common/validation/validation.go +++ b/internal/common/validation/validation.go @@ -21,7 +21,7 @@ const ( func ValidateModuleName(name string) error { if name == "" { - return fmt.Errorf("%w: opts.ModuleName must not be empty", commonerrors.ErrInvalidOption) + return fmt.Errorf("opts.ModuleName must not be empty: %w", commonerrors.ErrInvalidOption) } if len(name) > moduleNameMaxLength { @@ -30,9 +30,9 @@ func ValidateModuleName(name string) error { } if matched, err := regexp.MatchString(moduleNamePattern, name); err != nil { - return fmt.Errorf("%w: failed to evaluate regex pattern for opts.ModuleName", commonerrors.ErrInvalidOption) + return fmt.Errorf("failed to evaluate regex pattern for opts.ModuleName: %w", commonerrors.ErrInvalidOption) } else if !matched { - return fmt.Errorf("%w: opts.ModuleName must match the required pattern, e.g: 'github.com/path-to/your-repo'", + return fmt.Errorf("opts.ModuleName must match the required pattern, e.g: 'github.com/path-to/your-repo': %w", commonerrors.ErrInvalidOption) } @@ -41,7 +41,7 @@ func ValidateModuleName(name string) error { func ValidateModuleVersion(version string) error { if version == "" { - return fmt.Errorf("%w: opts.ModuleVersion must not be empty", commonerrors.ErrInvalidOption) + return fmt.Errorf("opts.ModuleVersion must not be empty: %w", commonerrors.ErrInvalidOption) } if err := validateSemanticVersion(version); err != nil { @@ -73,7 +73,7 @@ func ValidateNamespace(namespace string) error { if matched, err := regexp.MatchString(namespacePattern, namespace); err != nil { return fmt.Errorf("failed to evaluate regex pattern for module namespace: %w", err) } else if !matched { - return fmt.Errorf("%w: namespace must match the required pattern, only small alphanumeric characters and hyphens", + return fmt.Errorf("namespace must match the required pattern, only small alphanumeric characters and hyphens: %w", commonerrors.ErrInvalidOption) } @@ -83,11 +83,11 @@ func ValidateNamespace(namespace string) error { func ValidateMapEntries(nameLinkMap map[string]string) error { for name, link := range nameLinkMap { if name == "" { - return fmt.Errorf("%w: name must not be empty", commonerrors.ErrInvalidOption) + return fmt.Errorf("name must not be empty: %w", commonerrors.ErrInvalidOption) } if link == "" { - return fmt.Errorf("%w: link must not be empty", commonerrors.ErrInvalidOption) + return fmt.Errorf("link must not be empty: %w", commonerrors.ErrInvalidOption) } if err := ValidateIsValidHTTPSURL(link); err != nil { @@ -100,7 +100,7 @@ func ValidateMapEntries(nameLinkMap map[string]string) error { func ValidateIsValidHTTPSURL(input string) error { if input == "" { - return fmt.Errorf("%w: must not be empty", commonerrors.ErrInvalidOption) + return fmt.Errorf("must not be empty: %w", commonerrors.ErrInvalidOption) } _url, err := url.Parse(input) diff --git a/internal/service/contentprovider/securityconfig.go b/internal/service/contentprovider/securityconfig.go index dd3c3220..ad327da4 100644 --- a/internal/service/contentprovider/securityconfig.go +++ b/internal/service/contentprovider/securityconfig.go @@ -36,7 +36,7 @@ func (s *SecurityConfig) validateArgs(args types.KeyValueArgs) error { value, ok := args[ArgModuleName] if !ok { - return fmt.Errorf("%s: %v", ArgModuleName, ErrMissingArg) + return fmt.Errorf("%s: %w", ArgModuleName, ErrMissingArg) } if value == "" { From f412972fb60c676e703f4965f6ac5f5ae4a89740 Mon Sep 17 00:00:00 2001 From: medmes Date: Mon, 25 Nov 2024 17:21:51 +0100 Subject: [PATCH 03/18] Fix unit testing and wrapped errors #105 --- internal/service/create/options.go | 2 +- internal/service/moduleconfig/reader/moduleconfig_reader.go | 2 +- .../service/moduleconfig/reader/moduleconfig_reader_test.go | 3 ++- internal/service/scaffold/errors.go | 2 +- 4 files changed, 5 insertions(+), 4 deletions(-) diff --git a/internal/service/create/options.go b/internal/service/create/options.go index 97357979..765dba32 100644 --- a/internal/service/create/options.go +++ b/internal/service/create/options.go @@ -31,7 +31,7 @@ func (opts Options) Validate() error { if opts.Credentials != "" { matched, err := regexp.MatchString("(.+):(.+)", opts.Credentials) if err != nil { - return fmt.Errorf("opts.Credentials could not be parsed: %w", commonerrors.ErrInvalidOption, err) + return fmt.Errorf("%w: opts.Credentials could not be parsed: %w", commonerrors.ErrInvalidOption, err) } else if !matched { return fmt.Errorf("opts.Credentials is in invalid format: %w", commonerrors.ErrInvalidOption) } diff --git a/internal/service/moduleconfig/reader/moduleconfig_reader.go b/internal/service/moduleconfig/reader/moduleconfig_reader.go index 3b93f192..df32ab14 100644 --- a/internal/service/moduleconfig/reader/moduleconfig_reader.go +++ b/internal/service/moduleconfig/reader/moduleconfig_reader.go @@ -69,7 +69,7 @@ func ValidateModuleConfig(moduleConfig *contentprovider.ModuleConfig) error { } if len(moduleConfig.Icons) == 0 { - return fmt.Errorf("failed to validate module icons: %v: must contain at least one icon", + return fmt.Errorf("failed to validate module icons: %w: must contain at least one icon", commonerrors.ErrInvalidOption) } diff --git a/internal/service/moduleconfig/reader/moduleconfig_reader_test.go b/internal/service/moduleconfig/reader/moduleconfig_reader_test.go index cf6a4268..41b18b4b 100644 --- a/internal/service/moduleconfig/reader/moduleconfig_reader_test.go +++ b/internal/service/moduleconfig/reader/moduleconfig_reader_test.go @@ -357,7 +357,8 @@ func Test_ValidateModuleConfig(t *testing.T) { t.Run(test.name, func(t *testing.T) { err := moduleconfigreader.ValidateModuleConfig(test.moduleConfig) if test.expectedError != nil { - require.ErrorContains(t, err, test.expectedError.Error()) + require.ErrorAs(t, err, &test.expectedError) + require.ErrorIs(t, err, test.expectedError, test.expectedError.Error()) return } require.NoError(t, err) diff --git a/internal/service/scaffold/errors.go b/internal/service/scaffold/errors.go index 91922d10..e4042dbb 100644 --- a/internal/service/scaffold/errors.go +++ b/internal/service/scaffold/errors.go @@ -4,5 +4,5 @@ import "errors" var ( ErrGeneratingFile = errors.New("error generating file") - ErrOverwritingFile = errors.New("%w: error overwriting file") + ErrOverwritingFile = errors.New("error overwriting file") ) From b4d92da8fabc6980ae6776572394d789ab874d69 Mon Sep 17 00:00:00 2001 From: medmes Date: Tue, 26 Nov 2024 09:28:31 +0100 Subject: [PATCH 04/18] Fix e2e testing --- tests/e2e/create/create_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/e2e/create/create_test.go b/tests/e2e/create/create_test.go index e680fc28..a412e183 100644 --- a/tests/e2e/create/create_test.go +++ b/tests/e2e/create/create_test.go @@ -47,7 +47,7 @@ var _ = Describe("Test 'create' command", Ordered, func() { It("Then the command should fail", func() { err := cmd.execute() Expect(err).Should(HaveOccurred()) - Expect(err.Error()).Should(ContainSubstring("invalid Option: opts.ModuleName must not be empty")) + Expect(err.Error()).Should(ContainSubstring("opts.ModuleName must not be empty: invalid Option")) }) }) @@ -61,7 +61,7 @@ var _ = Describe("Test 'create' command", Ordered, func() { It("Then the command should fail", func() { err := cmd.execute() Expect(err).Should(HaveOccurred()) - Expect(err.Error()).Should(ContainSubstring("invalid Option: opts.ModuleVersion must not be empty")) + Expect(err.Error()).Should(ContainSubstring("opts.ModuleVersion must not be empty: invalid Option")) }) }) @@ -75,7 +75,7 @@ var _ = Describe("Test 'create' command", Ordered, func() { It("Then the command should fail", func() { err := cmd.execute() Expect(err).Should(HaveOccurred()) - Expect(err.Error()).Should(ContainSubstring("failed to parse module config: failed to validate module config: failed to validate manifest: invalid Option: must not be empty")) + Expect(err.Error()).Should(ContainSubstring("failed to parse module config: failed to validate module config: failed to validate manifest: must not be empty: invalid Option")) }) }) From 5f1c9426ea823fcca0a9dacf87e90d13ed2ad3e1 Mon Sep 17 00:00:00 2001 From: medmes Date: Tue, 26 Nov 2024 14:50:36 +0100 Subject: [PATCH 05/18] Fix e2e testing --- internal/common/validation/validation.go | 17 ++++++++-------- internal/service/registry/registry.go | 2 +- tests/e2e/create/create_test.go | 26 ++++++++++++------------ tools/ocirepo/ocirepo.go | 4 ++-- 4 files changed, 24 insertions(+), 25 deletions(-) diff --git a/internal/common/validation/validation.go b/internal/common/validation/validation.go index 66dc0f24..e3451f1a 100644 --- a/internal/common/validation/validation.go +++ b/internal/common/validation/validation.go @@ -25,8 +25,7 @@ func ValidateModuleName(name string) error { } if len(name) > moduleNameMaxLength { - return fmt.Errorf("%w: opts.ModuleName length must not exceed %q characters", commonerrors.ErrInvalidOption, - moduleNameMaxLength) + return fmt.Errorf("opts.ModuleName length must not exceed %q characters: %w", moduleNameMaxLength, commonerrors.ErrInvalidOption) } if matched, err := regexp.MatchString(moduleNamePattern, name); err != nil { @@ -53,7 +52,7 @@ func ValidateModuleVersion(version string) error { func ValidateModuleNamespace(namespace string) error { if namespace == "" { - return fmt.Errorf("%w: opts.ModuleNamespace must not be empty", commonerrors.ErrInvalidOption) + return fmt.Errorf("opts.ModuleNamespace must not be empty: %w", commonerrors.ErrInvalidOption) } if err := ValidateNamespace(namespace); err != nil { @@ -65,9 +64,9 @@ func ValidateModuleNamespace(namespace string) error { func ValidateNamespace(namespace string) error { if len(namespace) > namespaceMaxLength { - return fmt.Errorf("%w: opts.ModuleNamespace length must not exceed %q characters", - commonerrors.ErrInvalidOption, - namespaceMaxLength) + return fmt.Errorf("opts.ModuleNamespace length must not exceed %q characters: %w", + namespaceMaxLength, + commonerrors.ErrInvalidOption) } if matched, err := regexp.MatchString(namespacePattern, namespace); err != nil { @@ -105,11 +104,11 @@ func ValidateIsValidHTTPSURL(input string) error { _url, err := url.Parse(input) if err != nil { - return fmt.Errorf("%w: '%s' is not a valid URL", commonerrors.ErrInvalidOption, input) + return fmt.Errorf("'%s' is not a valid URL: %w", input, commonerrors.ErrInvalidOption) } if _url.Scheme != "https" { - return fmt.Errorf("%w: '%s' is not using https scheme", commonerrors.ErrInvalidOption, input) + return fmt.Errorf("'%s' is not using https scheme: %w", input, commonerrors.ErrInvalidOption) } return nil @@ -118,7 +117,7 @@ func ValidateIsValidHTTPSURL(input string) error { func validateSemanticVersion(version string) error { _, err := semver.StrictNewVersion(strings.TrimSpace(version)) if err != nil { - return fmt.Errorf("%w: opts.ModuleVersion failed to parse as semantic version: %w", + return fmt.Errorf("opts.ModuleVersion failed to parse as semantic version: %w: %w", commonerrors.ErrInvalidOption, err) } diff --git a/internal/service/registry/registry.go b/internal/service/registry/registry.go index b1c6b139..54a89786 100644 --- a/internal/service/registry/registry.go +++ b/internal/service/registry/registry.go @@ -29,7 +29,7 @@ type Service struct { func NewService(ociRepository OCIRepository, repo cpi.Repository) (*Service, error) { if ociRepository == nil { - return nil, fmt.Errorf("%w: ociRepository must not be nil", commonerrors.ErrInvalidArg) + return nil, fmt.Errorf("ociRepository must not be nil: %w", commonerrors.ErrInvalidArg) } return &Service{ diff --git a/tests/e2e/create/create_test.go b/tests/e2e/create/create_test.go index a412e183..c08ae38e 100644 --- a/tests/e2e/create/create_test.go +++ b/tests/e2e/create/create_test.go @@ -89,7 +89,7 @@ var _ = Describe("Test 'create' command", Ordered, func() { It("Then the command should fail", func() { err := cmd.execute() Expect(err).Should(HaveOccurred()) - Expect(err.Error()).Should(ContainSubstring("failed to parse module config: failed to validate module config: failed to validate repository: invalid Option: must not be empty")) + Expect(err.Error()).Should(ContainSubstring("failed to parse module config: failed to validate module config: failed to validate repository: must not be empty: invalid Option")) }) }) @@ -103,7 +103,7 @@ var _ = Describe("Test 'create' command", Ordered, func() { It("Then the command should fail", func() { err := cmd.execute() Expect(err).Should(HaveOccurred()) - Expect(err.Error()).Should(ContainSubstring("failed to parse module config: failed to validate module config: failed to validate documentation: invalid Option: must not be empty")) + Expect(err.Error()).Should(ContainSubstring("failed to parse module config: failed to validate module config: failed to validate documentation: must not be empty: invalid Option")) }) }) @@ -117,7 +117,7 @@ var _ = Describe("Test 'create' command", Ordered, func() { It("Then the command should fail", func() { err := cmd.execute() Expect(err).Should(HaveOccurred()) - Expect(err.Error()).Should(ContainSubstring("failed to parse module config: failed to validate module config: failed to validate repository: invalid Option: 'http://github.com/kyma-project/template-operator' is not using https scheme")) + Expect(err.Error()).Should(ContainSubstring("failed to parse module config: failed to validate module config: failed to validate repository: 'http://github.com/kyma-project/template-operator' is not using https scheme: invalid Option")) }) }) @@ -131,7 +131,7 @@ var _ = Describe("Test 'create' command", Ordered, func() { It("Then the command should fail", func() { err := cmd.execute() Expect(err).Should(HaveOccurred()) - Expect(err.Error()).Should(ContainSubstring("failed to parse module config: failed to validate module config: failed to validate documentation: invalid Option: 'http://github.com/kyma-project/template-operator/blob/main/README.md' is not using https scheme")) + Expect(err.Error()).Should(ContainSubstring("failed to parse module config: failed to validate module config: failed to validate documentation: 'http://github.com/kyma-project/template-operator/blob/main/README.md' is not using https scheme: invalid Option")) }) }) @@ -173,7 +173,7 @@ var _ = Describe("Test 'create' command", Ordered, func() { It("Then the command should fail", func() { err := cmd.execute() Expect(err).Should(HaveOccurred()) - Expect(err.Error()).Should(ContainSubstring("failed to parse module config: failed to validate module config: failed to validate module icons: invalid Option: link must not be empty")) + Expect(err.Error()).Should(ContainSubstring("failed to parse module config: failed to validate module config: failed to validate module icons: link must not be empty: invalid Option")) }) }) @@ -187,7 +187,7 @@ var _ = Describe("Test 'create' command", Ordered, func() { It("Then the command should fail", func() { err := cmd.execute() Expect(err).Should(HaveOccurred()) - Expect(err.Error()).Should(ContainSubstring("failed to parse module config: failed to validate module config: failed to validate module icons: invalid Option: name must not be empty")) + Expect(err.Error()).Should(ContainSubstring("failed to parse module config: failed to validate module config: failed to validate module icons: name must not be empty: invalid Option")) }) }) @@ -215,7 +215,7 @@ var _ = Describe("Test 'create' command", Ordered, func() { It("Then the command should fail", func() { err := cmd.execute() Expect(err).Should(HaveOccurred()) - Expect(err.Error()).Should(ContainSubstring("failed to parse module config: failed to validate module config: failed to validate resources: failed to validate link: invalid Option: 'http://some.other/location/template-operator.yaml' is not using https scheme")) + Expect(err.Error()).Should(ContainSubstring("failed to parse module config: failed to validate module config: failed to validate resources: failed to validate link: 'http://some.other/location/template-operator.yaml' is not using https scheme: invalid Option")) }) }) @@ -229,7 +229,7 @@ var _ = Describe("Test 'create' command", Ordered, func() { It("Then the command should fail", func() { err := cmd.execute() Expect(err).Should(HaveOccurred()) - Expect(err.Error()).Should(ContainSubstring("failed to parse module config: failed to validate module config: failed to validate resources: invalid Option: link must not be empty")) + Expect(err.Error()).Should(ContainSubstring("failed to parse module config: failed to validate module config: failed to validate resources: link must not be empty: invalid Option")) }) }) @@ -243,7 +243,7 @@ var _ = Describe("Test 'create' command", Ordered, func() { It("Then the command should fail", func() { err := cmd.execute() Expect(err).Should(HaveOccurred()) - Expect(err.Error()).Should(ContainSubstring("failed to parse module config: failed to validate module config: failed to validate resources: invalid Option: name must not be empty")) + Expect(err.Error()).Should(ContainSubstring("failed to parse module config: failed to validate module config: failed to validate resources: name must not be empty: invalid Option")) }) }) @@ -275,7 +275,7 @@ var _ = Describe("Test 'create' command", Ordered, func() { It("Then the command should fail", func() { err := cmd.execute() Expect(err).Should(HaveOccurred()) - Expect(err.Error()).Should(ContainSubstring("Error: could not push")) + Expect(err.Error()).Should(ContainSubstring("could not push")) }) }) @@ -376,7 +376,7 @@ var _ = Describe("Test 'create' command", Ordered, func() { }) It("Then the command should fail with same version exists message", func() { err := cmd.execute() - Expect(err.Error()).Should(ContainSubstring("could not push component version: component version already exists")) + Expect(err.Error()).Should(ContainSubstring("could not push component version: cannot push component version 1.0.0: component version already exists, cannot push the new version")) }) }) @@ -783,7 +783,7 @@ var _ = Describe("Test 'create' command", Ordered, func() { It("Then the command should fail", func() { err := cmd.execute() Expect(err).Should(HaveOccurred()) - Expect(err.Error()).Should(ContainSubstring("failed to parse module config: failed to validate module config: failed to validate manifest: invalid Option: './template-operator.yaml' is not using https scheme")) + Expect(err.Error()).Should(ContainSubstring("failed to parse module config: failed to validate module config: failed to validate manifest: './template-operator.yaml' is not using https scheme: invalid Option")) }) }) @@ -797,7 +797,7 @@ var _ = Describe("Test 'create' command", Ordered, func() { It("Then the command should fail", func() { err := cmd.execute() Expect(err).Should(HaveOccurred()) - Expect(err.Error()).Should(ContainSubstring("failed to parse module config: failed to validate module config: failed to validate default CR: invalid Option: '/tmp/default-sample-cr.yaml' is not using https scheme")) + Expect(err.Error()).Should(ContainSubstring("failed to parse module config: failed to validate module config: failed to validate default CR: '/tmp/default-sample-cr.yaml' is not using https scheme: invalid Option")) }) }) }) diff --git a/tools/ocirepo/ocirepo.go b/tools/ocirepo/ocirepo.go index 8fce224a..ee27afcc 100644 --- a/tools/ocirepo/ocirepo.go +++ b/tools/ocirepo/ocirepo.go @@ -29,8 +29,8 @@ func (o *OCIRepo) GetComponentVersion(archive *comparch.ComponentArchive, func (o *OCIRepo) PushComponentVersionIfNotExist(archive *comparch.ComponentArchive, repo cpi.Repository) error { if exists, _ := repo.ExistsComponentVersion(archive.GetName(), archive.GetVersion()); exists { - return fmt.Errorf("%w: cannot push component version %s", - errComponentVersionAlreadyExists, archive.GetVersion()) + return fmt.Errorf("cannot push component version %s: %w", + archive.GetVersion(), errComponentVersionAlreadyExists) } transferHandler, err := standard.New() From da3ba26dd016a40c4bb08f9d508fc2cba4513952 Mon Sep 17 00:00:00 2001 From: medmes Date: Tue, 26 Nov 2024 21:28:24 +0100 Subject: [PATCH 06/18] remove assertion duplication as require.errorAs is included in the require.errorIs --- cmd/modulectl/create/cmd.go | 3 ++- cmd/modulectl/scaffold/cmd.go | 3 ++- .../service/moduleconfig/reader/moduleconfig_reader_test.go | 1 - 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/cmd/modulectl/create/cmd.go b/cmd/modulectl/create/cmd.go index a5ac6b52..c10f323f 100644 --- a/cmd/modulectl/create/cmd.go +++ b/cmd/modulectl/create/cmd.go @@ -1,7 +1,6 @@ package create import ( - _ "embed" "fmt" "github.com/spf13/cobra" @@ -9,6 +8,8 @@ import ( commonerrors "github.com/kyma-project/modulectl/internal/common/errors" "github.com/kyma-project/modulectl/internal/service/create" iotools "github.com/kyma-project/modulectl/tools/io" + + _ "embed" ) //go:embed use.txt diff --git a/cmd/modulectl/scaffold/cmd.go b/cmd/modulectl/scaffold/cmd.go index 6bede5b1..b0feb965 100644 --- a/cmd/modulectl/scaffold/cmd.go +++ b/cmd/modulectl/scaffold/cmd.go @@ -1,7 +1,6 @@ package scaffold import ( - _ "embed" "fmt" "github.com/spf13/cobra" @@ -9,6 +8,8 @@ import ( commonerrors "github.com/kyma-project/modulectl/internal/common/errors" "github.com/kyma-project/modulectl/internal/service/scaffold" iotools "github.com/kyma-project/modulectl/tools/io" + + _ "embed" ) //go:embed use.txt diff --git a/internal/service/moduleconfig/reader/moduleconfig_reader_test.go b/internal/service/moduleconfig/reader/moduleconfig_reader_test.go index 41b18b4b..cde58740 100644 --- a/internal/service/moduleconfig/reader/moduleconfig_reader_test.go +++ b/internal/service/moduleconfig/reader/moduleconfig_reader_test.go @@ -357,7 +357,6 @@ func Test_ValidateModuleConfig(t *testing.T) { t.Run(test.name, func(t *testing.T) { err := moduleconfigreader.ValidateModuleConfig(test.moduleConfig) if test.expectedError != nil { - require.ErrorAs(t, err, &test.expectedError) require.ErrorIs(t, err, test.expectedError, test.expectedError.Error()) return } From 497df2f3cbae2f9dc7fcbfe1c1acd2a447bf5221 Mon Sep 17 00:00:00 2001 From: medmes Date: Tue, 26 Nov 2024 23:34:56 +0100 Subject: [PATCH 07/18] resolve error messages --- internal/common/validation/validation.go | 4 +-- .../reader/moduleconfig_reader.go | 2 +- .../reader/moduleconfig_reader_test.go | 36 +++++++++---------- tests/e2e/create/create_test.go | 2 +- 4 files changed, 22 insertions(+), 22 deletions(-) diff --git a/internal/common/validation/validation.go b/internal/common/validation/validation.go index e3451f1a..7a49377e 100644 --- a/internal/common/validation/validation.go +++ b/internal/common/validation/validation.go @@ -117,8 +117,8 @@ func ValidateIsValidHTTPSURL(input string) error { func validateSemanticVersion(version string) error { _, err := semver.StrictNewVersion(strings.TrimSpace(version)) if err != nil { - return fmt.Errorf("opts.ModuleVersion failed to parse as semantic version: %w: %w", - commonerrors.ErrInvalidOption, err) + return fmt.Errorf("opts.ModuleVersion failed to parse as semantic version: %w", + commonerrors.ErrInvalidOption) } return nil diff --git a/internal/service/moduleconfig/reader/moduleconfig_reader.go b/internal/service/moduleconfig/reader/moduleconfig_reader.go index df32ab14..8e40f9d5 100644 --- a/internal/service/moduleconfig/reader/moduleconfig_reader.go +++ b/internal/service/moduleconfig/reader/moduleconfig_reader.go @@ -69,7 +69,7 @@ func ValidateModuleConfig(moduleConfig *contentprovider.ModuleConfig) error { } if len(moduleConfig.Icons) == 0 { - return fmt.Errorf("failed to validate module icons: %w: must contain at least one icon", + return fmt.Errorf("failed to validate module icons: must contain at least one icon: %w", commonerrors.ErrInvalidOption) } diff --git a/internal/service/moduleconfig/reader/moduleconfig_reader_test.go b/internal/service/moduleconfig/reader/moduleconfig_reader_test.go index cde58740..2f2877a1 100644 --- a/internal/service/moduleconfig/reader/moduleconfig_reader_test.go +++ b/internal/service/moduleconfig/reader/moduleconfig_reader_test.go @@ -88,7 +88,7 @@ func Test_ValidateModuleConfig(t *testing.T) { "module-icon": "https://example.com/path/to/some-icon", }, }, - expectedError: fmt.Errorf("failed to validate module name: %w", commonerrors.ErrInvalidOption), + expectedError: fmt.Errorf("opts.ModuleName must match the required pattern, e.g: 'github.com/path-to/your-repo': %w", commonerrors.ErrInvalidOption), }, { name: "invalid module version", @@ -103,7 +103,7 @@ func Test_ValidateModuleConfig(t *testing.T) { "module-icon": "https://example.com/path/to/some-icon", }, }, - expectedError: fmt.Errorf("failed to validate module version: %w", commonerrors.ErrInvalidOption), + expectedError: fmt.Errorf("opts.ModuleVersion failed to parse as semantic version: %w", commonerrors.ErrInvalidOption), }, { name: "invalid module namespace", @@ -118,7 +118,7 @@ func Test_ValidateModuleConfig(t *testing.T) { "module-icon": "https://example.com/path/to/some-icon", }, }, - expectedError: fmt.Errorf("failed to validate module namespace: %w", commonerrors.ErrInvalidOption), + expectedError: fmt.Errorf("namespace must match the required pattern, only small alphanumeric characters and hyphens: %w", commonerrors.ErrInvalidOption), }, { name: "empty manifest path", @@ -133,7 +133,7 @@ func Test_ValidateModuleConfig(t *testing.T) { "module-icon": "https://example.com/path/to/some-icon", }, }, - expectedError: fmt.Errorf("failed to validate manifest: %w: must not be empty", + expectedError: fmt.Errorf("failed to validate manifest: must not be empty: %w", commonerrors.ErrInvalidOption), }, { @@ -149,7 +149,7 @@ func Test_ValidateModuleConfig(t *testing.T) { "module-icon": "https://example.com/path/to/some-icon", }, }, - expectedError: fmt.Errorf("failed to validate manifest: %w: './test' is not using https scheme", + expectedError: fmt.Errorf("failed to validate manifest: './test' is not using https scheme: %w", commonerrors.ErrInvalidOption), }, { @@ -166,7 +166,7 @@ func Test_ValidateModuleConfig(t *testing.T) { }, DefaultCR: "/test", }, - expectedError: fmt.Errorf("failed to validate default CR: %w: '/test' is not using https scheme", + expectedError: fmt.Errorf("failed to validate default CR: '/test' is not using https scheme: %w", commonerrors.ErrInvalidOption), }, { @@ -182,7 +182,7 @@ func Test_ValidateModuleConfig(t *testing.T) { "module-icon": "https://example.com/path/to/some-icon", }, }, - expectedError: fmt.Errorf("failed to validate repository: %w: must not be empty", + expectedError: fmt.Errorf("failed to validate repository: must not be empty: %w", commonerrors.ErrInvalidOption), }, { @@ -198,7 +198,7 @@ func Test_ValidateModuleConfig(t *testing.T) { "module-icon": "https://example.com/path/to/some-icon", }, }, - expectedError: fmt.Errorf("failed to validate repository: %w: 'some repository' is not using https scheme", + expectedError: fmt.Errorf("failed to validate repository: 'some repository' is not using https scheme: %w", commonerrors.ErrInvalidOption), }, { @@ -214,7 +214,7 @@ func Test_ValidateModuleConfig(t *testing.T) { "module-icon": "https://example.com/path/to/some-icon", }, }, - expectedError: fmt.Errorf("failed to validate documentation: %w: must not be empty", + expectedError: fmt.Errorf("failed to validate documentation: must not be empty: %w", commonerrors.ErrInvalidOption), }, { @@ -230,7 +230,7 @@ func Test_ValidateModuleConfig(t *testing.T) { "module-icon": "https://example.com/path/to/some-icon", }, }, - expectedError: fmt.Errorf("failed to validate documentation: %w: 'some documentation' is not using https scheme", + expectedError: fmt.Errorf("failed to validate documentation: 'some documentation' is not using https scheme: %w", commonerrors.ErrInvalidOption), }, { @@ -244,7 +244,7 @@ func Test_ValidateModuleConfig(t *testing.T) { Documentation: "https://example.com/path/to/documentation", Icons: contentprovider.Icons{}, }, - expectedError: fmt.Errorf("failed to validate module icons: %w: must contain at least one icon", + expectedError: fmt.Errorf("failed to validate module icons: must contain at least one icon: %w", commonerrors.ErrInvalidOption), }, { @@ -260,7 +260,7 @@ func Test_ValidateModuleConfig(t *testing.T) { "": "https://example.com/path/to/some-icon", }, }, - expectedError: fmt.Errorf("failed to validate module icons: %w: name must not be empty", + expectedError: fmt.Errorf("failed to validate module icons: name must not be empty: %w", commonerrors.ErrInvalidOption), }, { @@ -276,7 +276,7 @@ func Test_ValidateModuleConfig(t *testing.T) { "module-icon": "", }, }, - expectedError: fmt.Errorf("failed to validate module icons: %w: link must not be empty", + expectedError: fmt.Errorf("failed to validate module icons: link must not be empty: %w", commonerrors.ErrInvalidOption), }, { @@ -292,7 +292,7 @@ func Test_ValidateModuleConfig(t *testing.T) { "module-icon": "this is not a URL", }, }, - expectedError: fmt.Errorf("failed to validate module icons: failed to validate link: %w: 'this is not a URL' is not using https scheme", + expectedError: fmt.Errorf("failed to validate module icons: failed to validate link: 'this is not a URL' is not using https scheme: %w", commonerrors.ErrInvalidOption), }, { @@ -311,7 +311,7 @@ func Test_ValidateModuleConfig(t *testing.T) { "key": "%% not a URL", }, }, - expectedError: fmt.Errorf("failed to validate resources: failed to validate link: %w: '%%%% not a URL' is not a valid URL", + expectedError: fmt.Errorf("failed to validate resources: failed to validate link: '%%%% not a URL' is not a valid URL: %w", commonerrors.ErrInvalidOption), }, { @@ -330,7 +330,7 @@ func Test_ValidateModuleConfig(t *testing.T) { "": "https://github.com/kyma-project/template-operator/releases/download/1.0.1/template-operator.yaml", }, }, - expectedError: fmt.Errorf("failed to validate resources: %w: name must not be empty", + expectedError: fmt.Errorf("failed to validate resources: name must not be empty: %w", commonerrors.ErrInvalidOption), }, { @@ -349,7 +349,7 @@ func Test_ValidateModuleConfig(t *testing.T) { "name": "", }, }, - expectedError: fmt.Errorf("failed to validate resources: %w: link must not be empty", + expectedError: fmt.Errorf("failed to validate resources: link must not be empty: %w", commonerrors.ErrInvalidOption), }, } @@ -357,7 +357,7 @@ func Test_ValidateModuleConfig(t *testing.T) { t.Run(test.name, func(t *testing.T) { err := moduleconfigreader.ValidateModuleConfig(test.moduleConfig) if test.expectedError != nil { - require.ErrorIs(t, err, test.expectedError, test.expectedError.Error()) + require.ErrorContains(t, err, test.expectedError.Error()) return } require.NoError(t, err) diff --git a/tests/e2e/create/create_test.go b/tests/e2e/create/create_test.go index c08ae38e..9da1d706 100644 --- a/tests/e2e/create/create_test.go +++ b/tests/e2e/create/create_test.go @@ -145,7 +145,7 @@ var _ = Describe("Test 'create' command", Ordered, func() { It("Then the command should fail", func() { err := cmd.execute() Expect(err).Should(HaveOccurred()) - Expect(err.Error()).Should(ContainSubstring("failed to parse module config: failed to validate module config: failed to validate module icons: invalid Option: must contain at least one icon")) + Expect(err.Error()).Should(ContainSubstring("failed to parse module config: failed to validate module config: failed to validate module icons: must contain at least one icon: invalid Option")) }) }) From 18cb31861b85f49f26d41c8013cdc8ca15f3fc9b Mon Sep 17 00:00:00 2001 From: medmes Date: Thu, 28 Nov 2024 11:07:34 +0100 Subject: [PATCH 08/18] changes for the PR comment --- .../componentarchive/componentarchive.go | 2 +- .../componentdescriptor/securityconfig.go | 6 +++--- .../service/contentprovider/moduleconfig.go | 4 ++-- internal/service/create/options.go | 2 +- .../service/filegenerator/filegenerator.go | 2 +- .../reusefilegenerator/reuse_filegenerator.go | 12 ++++++------ .../reuse_filegenerator_test.go | 2 +- .../moduleconfig/reader/moduleconfig_reader.go | 2 +- internal/service/scaffold/options.go | 18 +++++++++--------- internal/service/scaffold/scaffold.go | 18 +++++++++--------- .../templategenerator/templategenerator.go | 2 +- tests/e2e/create/create_suite_test.go | 2 +- tests/e2e/scaffold/scaffold_suite_test.go | 2 +- tools/filesystem/archivefilesystem.go | 4 ++-- tools/filesystem/tempfilesystem.go | 13 ++++++++----- 15 files changed, 47 insertions(+), 44 deletions(-) diff --git a/internal/service/componentarchive/componentarchive.go b/internal/service/componentarchive/componentarchive.go index c6cff499..c44a4a27 100644 --- a/internal/service/componentarchive/componentarchive.go +++ b/internal/service/componentarchive/componentarchive.go @@ -32,7 +32,7 @@ type Service struct { func NewService(fileSystem ArchiveFileSystem) (*Service, error) { if fileSystem == nil { - return nil, fmt.Errorf("%w: fileSystem must not be nil", commonerrors.ErrInvalidArg) + return nil, fmt.Errorf("fileSystem must not be nil: %w", commonerrors.ErrInvalidArg) } return &Service{ diff --git a/internal/service/componentdescriptor/securityconfig.go b/internal/service/componentdescriptor/securityconfig.go index fd828686..8e4895d5 100644 --- a/internal/service/componentdescriptor/securityconfig.go +++ b/internal/service/componentdescriptor/securityconfig.go @@ -48,7 +48,7 @@ type SecurityConfigService struct { func NewSecurityConfigService(fileReader FileReader) (*SecurityConfigService, error) { if fileReader == nil { - return nil, fmt.Errorf("%w: fileReader must not be nil", commonerrors.ErrInvalidArg) + return nil, fmt.Errorf("fileReader must not be nil: %w", commonerrors.ErrInvalidArg) } return &SecurityConfigService{ @@ -193,12 +193,12 @@ func appendLabelToAccessor(labeled compdesc.LabelsAccessor, key, value, baseKey func GetImageNameAndTag(imageURL string) (string, string, error) { imageTag := strings.Split(imageURL, ":") if len(imageTag) != imageTagSlicesLength { - return "", "", fmt.Errorf("%w: , image URL: %s", errInvalidURL, imageURL) + return "", "", fmt.Errorf("image URL: %s: %w", imageURL, errInvalidURL) } imageName := strings.Split(imageTag[0], "/") if len(imageName) == 0 { - return "", "", fmt.Errorf("%w: , image URL: %s", errInvalidURL, imageURL) + return "", "", fmt.Errorf("image URL: %s: %w", imageURL, errInvalidURL) } return imageName[len(imageName)-1], imageTag[len(imageTag)-1], nil diff --git a/internal/service/contentprovider/moduleconfig.go b/internal/service/contentprovider/moduleconfig.go index 3c5decfc..cc012179 100644 --- a/internal/service/contentprovider/moduleconfig.go +++ b/internal/service/contentprovider/moduleconfig.go @@ -54,13 +54,13 @@ func (s *ModuleConfigProvider) validateArgs(args types.KeyValueArgs) error { if value, ok := args[ArgModuleName]; !ok { return fmt.Errorf("%s: %w", ArgModuleName, ErrMissingArg) } else if value == "" { - return fmt.Errorf("%s: %w ,must not be empty", ArgModuleName, ErrInvalidArg) + return fmt.Errorf("must not be empty: %s: %w", ArgModuleName, ErrInvalidArg) } if value, ok := args[ArgModuleVersion]; !ok { return fmt.Errorf("%s: %w", ArgModuleVersion, ErrMissingArg) } else if value == "" { - return fmt.Errorf("%s: %w ,must not be empty", ArgModuleVersion, ErrInvalidArg) + return fmt.Errorf("must not be empty: %s: %w", ArgModuleVersion, ErrInvalidArg) } return nil diff --git a/internal/service/create/options.go b/internal/service/create/options.go index 765dba32..a6b2b05f 100644 --- a/internal/service/create/options.go +++ b/internal/service/create/options.go @@ -31,7 +31,7 @@ func (opts Options) Validate() error { if opts.Credentials != "" { matched, err := regexp.MatchString("(.+):(.+)", opts.Credentials) if err != nil { - return fmt.Errorf("%w: opts.Credentials could not be parsed: %w", commonerrors.ErrInvalidOption, err) + return fmt.Errorf("opts.Credentials could not be parsed: %w: %w", commonerrors.ErrInvalidOption, err) } else if !matched { return fmt.Errorf("opts.Credentials is in invalid format: %w", commonerrors.ErrInvalidOption) } diff --git a/internal/service/filegenerator/filegenerator.go b/internal/service/filegenerator/filegenerator.go index e04d5e61..8dc66f8d 100644 --- a/internal/service/filegenerator/filegenerator.go +++ b/internal/service/filegenerator/filegenerator.go @@ -60,7 +60,7 @@ func (s *Service) GenerateFile(out iotools.Out, path string, args types.KeyValue func (s *Service) writeFile(content, path string) error { if err := s.fileWriter.WriteFile(path, content); err != nil { - return fmt.Errorf("%w %s: %w", ErrWritingFile, path, err) + return fmt.Errorf("%w at %s: %w", ErrWritingFile, path, err) } return nil diff --git a/internal/service/filegenerator/reusefilegenerator/reuse_filegenerator.go b/internal/service/filegenerator/reusefilegenerator/reuse_filegenerator.go index f4093300..cad15d17 100644 --- a/internal/service/filegenerator/reusefilegenerator/reuse_filegenerator.go +++ b/internal/service/filegenerator/reusefilegenerator/reuse_filegenerator.go @@ -29,15 +29,15 @@ func NewService( fileGenerator FileGenerator, ) (*Service, error) { if kind == "" { - return nil, fmt.Errorf("%w: kind must not be empty", commonerrors.ErrInvalidArg) + return nil, fmt.Errorf("kind must not be empty: %w", commonerrors.ErrInvalidArg) } if fileSystem == nil { - return nil, fmt.Errorf("%w: fileSystem must not be nil", commonerrors.ErrInvalidArg) + return nil, fmt.Errorf("fileSystem must not be nil: %w", commonerrors.ErrInvalidArg) } if fileGenerator == nil { - return nil, fmt.Errorf("%w: fileGenerator must not be nil", commonerrors.ErrInvalidArg) + return nil, fmt.Errorf("fileGenerator must not be nil: %w", commonerrors.ErrInvalidArg) } return &Service{ @@ -50,17 +50,17 @@ func NewService( func (s *Service) GenerateFile(out iotools.Out, path string, args types.KeyValueArgs) error { fileExists, err := s.fileReader.FileExists(path) if err != nil { - return fmt.Errorf("%w %s: %w", ErrCheckingFileExistence, path, err) + return fmt.Errorf("the path: '%s': %w: %w", path, ErrCheckingFileExistence, err) } if fileExists { - out.Write(fmt.Sprintf("The %s file already exists, reusing: %s\n", s.kind, path)) + out.Write(fmt.Sprintf("the path: '%s' file already exists, reusing: '%s'\n", s.kind, path)) return nil } err = s.fileGenerator.GenerateFile(out, path, args) if err != nil { - return fmt.Errorf("%w: %w", ErrGeneratingFile, err) + return fmt.Errorf("the path: '%s': %w: %w", path, ErrGeneratingFile, err) } return nil diff --git a/internal/service/filegenerator/reusefilegenerator/reuse_filegenerator_test.go b/internal/service/filegenerator/reusefilegenerator/reuse_filegenerator_test.go index a65eddff..da26759c 100644 --- a/internal/service/filegenerator/reusefilegenerator/reuse_filegenerator_test.go +++ b/internal/service/filegenerator/reusefilegenerator/reuse_filegenerator_test.go @@ -51,7 +51,7 @@ func Test_GenerateFile_Succeeds_WhenFileDoesAlreadyExist(t *testing.T) { require.NoError(t, result) require.Len(t, out.sink, 1) - assert.Contains(t, out.sink[0], "The test-kind file already exists, reusing:") + assert.Equal(t, out.sink[0], "the path: 'test-kind' file already exists, reusing: 'some-path'\n") } func Test_GenerateFile_ReturnsError_WhenFileGenerationReturnsError(t *testing.T) { diff --git a/internal/service/moduleconfig/reader/moduleconfig_reader.go b/internal/service/moduleconfig/reader/moduleconfig_reader.go index 8e40f9d5..eea55208 100644 --- a/internal/service/moduleconfig/reader/moduleconfig_reader.go +++ b/internal/service/moduleconfig/reader/moduleconfig_reader.go @@ -21,7 +21,7 @@ type Service struct { func NewService(fileSystem FileSystem) (*Service, error) { if fileSystem == nil { - return nil, fmt.Errorf("%w: fileSystem must not be nil", commonerrors.ErrInvalidArg) + return nil, fmt.Errorf("fileSystem must not be nil: %w", commonerrors.ErrInvalidArg) } return &Service{ diff --git a/internal/service/scaffold/options.go b/internal/service/scaffold/options.go index d2ad9ba0..2e5398f9 100644 --- a/internal/service/scaffold/options.go +++ b/internal/service/scaffold/options.go @@ -24,7 +24,7 @@ type Options struct { func (opts Options) Validate() error { if opts.Out == nil { - return fmt.Errorf("%w: opts.Out must not be nil", commonerrors.ErrInvalidOption) + return fmt.Errorf("opts.Out must not be nil: %w", commonerrors.ErrInvalidOption) } if err := opts.validateModuleName(); err != nil { @@ -40,11 +40,11 @@ func (opts Options) Validate() error { } if opts.ModuleConfigFileName == "" { - return fmt.Errorf("%w: opts.ModuleConfigFileName must not be empty", commonerrors.ErrInvalidOption) + return fmt.Errorf("opts.ModuleConfigFileName must not be empty: %w", commonerrors.ErrInvalidOption) } if opts.ManifestFileName == "" { - return fmt.Errorf("%w: opts.ManifestFileName must not be empty", commonerrors.ErrInvalidOption) + return fmt.Errorf("opts.ManifestFileName must not be empty: %w", commonerrors.ErrInvalidOption) } return nil @@ -52,19 +52,19 @@ func (opts Options) Validate() error { func (opts Options) validateDirectory() error { if opts.Directory == "" { - return fmt.Errorf("%w: opts.Directory must not be empty", commonerrors.ErrInvalidOption) + return fmt.Errorf("opts.Directory must not be empty: %w", commonerrors.ErrInvalidOption) } fileInfo, err := os.Stat(opts.Directory) if err != nil { if errors.Is(err, os.ErrNotExist) { - return fmt.Errorf("%w: directory %s does not exist", commonerrors.ErrInvalidOption, opts.Directory) + return fmt.Errorf("directory %s does not exist: %w: ", opts.Directory, commonerrors.ErrInvalidOption) } - return fmt.Errorf("%w: failed to get directory info %s: %w", commonerrors.ErrInvalidOption, opts.Directory, err) + return fmt.Errorf("failed to get directory info %s: %w: %w", opts.Directory, commonerrors.ErrInvalidOption, err) } if !fileInfo.IsDir() { - return fmt.Errorf("%w: %s is not a directory", commonerrors.ErrInvalidOption, opts.Directory) + return fmt.Errorf("%s is not a directory: %w", opts.Directory, commonerrors.ErrInvalidOption) } return nil @@ -72,7 +72,7 @@ func (opts Options) validateDirectory() error { func (opts Options) validateModuleName() error { if err := validation.ValidateModuleName(opts.ModuleName); err != nil { - return fmt.Errorf("%w: %w", commonerrors.ErrInvalidOption, err) + return fmt.Errorf("module name: %w: %w", commonerrors.ErrInvalidOption, err) } return nil @@ -80,7 +80,7 @@ func (opts Options) validateModuleName() error { func (opts Options) validateVersion() error { if err := validation.ValidateModuleVersion(opts.ModuleVersion); err != nil { - return fmt.Errorf("%w: %w", commonerrors.ErrInvalidOption, err) + return fmt.Errorf("module version: %w: %w", commonerrors.ErrInvalidOption, err) } return nil diff --git a/internal/service/scaffold/scaffold.go b/internal/service/scaffold/scaffold.go index b9fe8821..ba63c158 100644 --- a/internal/service/scaffold/scaffold.go +++ b/internal/service/scaffold/scaffold.go @@ -32,19 +32,19 @@ func NewService(moduleConfigService ModuleConfigService, securityConfigService FileGeneratorService, ) (*Service, error) { if moduleConfigService == nil { - return nil, fmt.Errorf("%w: moduleConfigService must not be nil", commonerrors.ErrInvalidArg) + return nil, fmt.Errorf("moduleConfigService must not be nil: %w", commonerrors.ErrInvalidArg) } if manifestService == nil { - return nil, fmt.Errorf("%w: manifestService must not be nil", commonerrors.ErrInvalidArg) + return nil, fmt.Errorf("manifestService must not be nil: %w", commonerrors.ErrInvalidArg) } if defaultCRService == nil { - return nil, fmt.Errorf("%w: defaultCRService must not be nil", commonerrors.ErrInvalidArg) + return nil, fmt.Errorf("defaultCRService must not be nil: %w", commonerrors.ErrInvalidArg) } if securityConfigService == nil { - return nil, fmt.Errorf("%w: securityConfigService must not be nil", commonerrors.ErrInvalidArg) + return nil, fmt.Errorf("securityConfigService must not be nil: %w", commonerrors.ErrInvalidArg) } return &Service{ @@ -62,19 +62,19 @@ func (s *Service) Run(opts Options) error { if err := s.moduleConfigService.ForceExplicitOverwrite(opts.Directory, opts.ModuleConfigFileName, opts.ModuleConfigFileOverwrite); err != nil { - return fmt.Errorf("%w %s: %w", ErrOverwritingFile, opts.ModuleConfigFileName, err) + return fmt.Errorf("%s %w: %w", opts.ModuleConfigFileName, ErrOverwritingFile, err) } manifestFilePath := path.Join(opts.Directory, opts.ManifestFileName) if err := s.manifestService.GenerateFile(opts.Out, manifestFilePath, nil); err != nil { - return fmt.Errorf("%w %s: %w", ErrGeneratingFile, opts.ManifestFileName, err) + return fmt.Errorf("%s %w: %w", opts.ManifestFileName, ErrGeneratingFile, err) } defaultCRFilePath := "" if opts.defaultCRFileNameConfigured() { defaultCRFilePath = path.Join(opts.Directory, opts.DefaultCRFileName) if err := s.defaultCRService.GenerateFile(opts.Out, defaultCRFilePath, nil); err != nil { - return fmt.Errorf("%w %s: %w", ErrGeneratingFile, opts.DefaultCRFileName, err) + return fmt.Errorf("%s %w: %w", opts.DefaultCRFileName, ErrGeneratingFile, err) } } @@ -85,7 +85,7 @@ func (s *Service) Run(opts Options) error { opts.Out, securityConfigFilePath, types.KeyValueArgs{contentprovider.ArgModuleName: opts.ModuleName}); err != nil { - return fmt.Errorf("%w %s: %w", ErrGeneratingFile, opts.SecurityConfigFileName, err) + return fmt.Errorf("%s %w: %w", opts.SecurityConfigFileName, ErrGeneratingFile, err) } } @@ -100,7 +100,7 @@ func (s *Service) Run(opts Options) error { contentprovider.ArgDefaultCRFile: defaultCRFilePath, contentprovider.ArgSecurityConfigFile: securityConfigFilePath, }); err != nil { - return fmt.Errorf("%w %s: %w", ErrGeneratingFile, opts.ModuleConfigFileName, err) + return fmt.Errorf("%s %w: %w", opts.ModuleConfigFileName, ErrGeneratingFile, err) } return nil diff --git a/internal/service/templategenerator/templategenerator.go b/internal/service/templategenerator/templategenerator.go index 5e28edff..acf4c775 100644 --- a/internal/service/templategenerator/templategenerator.go +++ b/internal/service/templategenerator/templategenerator.go @@ -32,7 +32,7 @@ type Service struct { func NewService(fileSystem FileSystem) (*Service, error) { if fileSystem == nil { - return nil, fmt.Errorf("%w: fileSystem must not be nil", commonerrors.ErrInvalidArg) + return nil, fmt.Errorf("fileSystem must not be nil: %w", commonerrors.ErrInvalidArg) } return &Service{ diff --git a/tests/e2e/create/create_suite_test.go b/tests/e2e/create/create_suite_test.go index f96d29fe..095a428c 100644 --- a/tests/e2e/create/create_suite_test.go +++ b/tests/e2e/create/create_suite_test.go @@ -88,7 +88,7 @@ func (cmd *createCmd) execute() error { println(" >>> Executing command: modulectl", strings.Join(args, " ")) - command = exec.Command("modulectl", args...) + command = exec.Command("/Users/Mo/Projects/kyma-operator-manager/modulectl/bin/modulectl-darwin-arm", args...) cmdOut, err := command.CombinedOutput() if err != nil { return fmt.Errorf("create command failed with output: %s and error: %w", cmdOut, err) diff --git a/tests/e2e/scaffold/scaffold_suite_test.go b/tests/e2e/scaffold/scaffold_suite_test.go index 473f0c8a..536592b6 100644 --- a/tests/e2e/scaffold/scaffold_suite_test.go +++ b/tests/e2e/scaffold/scaffold_suite_test.go @@ -61,7 +61,7 @@ func (cmd *scaffoldCmd) execute() error { args = append(args, "--overwrite=true") } - command = exec.Command("modulectl", args...) + command = exec.Command("/Users/Mo/Projects/kyma-operator-manager/modulectl/bin/modulectl-darwin-arm", args...) cmdOut, err := command.CombinedOutput() if err != nil { return fmt.Errorf("scaffold command failed with output: %s and error: %w", cmdOut, err) diff --git a/tools/filesystem/archivefilesystem.go b/tools/filesystem/archivefilesystem.go index 29f81515..6adfaa5e 100644 --- a/tools/filesystem/archivefilesystem.go +++ b/tools/filesystem/archivefilesystem.go @@ -23,11 +23,11 @@ type ArchiveFileSystem struct { func NewArchiveFileSystem(memoryFileSystem vfs.FileSystem, osFileSystem vfs.FileSystem) (*ArchiveFileSystem, error) { if memoryFileSystem == nil { - return nil, fmt.Errorf("%w: memoryFileSystem must not be nil", commonerrors.ErrInvalidArg) + return nil, fmt.Errorf("memoryFileSystem must not be nil: %w", commonerrors.ErrInvalidArg) } if osFileSystem == nil { - return nil, fmt.Errorf("%w: osFileSystem must not be nil", commonerrors.ErrInvalidArg) + return nil, fmt.Errorf("osFileSystem must not be nil: %w", commonerrors.ErrInvalidArg) } return &ArchiveFileSystem{ diff --git a/tools/filesystem/tempfilesystem.go b/tools/filesystem/tempfilesystem.go index d5062842..e483bd58 100644 --- a/tools/filesystem/tempfilesystem.go +++ b/tools/filesystem/tempfilesystem.go @@ -58,18 +58,21 @@ func getBytesFromURL(url *url.URL) ([]byte, error) { defer cancel() req, err := http.NewRequestWithContext(ctx, http.MethodGet, url.String(), nil) if err != nil { - return nil, fmt.Errorf("http GET request failed for %s: %w", url, err) + return nil, fmt.Errorf("failed to create HTTP GET request for %s: %w", url, err) } resp, err := http.DefaultClient.Do(req) if err != nil { - return nil, fmt.Errorf("http GET request failed for %s: %w", url, err) + return nil, fmt.Errorf("HTTP GET request failed for %s: %w", url, err) } - defer resp.Body.Close() + defer func() { + if closeErr := resp.Body.Close(); closeErr != nil { + err = fmt.Errorf("failed to close response body: %w", closeErr) + } + }() if resp.StatusCode != http.StatusOK { - return nil, fmt.Errorf("%w: bad status for GET request to %s: %q", errBadHTTPStatus, url, - resp.StatusCode) + return nil, fmt.Errorf("received status code %d for GET request to %s: %w", resp.StatusCode, url, errBadHTTPStatus) } data, err := io.ReadAll(resp.Body) From 7bda02500bb3ee304bada6bb1e3d684495d04937 Mon Sep 17 00:00:00 2001 From: medmes Date: Thu, 28 Nov 2024 11:14:50 +0100 Subject: [PATCH 09/18] changes for the PR comment --- internal/common/validation/validation.go | 2 +- internal/service/contentprovider/securityconfig.go | 2 +- .../service/moduleconfig/reader/moduleconfig_reader_test.go | 2 +- internal/service/scaffold/options_test.go | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/common/validation/validation.go b/internal/common/validation/validation.go index 7a49377e..5541ce10 100644 --- a/internal/common/validation/validation.go +++ b/internal/common/validation/validation.go @@ -117,7 +117,7 @@ func ValidateIsValidHTTPSURL(input string) error { func validateSemanticVersion(version string) error { _, err := semver.StrictNewVersion(strings.TrimSpace(version)) if err != nil { - return fmt.Errorf("opts.ModuleVersion failed to parse as semantic version: %w", + return fmt.Errorf("opts.ModuleVersion failed to be parsed as semantic version: %w", commonerrors.ErrInvalidOption) } diff --git a/internal/service/contentprovider/securityconfig.go b/internal/service/contentprovider/securityconfig.go index ad327da4..b79d878e 100644 --- a/internal/service/contentprovider/securityconfig.go +++ b/internal/service/contentprovider/securityconfig.go @@ -40,7 +40,7 @@ func (s *SecurityConfig) validateArgs(args types.KeyValueArgs) error { } if value == "" { - return fmt.Errorf("%s: %w must not be empty", ArgModuleName, ErrInvalidArg) + return fmt.Errorf("%s must not be empty: %w", ArgModuleName, ErrInvalidArg) } return nil diff --git a/internal/service/moduleconfig/reader/moduleconfig_reader_test.go b/internal/service/moduleconfig/reader/moduleconfig_reader_test.go index 2f2877a1..fad614e9 100644 --- a/internal/service/moduleconfig/reader/moduleconfig_reader_test.go +++ b/internal/service/moduleconfig/reader/moduleconfig_reader_test.go @@ -103,7 +103,7 @@ func Test_ValidateModuleConfig(t *testing.T) { "module-icon": "https://example.com/path/to/some-icon", }, }, - expectedError: fmt.Errorf("opts.ModuleVersion failed to parse as semantic version: %w", commonerrors.ErrInvalidOption), + expectedError: fmt.Errorf("opts.ModuleVersion failed to be parsed as semantic version: %w", commonerrors.ErrInvalidOption), }, { name: "invalid module namespace", diff --git a/internal/service/scaffold/options_test.go b/internal/service/scaffold/options_test.go index c69e1e21..c7d06ccf 100644 --- a/internal/service/scaffold/options_test.go +++ b/internal/service/scaffold/options_test.go @@ -81,7 +81,7 @@ func Test_Validate_Options(t *testing.T) { ModuleVersion: "invalid", }, wantErr: true, - errMsg: "opts.ModuleVersion failed to parse as semantic version", + errMsg: "opts.ModuleVersion failed to be parsed as semantic version", }, { name: "ModuleConfigFileName is empty", From fc5a3d264a0672ad844e0e84204a093048a74c7c Mon Sep 17 00:00:00 2001 From: medmes Date: Thu, 28 Nov 2024 11:18:02 +0100 Subject: [PATCH 10/18] patch the path error message --- internal/service/filegenerator/filegenerator.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/service/filegenerator/filegenerator.go b/internal/service/filegenerator/filegenerator.go index 8dc66f8d..8e96824c 100644 --- a/internal/service/filegenerator/filegenerator.go +++ b/internal/service/filegenerator/filegenerator.go @@ -60,7 +60,7 @@ func (s *Service) GenerateFile(out iotools.Out, path string, args types.KeyValue func (s *Service) writeFile(content, path string) error { if err := s.fileWriter.WriteFile(path, content); err != nil { - return fmt.Errorf("%w at %s: %w", ErrWritingFile, path, err) + return fmt.Errorf("%w at '%s': %w", ErrWritingFile, path, err) } return nil From a706186a9c88ebd86ff632984a846756635c27b3 Mon Sep 17 00:00:00 2001 From: medmes Date: Thu, 28 Nov 2024 13:40:17 +0100 Subject: [PATCH 11/18] patch the path error message --- .../reusefilegenerator/reuse_filegenerator_test.go | 2 +- tests/e2e/create/create_test.go | 14 +++++++++++++- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/internal/service/filegenerator/reusefilegenerator/reuse_filegenerator_test.go b/internal/service/filegenerator/reusefilegenerator/reuse_filegenerator_test.go index da26759c..968231f0 100644 --- a/internal/service/filegenerator/reusefilegenerator/reuse_filegenerator_test.go +++ b/internal/service/filegenerator/reusefilegenerator/reuse_filegenerator_test.go @@ -51,7 +51,7 @@ func Test_GenerateFile_Succeeds_WhenFileDoesAlreadyExist(t *testing.T) { require.NoError(t, result) require.Len(t, out.sink, 1) - assert.Equal(t, out.sink[0], "the path: 'test-kind' file already exists, reusing: 'some-path'\n") + assert.Equal(t, "the path: 'test-kind' file already exists, reusing: 'some-path'\n", out.sink[0]) } func Test_GenerateFile_ReturnsError_WhenFileGenerationReturnsError(t *testing.T) { diff --git a/tests/e2e/create/create_test.go b/tests/e2e/create/create_test.go index fb86a756..2341c074 100644 --- a/tests/e2e/create/create_test.go +++ b/tests/e2e/create/create_test.go @@ -3,6 +3,8 @@ package create_test import ( + "errors" + "fmt" "io/fs" "os" @@ -23,6 +25,14 @@ import ( . "github.com/onsi/gomega" ) +func printErrorChain(err error) { + fmt.Println("Error chain:") + for err != nil { + fmt.Printf(" - %v\n", err) + err = errors.Unwrap(err) + } +} + var _ = Describe("Test 'create' command", Ordered, func() { Context("Given 'modulectl create' command", func() { var cmd createCmd @@ -776,6 +786,7 @@ var _ = Describe("Test 'create' command", Ordered, func() { }) It("Then the command should fail", func() { err := cmd.execute() + printErrorChain(err) Expect(err).Should(HaveOccurred()) Expect(err.Error()).Should(ContainSubstring("failed to parse module config: failed to validate module config: failed to validate manifest: './template-operator.yaml' is not using https scheme: invalid Option")) }) @@ -790,8 +801,9 @@ var _ = Describe("Test 'create' command", Ordered, func() { }) It("Then the command should fail", func() { err := cmd.execute() + printErrorChain(err) Expect(err).Should(HaveOccurred()) - Expect(err.Error()).Should(ContainSubstring("failed to parse module config: failed to validate module config: failed to validate default CR: '/tmp/default-sample-cr.yaml' is not using https scheme: invalid Option")) + Expect(err.Error()).Should(ContainSubstring("failed to be parsed module config: failed to validate module config: failed to validate default CR: '/tmp/default-sample-cr.yaml' is not using https scheme: invalid Option")) }) }) }) From b738211ce02f4c6359da5304b9456ef324f4eb07 Mon Sep 17 00:00:00 2001 From: medmes Date: Thu, 28 Nov 2024 14:41:45 +0100 Subject: [PATCH 12/18] e2e cleanup err msg --- tests/e2e/create/create_suite_test.go | 2 +- tests/e2e/create/create_test.go | 14 +------------- tests/e2e/scaffold/scaffold_suite_test.go | 2 +- 3 files changed, 3 insertions(+), 15 deletions(-) diff --git a/tests/e2e/create/create_suite_test.go b/tests/e2e/create/create_suite_test.go index 095a428c..f96d29fe 100644 --- a/tests/e2e/create/create_suite_test.go +++ b/tests/e2e/create/create_suite_test.go @@ -88,7 +88,7 @@ func (cmd *createCmd) execute() error { println(" >>> Executing command: modulectl", strings.Join(args, " ")) - command = exec.Command("/Users/Mo/Projects/kyma-operator-manager/modulectl/bin/modulectl-darwin-arm", args...) + command = exec.Command("modulectl", args...) cmdOut, err := command.CombinedOutput() if err != nil { return fmt.Errorf("create command failed with output: %s and error: %w", cmdOut, err) diff --git a/tests/e2e/create/create_test.go b/tests/e2e/create/create_test.go index 2341c074..fb86a756 100644 --- a/tests/e2e/create/create_test.go +++ b/tests/e2e/create/create_test.go @@ -3,8 +3,6 @@ package create_test import ( - "errors" - "fmt" "io/fs" "os" @@ -25,14 +23,6 @@ import ( . "github.com/onsi/gomega" ) -func printErrorChain(err error) { - fmt.Println("Error chain:") - for err != nil { - fmt.Printf(" - %v\n", err) - err = errors.Unwrap(err) - } -} - var _ = Describe("Test 'create' command", Ordered, func() { Context("Given 'modulectl create' command", func() { var cmd createCmd @@ -786,7 +776,6 @@ var _ = Describe("Test 'create' command", Ordered, func() { }) It("Then the command should fail", func() { err := cmd.execute() - printErrorChain(err) Expect(err).Should(HaveOccurred()) Expect(err.Error()).Should(ContainSubstring("failed to parse module config: failed to validate module config: failed to validate manifest: './template-operator.yaml' is not using https scheme: invalid Option")) }) @@ -801,9 +790,8 @@ var _ = Describe("Test 'create' command", Ordered, func() { }) It("Then the command should fail", func() { err := cmd.execute() - printErrorChain(err) Expect(err).Should(HaveOccurred()) - Expect(err.Error()).Should(ContainSubstring("failed to be parsed module config: failed to validate module config: failed to validate default CR: '/tmp/default-sample-cr.yaml' is not using https scheme: invalid Option")) + Expect(err.Error()).Should(ContainSubstring("failed to parse module config: failed to validate module config: failed to validate default CR: '/tmp/default-sample-cr.yaml' is not using https scheme: invalid Option")) }) }) }) diff --git a/tests/e2e/scaffold/scaffold_suite_test.go b/tests/e2e/scaffold/scaffold_suite_test.go index 536592b6..473f0c8a 100644 --- a/tests/e2e/scaffold/scaffold_suite_test.go +++ b/tests/e2e/scaffold/scaffold_suite_test.go @@ -61,7 +61,7 @@ func (cmd *scaffoldCmd) execute() error { args = append(args, "--overwrite=true") } - command = exec.Command("/Users/Mo/Projects/kyma-operator-manager/modulectl/bin/modulectl-darwin-arm", args...) + command = exec.Command("modulectl", args...) cmdOut, err := command.CombinedOutput() if err != nil { return fmt.Errorf("scaffold command failed with output: %s and error: %w", cmdOut, err) From 9e2cc08d4a239995171ff312ec307e0af3b8fde5 Mon Sep 17 00:00:00 2001 From: medmes Date: Thu, 28 Nov 2024 15:46:11 +0100 Subject: [PATCH 13/18] reformat the error message --- .../filegenerator/reusefilegenerator/reuse_filegenerator.go | 6 +++--- .../reusefilegenerator/reuse_filegenerator_test.go | 2 +- internal/service/scaffold/options.go | 6 +++--- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/internal/service/filegenerator/reusefilegenerator/reuse_filegenerator.go b/internal/service/filegenerator/reusefilegenerator/reuse_filegenerator.go index cad15d17..b4815ebd 100644 --- a/internal/service/filegenerator/reusefilegenerator/reuse_filegenerator.go +++ b/internal/service/filegenerator/reusefilegenerator/reuse_filegenerator.go @@ -50,17 +50,17 @@ func NewService( func (s *Service) GenerateFile(out iotools.Out, path string, args types.KeyValueArgs) error { fileExists, err := s.fileReader.FileExists(path) if err != nil { - return fmt.Errorf("the path: '%s': %w: %w", path, ErrCheckingFileExistence, err) + return fmt.Errorf("the '%s' file path: %w: %w", path, ErrCheckingFileExistence, err) } if fileExists { - out.Write(fmt.Sprintf("the path: '%s' file already exists, reusing: '%s'\n", s.kind, path)) + out.Write(fmt.Sprintf("the '%s' file path already exists, reusing: '%s'\n", s.kind, path)) return nil } err = s.fileGenerator.GenerateFile(out, path, args) if err != nil { - return fmt.Errorf("the path: '%s': %w: %w", path, ErrGeneratingFile, err) + return fmt.Errorf("the '%s' file path: %w: %w", path, ErrGeneratingFile, err) } return nil diff --git a/internal/service/filegenerator/reusefilegenerator/reuse_filegenerator_test.go b/internal/service/filegenerator/reusefilegenerator/reuse_filegenerator_test.go index 968231f0..9157a319 100644 --- a/internal/service/filegenerator/reusefilegenerator/reuse_filegenerator_test.go +++ b/internal/service/filegenerator/reusefilegenerator/reuse_filegenerator_test.go @@ -51,7 +51,7 @@ func Test_GenerateFile_Succeeds_WhenFileDoesAlreadyExist(t *testing.T) { require.NoError(t, result) require.Len(t, out.sink, 1) - assert.Equal(t, "the path: 'test-kind' file already exists, reusing: 'some-path'\n", out.sink[0]) + assert.Equal(t, "the 'test-kind' file path already exists, reusing: 'some-path'\n", out.sink[0]) } func Test_GenerateFile_ReturnsError_WhenFileGenerationReturnsError(t *testing.T) { diff --git a/internal/service/scaffold/options.go b/internal/service/scaffold/options.go index 2e5398f9..bc9e26fa 100644 --- a/internal/service/scaffold/options.go +++ b/internal/service/scaffold/options.go @@ -58,7 +58,7 @@ func (opts Options) validateDirectory() error { fileInfo, err := os.Stat(opts.Directory) if err != nil { if errors.Is(err, os.ErrNotExist) { - return fmt.Errorf("directory %s does not exist: %w: ", opts.Directory, commonerrors.ErrInvalidOption) + return fmt.Errorf("directory %s does not exist: %w:", opts.Directory, commonerrors.ErrInvalidOption) } return fmt.Errorf("failed to get directory info %s: %w: %w", opts.Directory, commonerrors.ErrInvalidOption, err) } @@ -72,7 +72,7 @@ func (opts Options) validateDirectory() error { func (opts Options) validateModuleName() error { if err := validation.ValidateModuleName(opts.ModuleName); err != nil { - return fmt.Errorf("module name: %w: %w", commonerrors.ErrInvalidOption, err) + return fmt.Errorf("opts.ModuleName: %w: %w", commonerrors.ErrInvalidOption, err) } return nil @@ -80,7 +80,7 @@ func (opts Options) validateModuleName() error { func (opts Options) validateVersion() error { if err := validation.ValidateModuleVersion(opts.ModuleVersion); err != nil { - return fmt.Errorf("module version: %w: %w", commonerrors.ErrInvalidOption, err) + return fmt.Errorf("opts.ModuleVersion: %w", err) } return nil From ab6a91a734e1e49dec19c40aa121db667a0eb8fb Mon Sep 17 00:00:00 2001 From: medmes Date: Thu, 28 Nov 2024 16:03:10 +0100 Subject: [PATCH 14/18] reformat the error message --- internal/service/contentprovider/moduleconfig.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/service/contentprovider/moduleconfig.go b/internal/service/contentprovider/moduleconfig.go index cc012179..d5de12b1 100644 --- a/internal/service/contentprovider/moduleconfig.go +++ b/internal/service/contentprovider/moduleconfig.go @@ -54,13 +54,13 @@ func (s *ModuleConfigProvider) validateArgs(args types.KeyValueArgs) error { if value, ok := args[ArgModuleName]; !ok { return fmt.Errorf("%s: %w", ArgModuleName, ErrMissingArg) } else if value == "" { - return fmt.Errorf("must not be empty: %s: %w", ArgModuleName, ErrInvalidArg) + return fmt.Errorf("%s must not be empty: %w", ArgModuleName, ErrInvalidArg) } if value, ok := args[ArgModuleVersion]; !ok { return fmt.Errorf("%s: %w", ArgModuleVersion, ErrMissingArg) } else if value == "" { - return fmt.Errorf("must not be empty: %s: %w", ArgModuleVersion, ErrInvalidArg) + return fmt.Errorf("%s must not be empty: %w", ArgModuleVersion, ErrInvalidArg) } return nil From 86c11d654922480386b15ac16c610f5f76fec741 Mon Sep 17 00:00:00 2001 From: medmes Date: Thu, 28 Nov 2024 16:07:38 +0100 Subject: [PATCH 15/18] reformat the error message --- internal/service/scaffold/options.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/service/scaffold/options.go b/internal/service/scaffold/options.go index bc9e26fa..5ead33f6 100644 --- a/internal/service/scaffold/options.go +++ b/internal/service/scaffold/options.go @@ -58,7 +58,7 @@ func (opts Options) validateDirectory() error { fileInfo, err := os.Stat(opts.Directory) if err != nil { if errors.Is(err, os.ErrNotExist) { - return fmt.Errorf("directory %s does not exist: %w:", opts.Directory, commonerrors.ErrInvalidOption) + return fmt.Errorf("directory %s does not exist: %w", opts.Directory, commonerrors.ErrInvalidOption) } return fmt.Errorf("failed to get directory info %s: %w: %w", opts.Directory, commonerrors.ErrInvalidOption, err) } From 2cac7e9ab982fb18fbc0b17c2cf79b807d661a84 Mon Sep 17 00:00:00 2001 From: medmes Date: Thu, 28 Nov 2024 16:24:51 +0100 Subject: [PATCH 16/18] reformat the error message --- internal/service/filegenerator/filegenerator.go | 2 +- internal/service/scaffold/options.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/service/filegenerator/filegenerator.go b/internal/service/filegenerator/filegenerator.go index 8e96824c..9fbdb028 100644 --- a/internal/service/filegenerator/filegenerator.go +++ b/internal/service/filegenerator/filegenerator.go @@ -60,7 +60,7 @@ func (s *Service) GenerateFile(out iotools.Out, path string, args types.KeyValue func (s *Service) writeFile(content, path string) error { if err := s.fileWriter.WriteFile(path, content); err != nil { - return fmt.Errorf("%w at '%s': %w", ErrWritingFile, path, err) + return fmt.Errorf("file path: %s, %w: %w", path, err, ErrWritingFile) } return nil diff --git a/internal/service/scaffold/options.go b/internal/service/scaffold/options.go index 5ead33f6..405e0854 100644 --- a/internal/service/scaffold/options.go +++ b/internal/service/scaffold/options.go @@ -72,7 +72,7 @@ func (opts Options) validateDirectory() error { func (opts Options) validateModuleName() error { if err := validation.ValidateModuleName(opts.ModuleName); err != nil { - return fmt.Errorf("opts.ModuleName: %w: %w", commonerrors.ErrInvalidOption, err) + return fmt.Errorf("opts.ModuleName: %w", err) } return nil From 89c19c42b36e438ce26bfca5b5f073eb08aa7a9d Mon Sep 17 00:00:00 2001 From: medmes Date: Fri, 29 Nov 2024 11:36:01 +0100 Subject: [PATCH 17/18] remove errors constants for scaffold service. --- internal/service/scaffold/errors.go | 8 -------- internal/service/scaffold/scaffold.go | 16 ++++++++-------- internal/service/scaffold/scaffold_test.go | 4 ---- 3 files changed, 8 insertions(+), 20 deletions(-) delete mode 100644 internal/service/scaffold/errors.go diff --git a/internal/service/scaffold/errors.go b/internal/service/scaffold/errors.go deleted file mode 100644 index e4042dbb..00000000 --- a/internal/service/scaffold/errors.go +++ /dev/null @@ -1,8 +0,0 @@ -package scaffold - -import "errors" - -var ( - ErrGeneratingFile = errors.New("error generating file") - ErrOverwritingFile = errors.New("error overwriting file") -) diff --git a/internal/service/scaffold/scaffold.go b/internal/service/scaffold/scaffold.go index ba63c158..2b821517 100644 --- a/internal/service/scaffold/scaffold.go +++ b/internal/service/scaffold/scaffold.go @@ -57,35 +57,35 @@ func NewService(moduleConfigService ModuleConfigService, func (s *Service) Run(opts Options) error { if err := opts.Validate(); err != nil { - return err + return fmt.Errorf("validation failed for options: %w", err) } if err := s.moduleConfigService.ForceExplicitOverwrite(opts.Directory, opts.ModuleConfigFileName, opts.ModuleConfigFileOverwrite); err != nil { - return fmt.Errorf("%s %w: %w", opts.ModuleConfigFileName, ErrOverwritingFile, err) + return fmt.Errorf("failed to force explicit overwrite for file %q in directory %q: %w", opts.ModuleConfigFileName, opts.Directory, err) } manifestFilePath := path.Join(opts.Directory, opts.ManifestFileName) if err := s.manifestService.GenerateFile(opts.Out, manifestFilePath, nil); err != nil { - return fmt.Errorf("%s %w: %w", opts.ManifestFileName, ErrGeneratingFile, err) + return fmt.Errorf("failed to generate manifest file %q at %q: %w", opts.ManifestFileName, manifestFilePath, err) } - defaultCRFilePath := "" + var defaultCRFilePath string if opts.defaultCRFileNameConfigured() { defaultCRFilePath = path.Join(opts.Directory, opts.DefaultCRFileName) if err := s.defaultCRService.GenerateFile(opts.Out, defaultCRFilePath, nil); err != nil { - return fmt.Errorf("%s %w: %w", opts.DefaultCRFileName, ErrGeneratingFile, err) + return fmt.Errorf("failed to generate default CR file %q at %q: %w", opts.DefaultCRFileName, defaultCRFilePath, err) } } - securityConfigFilePath := "" + var securityConfigFilePath string if opts.securityConfigFileNameConfigured() { securityConfigFilePath = path.Join(opts.Directory, opts.SecurityConfigFileName) if err := s.securityConfigService.GenerateFile( opts.Out, securityConfigFilePath, types.KeyValueArgs{contentprovider.ArgModuleName: opts.ModuleName}); err != nil { - return fmt.Errorf("%s %w: %w", opts.SecurityConfigFileName, ErrGeneratingFile, err) + return fmt.Errorf("failed to generate security config file %q at %q for module %q: %w", opts.SecurityConfigFileName, securityConfigFilePath, opts.ModuleName, err) } } @@ -100,7 +100,7 @@ func (s *Service) Run(opts Options) error { contentprovider.ArgDefaultCRFile: defaultCRFilePath, contentprovider.ArgSecurityConfigFile: securityConfigFilePath, }); err != nil { - return fmt.Errorf("%s %w: %w", opts.ModuleConfigFileName, ErrGeneratingFile, err) + return fmt.Errorf("failed to generate module config file %q at %q: %w", opts.ModuleConfigFileName, moduleConfigFilePath, err) } return nil diff --git a/internal/service/scaffold/scaffold_test.go b/internal/service/scaffold/scaffold_test.go index 37c10a65..33508bdb 100644 --- a/internal/service/scaffold/scaffold_test.go +++ b/internal/service/scaffold/scaffold_test.go @@ -79,7 +79,6 @@ func Test_CreateScaffold_ReturnsError_WhenGeneratingManifestFileFails(t *testing result := svc.Run(newScaffoldOptionsBuilder().build()) - require.ErrorIs(t, result, scaffold.ErrGeneratingFile) require.ErrorIs(t, result, errSomeFileGeneratorError) assert.Contains(t, result.Error(), "manifest.yaml") } @@ -117,7 +116,6 @@ func Test_CreateScaffold_ReturnsError_WhenGeneratingDefaultCRFileFails(t *testin result := svc.Run(newScaffoldOptionsBuilder().build()) - require.ErrorIs(t, result, scaffold.ErrGeneratingFile) require.ErrorIs(t, result, errSomeFileGeneratorError) assert.Contains(t, result.Error(), "default-cr.yaml") } @@ -143,7 +141,6 @@ func Test_CreateScaffold_ReturnsError_WhenGeneratingSecurityConfigFileFails(t *t result := svc.Run(newScaffoldOptionsBuilder().build()) - require.ErrorIs(t, result, scaffold.ErrGeneratingFile) require.ErrorIs(t, result, errSomeFileGeneratorError) assert.Contains(t, result.Error(), "security-config.yaml") } @@ -169,7 +166,6 @@ func Test_CreateScaffold_ReturnsError_WhenGeneratingModuleConfigReturnsError(t * result := svc.Run(newScaffoldOptionsBuilder().build()) - require.ErrorIs(t, result, scaffold.ErrGeneratingFile) require.ErrorIs(t, result, errSomeFileGeneratorError) assert.Contains(t, result.Error(), "module-config.yaml") } From e6f7a74fe767b6465cdde3254282b2b0866b8a39 Mon Sep 17 00:00:00 2001 From: medmes Date: Fri, 29 Nov 2024 12:11:48 +0100 Subject: [PATCH 18/18] remove module name --- internal/service/scaffold/scaffold.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/service/scaffold/scaffold.go b/internal/service/scaffold/scaffold.go index 2b821517..c77b24fd 100644 --- a/internal/service/scaffold/scaffold.go +++ b/internal/service/scaffold/scaffold.go @@ -85,7 +85,7 @@ func (s *Service) Run(opts Options) error { opts.Out, securityConfigFilePath, types.KeyValueArgs{contentprovider.ArgModuleName: opts.ModuleName}); err != nil { - return fmt.Errorf("failed to generate security config file %q at %q for module %q: %w", opts.SecurityConfigFileName, securityConfigFilePath, opts.ModuleName, err) + return fmt.Errorf("failed to generate security config file %q at %q: %w", opts.SecurityConfigFileName, securityConfigFilePath, err) } }