From 30d4ab020d1a869e4047772f3c3b9b8066d52603 Mon Sep 17 00:00:00 2001 From: Stephanie Date: Thu, 22 Apr 2021 16:29:14 -0400 Subject: [PATCH 1/6] checks for import reference attributes in validtor Signed-off-by: Stephanie --- pkg/validation/commands.go | 7 +++-- pkg/validation/commands_test.go | 27 +++++++++++++++++++ pkg/validation/components.go | 21 +++++++++------ pkg/validation/components_test.go | 22 +++++++++++++++ pkg/validation/errors.go | 40 ++++++++++++++++++++++++++- pkg/validation/projects.go | 26 +++++++++++++----- pkg/validation/projects_test.go | 45 +++++++++++++++++++++++++++++++ pkg/validation/utils.go | 6 +++++ 8 files changed, 174 insertions(+), 20 deletions(-) diff --git a/pkg/validation/commands.go b/pkg/validation/commands.go index 8447737c8..eaea7e125 100644 --- a/pkg/validation/commands.go +++ b/pkg/validation/commands.go @@ -25,7 +25,7 @@ func ValidateCommands(commands []v1alpha2.Command, components []v1alpha2.Compone parentCommands := make(map[string]string) err = validateCommand(command, parentCommands, commandMap, components) if err != nil { - return err + return resolveErrorMessageWithImportArrtibutes(err, command.Attributes) } commandGroup := getGroup(command) @@ -50,7 +50,7 @@ func ValidateCommands(commands []v1alpha2.Command, components []v1alpha2.Compone // validateCommand validates a given devfile command where parentCommands is a map to track all the parent commands when validating // the composite command's subcommands recursively and devfileCommands is a map of command id to the devfile command -func validateCommand(command v1alpha2.Command, parentCommands map[string]string, devfileCommands map[string]v1alpha2.Command, components []v1alpha2.Component) (err error) { +func validateCommand(command v1alpha2.Command, parentCommands map[string]string, devfileCommands map[string]v1alpha2.Command, components []v1alpha2.Component) error { switch { case command.Composite != nil: @@ -58,10 +58,9 @@ func validateCommand(command v1alpha2.Command, parentCommands map[string]string, case command.Exec != nil || command.Apply != nil: return validateCommandComponent(command, components) default: - err = fmt.Errorf("command %s type is invalid", command.Id) + return &InvalidCommandTypeError{commandId: command.Id} } - return err } // validateGroup validates commands belonging to a specific group kind. If there are multiple commands belonging to the same group: diff --git a/pkg/validation/commands_test.go b/pkg/validation/commands_test.go index e901608c4..d64297846 100644 --- a/pkg/validation/commands_test.go +++ b/pkg/validation/commands_test.go @@ -1,6 +1,7 @@ package validation import ( + "github.com/devfile/api/v2/pkg/attributes" "testing" "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" @@ -76,6 +77,12 @@ func TestValidateCommands(t *testing.T) { multipleDefaultCmdErr := ".*there should be exactly one default command, currently there is more than one default command" invalidCmdErr := ".*command does not map to a container component" + + parentOverridesFromMainDevfile := attributes.Attributes{}.PutString(ImportSourceAttribute, + "uri: http://127.0.0.1:8080").PutString(ParentOverrideAttribute, "main devfile") + invalidCmdErrWithImportAttributes := ".*command does not map to a container component, imported from uri: http://127.0.0.1:8080, in parent overrides from main devfile" + + tests := []struct { name string commands []v1alpha2.Command @@ -144,6 +151,26 @@ func TestValidateCommands(t *testing.T) { generateDummyCompositeCommand("composite-a", []string{"basic-exec"}, nil), }, }, + { + name: "Invalid command with import source attribute", + commands: []v1alpha2.Command{ + { + Attributes: parentOverridesFromMainDevfile, + Id: "command", + CommandUnion: v1alpha2.CommandUnion{ + Apply: &v1alpha2.ApplyCommand{ + LabeledCommand: v1alpha2.LabeledCommand{ + BaseCommand: v1alpha2.BaseCommand{ + Group: nil, + }, + }, + Component: "invalidComponent", + }, + }, + }, + }, + wantErr: &invalidCmdErrWithImportAttributes, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/pkg/validation/components.go b/pkg/validation/components.go index 73d290778..c07f3e821 100644 --- a/pkg/validation/components.go +++ b/pkg/validation/components.go @@ -26,6 +26,7 @@ func ValidateComponents(components []v1alpha2.Component) error { processedVolumeMounts := make(map[string][]string) processedEndPointName := make(map[string]bool) processedEndPointPort := make(map[int]bool) + processedComponentWithVolumeMounts := make(map[string]v1alpha2.Component) err := v1alpha2.CheckDuplicateKeys(components) if err != nil { @@ -38,6 +39,7 @@ func ValidateComponents(components []v1alpha2.Component) error { // Process all the volume mounts in container components to validate them later for _, volumeMount := range component.Container.VolumeMounts { processedVolumeMounts[component.Name] = append(processedVolumeMounts[component.Name], volumeMount.Name) + processedComponentWithVolumeMounts[component.Name] = component } @@ -52,7 +54,7 @@ func ValidateComponents(components []v1alpha2.Component) error { err := validateEndpoints(component.Container.Endpoints, processedEndPointPort, processedEndPointName) if err != nil { - return err + return resolveErrorMessageWithImportArrtibutes(err, component.Attributes) } case component.Volume != nil: processedVolumes[component.Name] = true @@ -61,37 +63,38 @@ func ValidateComponents(components []v1alpha2.Component) error { // express storage in Kubernetes. For reference, you may check doc // https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/ if _, err := resource.ParseQuantity(component.Volume.Size); err != nil { - return &InvalidVolumeError{name: component.Name, reason: fmt.Sprintf("size %s for volume component is invalid, %v. Example - 2Gi, 1024Mi", component.Volume.Size, err)} + invalidVolErr := &InvalidVolumeError{name: component.Name, reason: fmt.Sprintf("size %s for volume component is invalid, %v. Example - 2Gi, 1024Mi", component.Volume.Size, err)} + return resolveErrorMessageWithImportArrtibutes(invalidVolErr, component.Attributes) } } case component.Openshift != nil: if component.Openshift.Uri != "" { err := ValidateURI(component.Openshift.Uri) if err != nil { - return err + return resolveErrorMessageWithImportArrtibutes(err, component.Attributes) } } err := validateEndpoints(component.Openshift.Endpoints, processedEndPointPort, processedEndPointName) if err != nil { - return err + return resolveErrorMessageWithImportArrtibutes(err, component.Attributes) } case component.Kubernetes != nil: if component.Kubernetes.Uri != "" { err := ValidateURI(component.Kubernetes.Uri) if err != nil { - return err + return resolveErrorMessageWithImportArrtibutes(err, component.Attributes) } } err := validateEndpoints(component.Kubernetes.Endpoints, processedEndPointPort, processedEndPointName) if err != nil { - return err + return resolveErrorMessageWithImportArrtibutes(err, component.Attributes) } case component.Plugin != nil: if component.Plugin.RegistryUrl != "" { err := ValidateURI(component.Plugin.RegistryUrl) if err != nil { - return err + return resolveErrorMessageWithImportArrtibutes(err, component.Attributes) } } } @@ -103,7 +106,9 @@ func ValidateComponents(components []v1alpha2.Component) error { for componentName, volumeMountNames := range processedVolumeMounts { for _, volumeMountName := range volumeMountNames { if !processedVolumes[volumeMountName] { - invalidVolumeMountsErr += fmt.Sprintf("\nvolume mount %s belonging to the container component %s", volumeMountName, componentName) + missingVolumeMountErr := fmt.Errorf("\nvolume mount %s belonging to the container component %s", volumeMountName, componentName) + newErr := resolveErrorMessageWithImportArrtibutes(missingVolumeMountErr, processedComponentWithVolumeMounts[componentName].Attributes) + invalidVolumeMountsErr += newErr.Error() } } } diff --git a/pkg/validation/components_test.go b/pkg/validation/components_test.go index 77f0a302e..63dc2efda 100644 --- a/pkg/validation/components_test.go +++ b/pkg/validation/components_test.go @@ -1,6 +1,7 @@ package validation import ( + "github.com/devfile/api/v2/pkg/attributes" "testing" "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" @@ -137,6 +138,10 @@ func TestValidateComponents(t *testing.T) { sameTargetPortErr := "devfile contains multiple containers with same TargetPort.*" invalidURIErr := ".*invalid URI for request" + pluginOverridesFromMainDevfile := attributes.Attributes{}.PutString(ImportSourceAttribute, + "uri: http://127.0.0.1:8080").PutString(PluginOverrideAttribute, "main devfile") + invalidURIErrWithImportAttributes := ".*invalid URI for request, imported from uri: http://127.0.0.1:8080, in plugin overrides from main devfile" + tests := []struct { name string components []v1alpha2.Component @@ -237,6 +242,23 @@ func TestValidateComponents(t *testing.T) { }, wantErr: &invalidURIErr, }, + { + name: "Invalid component due to bad URI with import source attributes", + components: []v1alpha2.Component{ + { + Attributes: pluginOverridesFromMainDevfile, + Name: "name", + ComponentUnion: v1alpha2.ComponentUnion{ + Plugin: &v1alpha2.PluginComponent{ + ImportReference: v1alpha2.ImportReference{ + RegistryUrl: "http//invalidregistryurl", + }, + }, + }, + }, + }, + wantErr: &invalidURIErrWithImportAttributes, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/pkg/validation/errors.go b/pkg/validation/errors.go index ecca9ff66..715cc9245 100644 --- a/pkg/validation/errors.go +++ b/pkg/validation/errors.go @@ -1,6 +1,9 @@ package validation -import "fmt" +import ( + "fmt" + attributesAPI "github.com/devfile/api/v2/pkg/attributes" +) // InvalidEventError returns an error if the devfile event type has invalid events type InvalidEventError struct { @@ -22,6 +25,15 @@ func (e *InvalidCommandError) Error() string { return fmt.Sprintf("the command %q is invalid - %s", e.commandId, e.reason) } +// InvalidCommandError returns an error if the command is invalid +type InvalidCommandTypeError struct { + commandId string +} + +func (e *InvalidCommandTypeError) Error() string { + return fmt.Sprintf("command %s type is invalid", e.commandId) +} + // ReservedEnvError returns an error if the user attempts to customize a reserved ENV in a container type ReservedEnvError struct { componentName string @@ -77,3 +89,29 @@ type InvalidComponentError struct { func (e *InvalidComponentError) Error() string { return fmt.Sprintf("the component %q is invalid - %s", e.componentName, e.reason) } + + +// resolveErrorMessageWithImportArrtibutes returns an error message with detailed information on the import resource +// example: "the component is invalid - ", imported from Uri: http://example.com/devfile.yaml, in parent overrides from main devfile" +func resolveErrorMessageWithImportArrtibutes(err error, attributes attributesAPI.Attributes) error { + var findKeyErr error + importReference := attributes.Get(ImportSourceAttribute, &findKeyErr) + returnErr := err + + if findKeyErr == nil { + returnErr = fmt.Errorf("%s, imported from %s", returnErr.Error(), importReference) + parentOverrideReference := attributes.Get(ParentOverrideAttribute, &findKeyErr) + if findKeyErr == nil { + returnErr = fmt.Errorf("%s, in parent overrides from %s", returnErr.Error(), parentOverrideReference) + } else { + // reset findKeyErr to nil + findKeyErr = nil + pluginOverrideReference := attributes.Get(PluginOverrideAttribute, &findKeyErr) + if findKeyErr == nil { + returnErr = fmt.Errorf("%s, in plugin overrides from %s", returnErr.Error(), pluginOverrideReference) + } + } + } + + return returnErr +} \ No newline at end of file diff --git a/pkg/validation/projects.go b/pkg/validation/projects.go index ea4e8880d..d3690979d 100644 --- a/pkg/validation/projects.go +++ b/pkg/validation/projects.go @@ -23,16 +23,22 @@ func ValidateStarterProjects(starterProjects []v1alpha2.StarterProject) error { switch len(gitSource.Remotes) { case 0: - errString += fmt.Sprintf("\nstarterProject %s should have at least one remote", starterProject.Name) + starterProjectErr := fmt.Errorf("\nstarterProject %s should have at least one remote", starterProject.Name) + newErr := resolveErrorMessageWithImportArrtibutes(starterProjectErr, starterProject.Attributes) + errString += newErr.Error() case 1: if gitSource.CheckoutFrom != nil && gitSource.CheckoutFrom.Remote != "" { err := validateRemoteMap(gitSource.Remotes, gitSource.CheckoutFrom.Remote, starterProject.Name) if err != nil { - errString += fmt.Sprintf("\n%s", err.Error()) + newErr := resolveErrorMessageWithImportArrtibutes(err, starterProject.Attributes) + errString += newErr.Error() + errString += fmt.Sprintf("\n%s", newErr.Error()) } } default: // len(gitSource.Remotes) >= 2 - errString += fmt.Sprintf("\nstarterProject %s should have one remote only", starterProject.Name) + starterProjectErr := fmt.Errorf("\nstarterProject %s should have one remote only", starterProject.Name) + newErr := resolveErrorMessageWithImportArrtibutes(starterProjectErr, starterProject.Attributes) + errString += newErr.Error() } } @@ -61,20 +67,26 @@ func ValidateProjects(projects []v1alpha2.Project) error { switch len(gitSource.Remotes) { case 0: - errString += fmt.Sprintf("\nprojects %s should have at least one remote", project.Name) + projectErr := fmt.Errorf("\nprojects %s should have at least one remote", project.Name) + newErr := resolveErrorMessageWithImportArrtibutes(projectErr, project.Attributes) + errString += newErr.Error() case 1: if gitSource.CheckoutFrom != nil && gitSource.CheckoutFrom.Remote != "" { if err := validateRemoteMap(gitSource.Remotes, gitSource.CheckoutFrom.Remote, project.Name); err != nil { - errString += fmt.Sprintf("\n%s", err.Error()) + newErr := resolveErrorMessageWithImportArrtibutes(err, project.Attributes) + errString += fmt.Sprintf("\n%s", newErr.Error()) } } default: // len(gitSource.Remotes) >= 2 if gitSource.CheckoutFrom == nil || gitSource.CheckoutFrom.Remote == "" { - errString += fmt.Sprintf("\nproject %s has more than one remote defined, but has no checkoutfrom remote defined", project.Name) + projectErr := fmt.Errorf("\nproject %s has more than one remote defined, but has no checkoutfrom remote defined", project.Name) + newErr := resolveErrorMessageWithImportArrtibutes(projectErr, project.Attributes) + errString += newErr.Error() continue } if err := validateRemoteMap(gitSource.Remotes, gitSource.CheckoutFrom.Remote, project.Name); err != nil { - errString += fmt.Sprintf("\n%s", err.Error()) + newErr := resolveErrorMessageWithImportArrtibutes(err, project.Attributes) + errString += fmt.Sprintf("\n%s", newErr.Error()) } } } diff --git a/pkg/validation/projects_test.go b/pkg/validation/projects_test.go index c32853e71..c8d1994b3 100644 --- a/pkg/validation/projects_test.go +++ b/pkg/validation/projects_test.go @@ -1,6 +1,7 @@ package validation import ( + "github.com/devfile/api/v2/pkg/attributes" "testing" "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" @@ -69,6 +70,10 @@ func TestValidateStarterProjects(t *testing.T) { wrongCheckoutErr := "unable to find the checkout remote .* in the remotes for project.*" atleastOneRemoteErr := "starterProject .* should have at least one remote" + parentOverridesFromMainDevfile := attributes.Attributes{}.PutString(ImportSourceAttribute, + "uri: http://127.0.0.1:8080").PutString(ParentOverrideAttribute, "main devfile") + wrongCheckoutErrWithImportAttributes := "unable to find the checkout remote .* in the remotes for project.*, imported from uri: http://127.0.0.1:8080, in parent overrides from main devfile" + tests := []struct { name string starterProjects []v1alpha2.StarterProject @@ -115,6 +120,24 @@ func TestValidateStarterProjects(t *testing.T) { }, wantErr: &atleastOneRemoteErr, }, + { + name: "Invalid Starter Project due to wrong checkout with import source attributes", + starterProjects: []v1alpha2.StarterProject{ + { + Attributes: parentOverridesFromMainDevfile, + Name: "starterproject1", + ProjectSource: v1alpha2.ProjectSource{ + Github: &v1alpha2.GithubProjectSource{ + GitLikeProjectSource: v1alpha2.GitLikeProjectSource{ + Remotes: map[string]string{"test": "testremote"}, + CheckoutFrom: &v1alpha2.CheckoutFrom{Remote: "origin"}, + }, + }, + }, + }, + }, + wantErr: &wrongCheckoutErrWithImportAttributes, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -135,6 +158,10 @@ func TestValidateProjects(t *testing.T) { atleastOneRemoteErr := "projects .* should have at least one remote" missingCheckOutFromRemoteErr := "project .* has more than one remote defined, but has no checkoutfrom remote defined" + parentOverridesFromMainDevfile := attributes.Attributes{}.PutString(ImportSourceAttribute, + "uri: http://127.0.0.1:8080").PutString(ParentOverrideAttribute, "main devfile") + wrongCheckoutErrWithImportAttributes := "unable to find the checkout remote .* in the remotes for project.*, imported from uri: http://127.0.0.1:8080, in parent overrides from main devfile" + tests := []struct { name string projects []v1alpha2.Project @@ -184,6 +211,24 @@ func TestValidateProjects(t *testing.T) { }, wantErr: &atleastOneRemoteErr, }, + { + name: "Invalid Project due to wrong checkout with import source attributes", + projects: []v1alpha2.Project{ + { + Attributes: parentOverridesFromMainDevfile, + Name: "project1", + ProjectSource: v1alpha2.ProjectSource{ + Github: &v1alpha2.GithubProjectSource{ + GitLikeProjectSource: v1alpha2.GitLikeProjectSource{ + Remotes: map[string]string{"test": "testremote"}, + CheckoutFrom: &v1alpha2.CheckoutFrom{Remote: "origin"}, + }, + }, + }, + }, + }, + wantErr: &wrongCheckoutErrWithImportAttributes, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/pkg/validation/utils.go b/pkg/validation/utils.go index e8de8ad38..e5296eb25 100644 --- a/pkg/validation/utils.go +++ b/pkg/validation/utils.go @@ -7,6 +7,12 @@ import ( "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" ) +const ( + ImportSourceAttribute = "library.devfile.io/imported-from" + ParentOverrideAttribute = "library.devfile.io/parent-override-from" + PluginOverrideAttribute = "library.devfile.io/plugin-override-from" +) + // getCommandsMap iterates through the commands and returns a map of command func getCommandsMap(commands []v1alpha2.Command) map[string]v1alpha2.Command { commandMap := make(map[string]v1alpha2.Command, len(commands)) From 92d5105c3348aa36da6ee3d00978a312512f3ec9 Mon Sep 17 00:00:00 2001 From: Stephanie Date: Thu, 22 Apr 2021 16:52:16 -0400 Subject: [PATCH 2/6] update go format Signed-off-by: Stephanie --- pkg/validation/commands_test.go | 4 +--- pkg/validation/components_test.go | 2 +- pkg/validation/errors.go | 3 +-- pkg/validation/projects_test.go | 4 ++-- 4 files changed, 5 insertions(+), 8 deletions(-) diff --git a/pkg/validation/commands_test.go b/pkg/validation/commands_test.go index d64297846..3dfa59817 100644 --- a/pkg/validation/commands_test.go +++ b/pkg/validation/commands_test.go @@ -77,12 +77,10 @@ func TestValidateCommands(t *testing.T) { multipleDefaultCmdErr := ".*there should be exactly one default command, currently there is more than one default command" invalidCmdErr := ".*command does not map to a container component" - parentOverridesFromMainDevfile := attributes.Attributes{}.PutString(ImportSourceAttribute, "uri: http://127.0.0.1:8080").PutString(ParentOverrideAttribute, "main devfile") invalidCmdErrWithImportAttributes := ".*command does not map to a container component, imported from uri: http://127.0.0.1:8080, in parent overrides from main devfile" - tests := []struct { name string commands []v1alpha2.Command @@ -156,7 +154,7 @@ func TestValidateCommands(t *testing.T) { commands: []v1alpha2.Command{ { Attributes: parentOverridesFromMainDevfile, - Id: "command", + Id: "command", CommandUnion: v1alpha2.CommandUnion{ Apply: &v1alpha2.ApplyCommand{ LabeledCommand: v1alpha2.LabeledCommand{ diff --git a/pkg/validation/components_test.go b/pkg/validation/components_test.go index 63dc2efda..9aa955504 100644 --- a/pkg/validation/components_test.go +++ b/pkg/validation/components_test.go @@ -247,7 +247,7 @@ func TestValidateComponents(t *testing.T) { components: []v1alpha2.Component{ { Attributes: pluginOverridesFromMainDevfile, - Name: "name", + Name: "name", ComponentUnion: v1alpha2.ComponentUnion{ Plugin: &v1alpha2.PluginComponent{ ImportReference: v1alpha2.ImportReference{ diff --git a/pkg/validation/errors.go b/pkg/validation/errors.go index 715cc9245..128a57173 100644 --- a/pkg/validation/errors.go +++ b/pkg/validation/errors.go @@ -90,7 +90,6 @@ func (e *InvalidComponentError) Error() string { return fmt.Sprintf("the component %q is invalid - %s", e.componentName, e.reason) } - // resolveErrorMessageWithImportArrtibutes returns an error message with detailed information on the import resource // example: "the component is invalid - ", imported from Uri: http://example.com/devfile.yaml, in parent overrides from main devfile" func resolveErrorMessageWithImportArrtibutes(err error, attributes attributesAPI.Attributes) error { @@ -114,4 +113,4 @@ func resolveErrorMessageWithImportArrtibutes(err error, attributes attributesAPI } return returnErr -} \ No newline at end of file +} diff --git a/pkg/validation/projects_test.go b/pkg/validation/projects_test.go index c8d1994b3..6acc879ec 100644 --- a/pkg/validation/projects_test.go +++ b/pkg/validation/projects_test.go @@ -125,7 +125,7 @@ func TestValidateStarterProjects(t *testing.T) { starterProjects: []v1alpha2.StarterProject{ { Attributes: parentOverridesFromMainDevfile, - Name: "starterproject1", + Name: "starterproject1", ProjectSource: v1alpha2.ProjectSource{ Github: &v1alpha2.GithubProjectSource{ GitLikeProjectSource: v1alpha2.GitLikeProjectSource{ @@ -216,7 +216,7 @@ func TestValidateProjects(t *testing.T) { projects: []v1alpha2.Project{ { Attributes: parentOverridesFromMainDevfile, - Name: "project1", + Name: "project1", ProjectSource: v1alpha2.ProjectSource{ Github: &v1alpha2.GithubProjectSource{ GitLikeProjectSource: v1alpha2.GitLikeProjectSource{ From 4ba783751342199133b3ba0f10bc1773abeb8ce9 Mon Sep 17 00:00:00 2001 From: Stephanie Date: Thu, 29 Apr 2021 12:19:57 -0400 Subject: [PATCH 3/6] address some review comments Signed-off-by: Stephanie --- pkg/validation/errors.go | 23 +++++++++++++++-------- pkg/validation/projects.go | 1 - pkg/validation/utils.go | 7 ++++++- 3 files changed, 21 insertions(+), 10 deletions(-) diff --git a/pkg/validation/errors.go b/pkg/validation/errors.go index 128a57173..80efe5b7f 100644 --- a/pkg/validation/errors.go +++ b/pkg/validation/errors.go @@ -90,27 +90,34 @@ func (e *InvalidComponentError) Error() string { return fmt.Sprintf("the component %q is invalid - %s", e.componentName, e.reason) } -// resolveErrorMessageWithImportArrtibutes returns an error message with detailed information on the import resource -// example: "the component is invalid - ", imported from Uri: http://example.com/devfile.yaml, in parent overrides from main devfile" -func resolveErrorMessageWithImportArrtibutes(err error, attributes attributesAPI.Attributes) error { +// resolveErrorMessageWithImportArrtibutes returns an updated error message +// with detailed information on the imported and overriden resource. +// example: +// "the component is invalid - , imported from Uri: http://example.com/devfile.yaml, in parent overrides from main devfile" +func resolveErrorMessageWithImportArrtibutes(validationErr error, attributes attributesAPI.Attributes) error { var findKeyErr error importReference := attributes.Get(ImportSourceAttribute, &findKeyErr) - returnErr := err + // overridden element must contain import resource information + // an overridden element can be either parentOverride or pluginOverride + // example: + // if an element is imported from another devfile, but contains no overrides - ImportSourceAttribute + // if an element is from parentOverride - ImportSourceAttribute + ParentOverrideAttribute + // if an element is from pluginOverride - ImportSourceAttribute + PluginOverrideAttribute if findKeyErr == nil { - returnErr = fmt.Errorf("%s, imported from %s", returnErr.Error(), importReference) + validationErr = fmt.Errorf("%s, imported from %s", validationErr.Error(), importReference) parentOverrideReference := attributes.Get(ParentOverrideAttribute, &findKeyErr) if findKeyErr == nil { - returnErr = fmt.Errorf("%s, in parent overrides from %s", returnErr.Error(), parentOverrideReference) + validationErr = fmt.Errorf("%s, in parent overrides from %s", validationErr.Error(), parentOverrideReference) } else { // reset findKeyErr to nil findKeyErr = nil pluginOverrideReference := attributes.Get(PluginOverrideAttribute, &findKeyErr) if findKeyErr == nil { - returnErr = fmt.Errorf("%s, in plugin overrides from %s", returnErr.Error(), pluginOverrideReference) + validationErr = fmt.Errorf("%s, in plugin overrides from %s", validationErr.Error(), pluginOverrideReference) } } } - return returnErr + return validationErr } diff --git a/pkg/validation/projects.go b/pkg/validation/projects.go index d3690979d..a62ec6c7c 100644 --- a/pkg/validation/projects.go +++ b/pkg/validation/projects.go @@ -31,7 +31,6 @@ func ValidateStarterProjects(starterProjects []v1alpha2.StarterProject) error { err := validateRemoteMap(gitSource.Remotes, gitSource.CheckoutFrom.Remote, starterProject.Name) if err != nil { newErr := resolveErrorMessageWithImportArrtibutes(err, starterProject.Attributes) - errString += newErr.Error() errString += fmt.Sprintf("\n%s", newErr.Error()) } } diff --git a/pkg/validation/utils.go b/pkg/validation/utils.go index e5296eb25..a2d3af54f 100644 --- a/pkg/validation/utils.go +++ b/pkg/validation/utils.go @@ -7,9 +7,14 @@ import ( "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" ) +// attribute keys for imported and overridden elements +// the value of those keys is the resource information const ( - ImportSourceAttribute = "library.devfile.io/imported-from" + // attribute key of the imported element resource information + ImportSourceAttribute = "library.devfile.io/imported-from" + // attribute key of the parent overridden element resource information ParentOverrideAttribute = "library.devfile.io/parent-override-from" + // attribute key of the plugin overridden element resource information PluginOverrideAttribute = "library.devfile.io/plugin-override-from" ) From a22cea840ce384acf6e0da4da6ac55da93358035 Mon Sep 17 00:00:00 2001 From: Stephanie Date: Thu, 29 Apr 2021 14:36:20 -0400 Subject: [PATCH 4/6] check for attribute and print out all default command and it's import reference if any Signed-off-by: Stephanie --- pkg/validation/commands.go | 13 ++++++-- pkg/validation/commands_test.go | 42 ++++++++++++------------ pkg/validation/components.go | 16 ++++----- pkg/validation/components_test.go | 19 +++-------- pkg/validation/errors.go | 4 +-- pkg/validation/events_test.go | 9 +++--- pkg/validation/projects.go | 14 ++++---- pkg/validation/projects_test.go | 54 ++++++++++--------------------- 8 files changed, 76 insertions(+), 95 deletions(-) diff --git a/pkg/validation/commands.go b/pkg/validation/commands.go index eaea7e125..e48fbf6a0 100644 --- a/pkg/validation/commands.go +++ b/pkg/validation/commands.go @@ -25,7 +25,7 @@ func ValidateCommands(commands []v1alpha2.Command, components []v1alpha2.Compone parentCommands := make(map[string]string) err = validateCommand(command, parentCommands, commandMap, components) if err != nil { - return resolveErrorMessageWithImportArrtibutes(err, command.Attributes) + return resolveErrorMessageWithImportAttributes(err, command.Attributes) } commandGroup := getGroup(command) @@ -68,11 +68,12 @@ func validateCommand(command v1alpha2.Command, parentCommands map[string]string, // 2. with more than one default, err out func validateGroup(commands []v1alpha2.Command) error { defaultCommandCount := 0 - + var defaultCommands []v1alpha2.Command if len(commands) > 1 { for _, command := range commands { if getGroup(command).IsDefault { defaultCommandCount++ + defaultCommands = append(defaultCommands, command) } } } else { @@ -82,7 +83,13 @@ func validateGroup(commands []v1alpha2.Command) error { if defaultCommandCount == 0 { return fmt.Errorf("there should be exactly one default command, currently there is no default command") } else if defaultCommandCount > 1 { - return fmt.Errorf("there should be exactly one default command, currently there is more than one default command") + var commandsReference string + for _, command := range defaultCommands { + commandsReference += resolveErrorMessageWithImportAttributes(fmt.Errorf("; command: %s", command.Id), command.Attributes).Error() + } + // example: there should be exactly one default command, currently there is more than one default command; + // command: ; command: , imported from uri: http://127.0.0.1:8080, in parent overrides from main devfile" + return fmt.Errorf("there should be exactly one default command, currently there is more than one default command%s", commandsReference) } return nil diff --git a/pkg/validation/commands_test.go b/pkg/validation/commands_test.go index 3dfa59817..b05f4bb15 100644 --- a/pkg/validation/commands_test.go +++ b/pkg/validation/commands_test.go @@ -31,9 +31,10 @@ func generateDummyExecCommand(name, component string, group *v1alpha2.CommandGro } // generateDummyExecCommand returns a dummy apply command for testing -func generateDummyApplyCommand(name, component string, group *v1alpha2.CommandGroup) v1alpha2.Command { +func generateDummyApplyCommand(name, component string, group *v1alpha2.CommandGroup, cmdAttributes attributes.Attributes) v1alpha2.Command { return v1alpha2.Command{ - Id: name, + Attributes: cmdAttributes, + Id: name, CommandUnion: v1alpha2.CommandUnion{ Apply: &v1alpha2.ApplyCommand{ LabeledCommand: v1alpha2.LabeledCommand{ @@ -136,7 +137,7 @@ func TestValidateCommands(t *testing.T) { { name: "Invalid Apply command with wrong component", commands: []v1alpha2.Command{ - generateDummyApplyCommand("command", "invalidComponent", nil), + generateDummyApplyCommand("command", "invalidComponent", nil, attributes.Attributes{}), }, wantErr: &invalidCmdErr, }, @@ -152,20 +153,7 @@ func TestValidateCommands(t *testing.T) { { name: "Invalid command with import source attribute", commands: []v1alpha2.Command{ - { - Attributes: parentOverridesFromMainDevfile, - Id: "command", - CommandUnion: v1alpha2.CommandUnion{ - Apply: &v1alpha2.ApplyCommand{ - LabeledCommand: v1alpha2.LabeledCommand{ - BaseCommand: v1alpha2.BaseCommand{ - Group: nil, - }, - }, - Component: "invalidComponent", - }, - }, - }, + generateDummyApplyCommand("command", "invalidComponent", nil, parentOverridesFromMainDevfile), }, wantErr: &invalidCmdErrWithImportAttributes, }, @@ -218,11 +206,11 @@ func TestValidateCommandComponent(t *testing.T) { }, { name: "Valid Apply Command", - command: generateDummyApplyCommand("command", component, nil), + command: generateDummyApplyCommand("command", component, nil, attributes.Attributes{}), }, { name: "Invalid Apply Command with wrong component", - command: generateDummyApplyCommand("command", invalidComponent, &v1alpha2.CommandGroup{Kind: runGroup}), + command: generateDummyApplyCommand("command", invalidComponent, &v1alpha2.CommandGroup{Kind: runGroup}, attributes.Attributes{}), wantErr: &invalidCmdErr, }, } @@ -332,7 +320,13 @@ func TestValidateGroup(t *testing.T) { component := "alias1" noDefaultCmdErr := ".*there should be exactly one default command, currently there is no default command" - multipleDefaultCmdErr := ".*there should be exactly one default command, currently there is more than one default command" + multipleDefaultError := ".*there should be exactly one default command, currently there is more than one default command" + multipleDefaultCmdErr := multipleDefaultError + "; command: run command; command: customcommand" + + parentOverridesFromMainDevfile := attributes.Attributes{}.PutString(ImportSourceAttribute, + "uri: http://127.0.0.1:8080").PutString(ParentOverrideAttribute, "main devfile") + multipleDefaultCmdErrWithImportAttributes := multipleDefaultError + + "; command: run command; command: customcommand, imported from uri: http://127.0.0.1:8080, in parent overrides from main devfile" tests := []struct { name string @@ -347,6 +341,14 @@ func TestValidateGroup(t *testing.T) { }, wantErr: &multipleDefaultCmdErr, }, + { + name: "Two default run commands with import source attribute", + commands: []v1alpha2.Command{ + generateDummyExecCommand("run command", component, &v1alpha2.CommandGroup{Kind: runGroup, IsDefault: true}), + generateDummyApplyCommand("customcommand", component, &v1alpha2.CommandGroup{Kind: runGroup, IsDefault: true}, parentOverridesFromMainDevfile), + }, + wantErr: &multipleDefaultCmdErrWithImportAttributes, + }, { name: "No default for more than one build commands", commands: []v1alpha2.Command{ diff --git a/pkg/validation/components.go b/pkg/validation/components.go index c07f3e821..5c4b40470 100644 --- a/pkg/validation/components.go +++ b/pkg/validation/components.go @@ -54,7 +54,7 @@ func ValidateComponents(components []v1alpha2.Component) error { err := validateEndpoints(component.Container.Endpoints, processedEndPointPort, processedEndPointName) if err != nil { - return resolveErrorMessageWithImportArrtibutes(err, component.Attributes) + return resolveErrorMessageWithImportAttributes(err, component.Attributes) } case component.Volume != nil: processedVolumes[component.Name] = true @@ -64,37 +64,37 @@ func ValidateComponents(components []v1alpha2.Component) error { // https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/ if _, err := resource.ParseQuantity(component.Volume.Size); err != nil { invalidVolErr := &InvalidVolumeError{name: component.Name, reason: fmt.Sprintf("size %s for volume component is invalid, %v. Example - 2Gi, 1024Mi", component.Volume.Size, err)} - return resolveErrorMessageWithImportArrtibutes(invalidVolErr, component.Attributes) + return resolveErrorMessageWithImportAttributes(invalidVolErr, component.Attributes) } } case component.Openshift != nil: if component.Openshift.Uri != "" { err := ValidateURI(component.Openshift.Uri) if err != nil { - return resolveErrorMessageWithImportArrtibutes(err, component.Attributes) + return resolveErrorMessageWithImportAttributes(err, component.Attributes) } } err := validateEndpoints(component.Openshift.Endpoints, processedEndPointPort, processedEndPointName) if err != nil { - return resolveErrorMessageWithImportArrtibutes(err, component.Attributes) + return resolveErrorMessageWithImportAttributes(err, component.Attributes) } case component.Kubernetes != nil: if component.Kubernetes.Uri != "" { err := ValidateURI(component.Kubernetes.Uri) if err != nil { - return resolveErrorMessageWithImportArrtibutes(err, component.Attributes) + return resolveErrorMessageWithImportAttributes(err, component.Attributes) } } err := validateEndpoints(component.Kubernetes.Endpoints, processedEndPointPort, processedEndPointName) if err != nil { - return resolveErrorMessageWithImportArrtibutes(err, component.Attributes) + return resolveErrorMessageWithImportAttributes(err, component.Attributes) } case component.Plugin != nil: if component.Plugin.RegistryUrl != "" { err := ValidateURI(component.Plugin.RegistryUrl) if err != nil { - return resolveErrorMessageWithImportArrtibutes(err, component.Attributes) + return resolveErrorMessageWithImportAttributes(err, component.Attributes) } } } @@ -107,7 +107,7 @@ func ValidateComponents(components []v1alpha2.Component) error { for _, volumeMountName := range volumeMountNames { if !processedVolumes[volumeMountName] { missingVolumeMountErr := fmt.Errorf("\nvolume mount %s belonging to the container component %s", volumeMountName, componentName) - newErr := resolveErrorMessageWithImportArrtibutes(missingVolumeMountErr, processedComponentWithVolumeMounts[componentName].Attributes) + newErr := resolveErrorMessageWithImportAttributes(missingVolumeMountErr, processedComponentWithVolumeMounts[componentName].Attributes) invalidVolumeMountsErr += newErr.Error() } } diff --git a/pkg/validation/components_test.go b/pkg/validation/components_test.go index 9aa955504..81a47eb90 100644 --- a/pkg/validation/components_test.go +++ b/pkg/validation/components_test.go @@ -80,10 +80,11 @@ func generateDummyKubernetesComponent(name string, endpoints []v1alpha2.Endpoint } // generateDummyPluginComponent returns a dummy Plugin component for testing -func generateDummyPluginComponent(name, url string) v1alpha2.Component { +func generateDummyPluginComponent(name, url string, compAttribute attributes.Attributes) v1alpha2.Component { return v1alpha2.Component{ - Name: name, + Attributes: compAttribute, + Name: name, ComponentUnion: v1alpha2.ComponentUnion{ Plugin: &v1alpha2.PluginComponent{ ImportReference: v1alpha2.ImportReference{ @@ -238,24 +239,14 @@ func TestValidateComponents(t *testing.T) { { name: "Invalid plugin registry url", components: []v1alpha2.Component{ - generateDummyPluginComponent("abc", "http//invalidregistryurl"), + generateDummyPluginComponent("abc", "http//invalidregistryurl", attributes.Attributes{}), }, wantErr: &invalidURIErr, }, { name: "Invalid component due to bad URI with import source attributes", components: []v1alpha2.Component{ - { - Attributes: pluginOverridesFromMainDevfile, - Name: "name", - ComponentUnion: v1alpha2.ComponentUnion{ - Plugin: &v1alpha2.PluginComponent{ - ImportReference: v1alpha2.ImportReference{ - RegistryUrl: "http//invalidregistryurl", - }, - }, - }, - }, + generateDummyPluginComponent("abc", "http//invalidregistryurl", pluginOverridesFromMainDevfile), }, wantErr: &invalidURIErrWithImportAttributes, }, diff --git a/pkg/validation/errors.go b/pkg/validation/errors.go index 80efe5b7f..a87b8d723 100644 --- a/pkg/validation/errors.go +++ b/pkg/validation/errors.go @@ -90,11 +90,11 @@ func (e *InvalidComponentError) Error() string { return fmt.Sprintf("the component %q is invalid - %s", e.componentName, e.reason) } -// resolveErrorMessageWithImportArrtibutes returns an updated error message +// resolveErrorMessageWithImportAttributes returns an updated error message // with detailed information on the imported and overriden resource. // example: // "the component is invalid - , imported from Uri: http://example.com/devfile.yaml, in parent overrides from main devfile" -func resolveErrorMessageWithImportArrtibutes(validationErr error, attributes attributesAPI.Attributes) error { +func resolveErrorMessageWithImportAttributes(validationErr error, attributes attributesAPI.Attributes) error { var findKeyErr error importReference := attributes.Get(ImportSourceAttribute, &findKeyErr) diff --git a/pkg/validation/events_test.go b/pkg/validation/events_test.go index d7bf5ed81..4d2db5aa4 100644 --- a/pkg/validation/events_test.go +++ b/pkg/validation/events_test.go @@ -4,6 +4,7 @@ import ( "testing" "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" + "github.com/devfile/api/v2/pkg/attributes" "github.com/stretchr/testify/assert" ) @@ -12,8 +13,8 @@ func TestValidateEvents(t *testing.T) { containers := []string{"container1", "container2"} commands := []v1alpha2.Command{ - generateDummyApplyCommand("apply1", containers[0], nil), - generateDummyApplyCommand("apply2", containers[0], nil), + generateDummyApplyCommand("apply1", containers[0], nil, attributes.Attributes{}), + generateDummyApplyCommand("apply2", containers[0], nil, attributes.Attributes{}), generateDummyExecCommand("exec1", containers[1], nil), generateDummyExecCommand("exec2", containers[1], nil), generateDummyCompositeCommand("compositeOnlyApply", []string{"apply1", "apply2"}, nil), @@ -112,8 +113,8 @@ func TestIsEventValid(t *testing.T) { containers := []string{"container1", "container2"} commands := []v1alpha2.Command{ - generateDummyApplyCommand("apply1", containers[0], nil), - generateDummyApplyCommand("apply2", containers[0], nil), + generateDummyApplyCommand("apply1", containers[0], nil, attributes.Attributes{}), + generateDummyApplyCommand("apply2", containers[0], nil, attributes.Attributes{}), generateDummyExecCommand("exec1", containers[1], nil), generateDummyExecCommand("exec2", containers[1], nil), generateDummyCompositeCommand("compositeOnlyApply", []string{"apply1", "apply2"}, nil), diff --git a/pkg/validation/projects.go b/pkg/validation/projects.go index a62ec6c7c..b520674cd 100644 --- a/pkg/validation/projects.go +++ b/pkg/validation/projects.go @@ -24,19 +24,19 @@ func ValidateStarterProjects(starterProjects []v1alpha2.StarterProject) error { switch len(gitSource.Remotes) { case 0: starterProjectErr := fmt.Errorf("\nstarterProject %s should have at least one remote", starterProject.Name) - newErr := resolveErrorMessageWithImportArrtibutes(starterProjectErr, starterProject.Attributes) + newErr := resolveErrorMessageWithImportAttributes(starterProjectErr, starterProject.Attributes) errString += newErr.Error() case 1: if gitSource.CheckoutFrom != nil && gitSource.CheckoutFrom.Remote != "" { err := validateRemoteMap(gitSource.Remotes, gitSource.CheckoutFrom.Remote, starterProject.Name) if err != nil { - newErr := resolveErrorMessageWithImportArrtibutes(err, starterProject.Attributes) + newErr := resolveErrorMessageWithImportAttributes(err, starterProject.Attributes) errString += fmt.Sprintf("\n%s", newErr.Error()) } } default: // len(gitSource.Remotes) >= 2 starterProjectErr := fmt.Errorf("\nstarterProject %s should have one remote only", starterProject.Name) - newErr := resolveErrorMessageWithImportArrtibutes(starterProjectErr, starterProject.Attributes) + newErr := resolveErrorMessageWithImportAttributes(starterProjectErr, starterProject.Attributes) errString += newErr.Error() } } @@ -67,24 +67,24 @@ func ValidateProjects(projects []v1alpha2.Project) error { switch len(gitSource.Remotes) { case 0: projectErr := fmt.Errorf("\nprojects %s should have at least one remote", project.Name) - newErr := resolveErrorMessageWithImportArrtibutes(projectErr, project.Attributes) + newErr := resolveErrorMessageWithImportAttributes(projectErr, project.Attributes) errString += newErr.Error() case 1: if gitSource.CheckoutFrom != nil && gitSource.CheckoutFrom.Remote != "" { if err := validateRemoteMap(gitSource.Remotes, gitSource.CheckoutFrom.Remote, project.Name); err != nil { - newErr := resolveErrorMessageWithImportArrtibutes(err, project.Attributes) + newErr := resolveErrorMessageWithImportAttributes(err, project.Attributes) errString += fmt.Sprintf("\n%s", newErr.Error()) } } default: // len(gitSource.Remotes) >= 2 if gitSource.CheckoutFrom == nil || gitSource.CheckoutFrom.Remote == "" { projectErr := fmt.Errorf("\nproject %s has more than one remote defined, but has no checkoutfrom remote defined", project.Name) - newErr := resolveErrorMessageWithImportArrtibutes(projectErr, project.Attributes) + newErr := resolveErrorMessageWithImportAttributes(projectErr, project.Attributes) errString += newErr.Error() continue } if err := validateRemoteMap(gitSource.Remotes, gitSource.CheckoutFrom.Remote, project.Name); err != nil { - newErr := resolveErrorMessageWithImportArrtibutes(err, project.Attributes) + newErr := resolveErrorMessageWithImportAttributes(err, project.Attributes) errString += fmt.Sprintf("\n%s", newErr.Error()) } } diff --git a/pkg/validation/projects_test.go b/pkg/validation/projects_test.go index 6acc879ec..fd2549b82 100644 --- a/pkg/validation/projects_test.go +++ b/pkg/validation/projects_test.go @@ -8,9 +8,10 @@ import ( "github.com/stretchr/testify/assert" ) -func generateDummyGitStarterProject(name string, checkoutRemote *v1alpha2.CheckoutFrom, remotes map[string]string) v1alpha2.StarterProject { +func generateDummyGitStarterProject(name string, checkoutRemote *v1alpha2.CheckoutFrom, remotes map[string]string, projectAttribute attributes.Attributes) v1alpha2.StarterProject { return v1alpha2.StarterProject{ - Name: name, + Attributes: projectAttribute, + Name: name, ProjectSource: v1alpha2.ProjectSource{ Git: &v1alpha2.GitProjectSource{ GitLikeProjectSource: v1alpha2.GitLikeProjectSource{ @@ -36,9 +37,10 @@ func generateDummyGithubStarterProject(name string, checkoutRemote *v1alpha2.Che } } -func generateDummyGitProject(name string, checkoutRemote *v1alpha2.CheckoutFrom, remotes map[string]string) v1alpha2.Project { +func generateDummyGitProject(name string, checkoutRemote *v1alpha2.CheckoutFrom, remotes map[string]string, projectAttribute attributes.Attributes) v1alpha2.Project { return v1alpha2.Project{ - Name: name, + Attributes: projectAttribute, + Name: name, ProjectSource: v1alpha2.ProjectSource{ Git: &v1alpha2.GitProjectSource{ GitLikeProjectSource: v1alpha2.GitLikeProjectSource{ @@ -82,8 +84,8 @@ func TestValidateStarterProjects(t *testing.T) { { name: "Valid Starter Project", starterProjects: []v1alpha2.StarterProject{ - generateDummyGitStarterProject("project1", &v1alpha2.CheckoutFrom{Remote: "origin"}, map[string]string{"origin": "originremote"}), - generateDummyGitStarterProject("project2", &v1alpha2.CheckoutFrom{Remote: "origin"}, map[string]string{"origin": "originremote2"}), + generateDummyGitStarterProject("project1", &v1alpha2.CheckoutFrom{Remote: "origin"}, map[string]string{"origin": "originremote"}, attributes.Attributes{}), + generateDummyGitStarterProject("project2", &v1alpha2.CheckoutFrom{Remote: "origin"}, map[string]string{"origin": "originremote2"}, attributes.Attributes{}), }, }, { @@ -103,13 +105,13 @@ func TestValidateStarterProjects(t *testing.T) { { name: "Valid Starter Project with empty checkout remote", starterProjects: []v1alpha2.StarterProject{ - generateDummyGitStarterProject("project1", &v1alpha2.CheckoutFrom{Remote: ""}, map[string]string{"origin": "originremote"}), + generateDummyGitStarterProject("project1", &v1alpha2.CheckoutFrom{Remote: ""}, map[string]string{"origin": "originremote"}, attributes.Attributes{}), }, }, { name: "Valid Starter Project with no checkout remote", starterProjects: []v1alpha2.StarterProject{ - generateDummyGitStarterProject("project1", nil, map[string]string{"origin": "originremote"}), + generateDummyGitStarterProject("project1", nil, map[string]string{"origin": "originremote"}, attributes.Attributes{}), }, }, { @@ -123,18 +125,7 @@ func TestValidateStarterProjects(t *testing.T) { { name: "Invalid Starter Project due to wrong checkout with import source attributes", starterProjects: []v1alpha2.StarterProject{ - { - Attributes: parentOverridesFromMainDevfile, - Name: "starterproject1", - ProjectSource: v1alpha2.ProjectSource{ - Github: &v1alpha2.GithubProjectSource{ - GitLikeProjectSource: v1alpha2.GitLikeProjectSource{ - Remotes: map[string]string{"test": "testremote"}, - CheckoutFrom: &v1alpha2.CheckoutFrom{Remote: "origin"}, - }, - }, - }, - }, + generateDummyGitStarterProject("project1", &v1alpha2.CheckoutFrom{Remote: "origin"}, map[string]string{"test": "testremote"}, parentOverridesFromMainDevfile), }, wantErr: &wrongCheckoutErrWithImportAttributes, }, @@ -170,7 +161,7 @@ func TestValidateProjects(t *testing.T) { { name: "Valid Project", projects: []v1alpha2.Project{ - generateDummyGitProject("project1", &v1alpha2.CheckoutFrom{Remote: "origin"}, map[string]string{"origin": "originremote"}), + generateDummyGitProject("project1", &v1alpha2.CheckoutFrom{Remote: "origin"}, map[string]string{"origin": "originremote"}, attributes.Attributes{}), generateDummyGithubProject("project2", &v1alpha2.CheckoutFrom{Remote: "origin"}, map[string]string{"origin": "originremote"}), }, }, @@ -184,7 +175,7 @@ func TestValidateProjects(t *testing.T) { { name: "Invalid Project with multiple remote and empty checkout remote", projects: []v1alpha2.Project{ - generateDummyGitProject("project2", &v1alpha2.CheckoutFrom{Remote: "origin"}, map[string]string{"origin": "originremote"}), + generateDummyGitProject("project2", &v1alpha2.CheckoutFrom{Remote: "origin"}, map[string]string{"origin": "originremote"}, attributes.Attributes{}), generateDummyGithubProject("project1", &v1alpha2.CheckoutFrom{Remote: ""}, map[string]string{"origin": "originremote", "test": "testremote"}), }, wantErr: &missingCheckOutFromRemoteErr, @@ -193,20 +184,20 @@ func TestValidateProjects(t *testing.T) { name: "Invalid Project with wrong checkout", projects: []v1alpha2.Project{ generateDummyGithubProject("project1", &v1alpha2.CheckoutFrom{Remote: "origin1"}, map[string]string{"origin": "originremote", "test": "testremote"}), - generateDummyGitProject("project2", &v1alpha2.CheckoutFrom{Remote: "origin1"}, map[string]string{"origin2": "originremote2"}), + generateDummyGitProject("project2", &v1alpha2.CheckoutFrom{Remote: "origin1"}, map[string]string{"origin2": "originremote2"}, attributes.Attributes{}), }, wantErr: &wrongCheckoutErr, }, { name: "Valid Project with empty checkout remote", projects: []v1alpha2.Project{ - generateDummyGitProject("project1", &v1alpha2.CheckoutFrom{Remote: ""}, map[string]string{"origin": "originremote"}), + generateDummyGitProject("project1", &v1alpha2.CheckoutFrom{Remote: ""}, map[string]string{"origin": "originremote"}, attributes.Attributes{}), }, }, { name: "Invalid Project with empty remotes", projects: []v1alpha2.Project{ - generateDummyGitProject("project1", &v1alpha2.CheckoutFrom{Remote: "origin"}, map[string]string{}), + generateDummyGitProject("project1", &v1alpha2.CheckoutFrom{Remote: "origin"}, map[string]string{}, attributes.Attributes{}), generateDummyGithubProject("project2", &v1alpha2.CheckoutFrom{Remote: "origins"}, map[string]string{"origin": "originremote", "test": "testremote"}), }, wantErr: &atleastOneRemoteErr, @@ -214,18 +205,7 @@ func TestValidateProjects(t *testing.T) { { name: "Invalid Project due to wrong checkout with import source attributes", projects: []v1alpha2.Project{ - { - Attributes: parentOverridesFromMainDevfile, - Name: "project1", - ProjectSource: v1alpha2.ProjectSource{ - Github: &v1alpha2.GithubProjectSource{ - GitLikeProjectSource: v1alpha2.GitLikeProjectSource{ - Remotes: map[string]string{"test": "testremote"}, - CheckoutFrom: &v1alpha2.CheckoutFrom{Remote: "origin"}, - }, - }, - }, - }, + generateDummyGitProject("project1", &v1alpha2.CheckoutFrom{Remote: "origin"}, map[string]string{"test": "testremote"}, parentOverridesFromMainDevfile), }, wantErr: &wrongCheckoutErrWithImportAttributes, }, From 7dc039839a8b8d5ccf583c2beb599e1ecc3cd9a7 Mon Sep 17 00:00:00 2001 From: Stephanie Date: Mon, 3 May 2021 12:05:47 -0400 Subject: [PATCH 5/6] update the attribute keys to use prefix: api.devfile.io Signed-off-by: Stephanie --- pkg/validation/utils.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/validation/utils.go b/pkg/validation/utils.go index a2d3af54f..6f0ec12df 100644 --- a/pkg/validation/utils.go +++ b/pkg/validation/utils.go @@ -11,11 +11,11 @@ import ( // the value of those keys is the resource information const ( // attribute key of the imported element resource information - ImportSourceAttribute = "library.devfile.io/imported-from" + ImportSourceAttribute = "api.devfile.io/imported-from" // attribute key of the parent overridden element resource information - ParentOverrideAttribute = "library.devfile.io/parent-override-from" + ParentOverrideAttribute = "api.devfile.io/parent-override-from" // attribute key of the plugin overridden element resource information - PluginOverrideAttribute = "library.devfile.io/plugin-override-from" + PluginOverrideAttribute = "api.devfile.io/plugin-override-from" ) // getCommandsMap iterates through the commands and returns a map of command From 7cc978e648ec167610ca7096fd1cf9316dae93d6 Mon Sep 17 00:00:00 2001 From: Stephanie Date: Thu, 6 May 2021 11:43:35 -0400 Subject: [PATCH 6/6] use string join to join & split errors Signed-off-by: Stephanie --- pkg/validation/commands.go | 17 +++++++++------- pkg/validation/components.go | 10 ++++++---- pkg/validation/errors.go | 2 +- pkg/validation/events.go | 25 ++++++++++++------------ pkg/validation/projects.go | 38 +++++++++++++++++++----------------- 5 files changed, 50 insertions(+), 42 deletions(-) diff --git a/pkg/validation/commands.go b/pkg/validation/commands.go index e48fbf6a0..64d024bb1 100644 --- a/pkg/validation/commands.go +++ b/pkg/validation/commands.go @@ -34,15 +34,16 @@ func ValidateCommands(commands []v1alpha2.Command, components []v1alpha2.Compone } } - groupErrors := "" + var groupErrorsList []string for groupKind, commands := range groupKindCommandMap { if err = validateGroup(commands); err != nil { - groupErrors += fmt.Sprintf("\ncommand group %s error - %s", groupKind, err.Error()) + groupErrorsList = append(groupErrorsList, fmt.Sprintf("command group %s error - %s", groupKind, err.Error())) } } - if len(groupErrors) > 0 { - err = fmt.Errorf("%s", groupErrors) + if len(groupErrorsList) > 0 { + groupErrors := strings.Join(groupErrorsList, "\n") + err = fmt.Errorf("\n%s", groupErrors) } return err @@ -83,13 +84,15 @@ func validateGroup(commands []v1alpha2.Command) error { if defaultCommandCount == 0 { return fmt.Errorf("there should be exactly one default command, currently there is no default command") } else if defaultCommandCount > 1 { - var commandsReference string + var commandsReferenceList []string for _, command := range defaultCommands { - commandsReference += resolveErrorMessageWithImportAttributes(fmt.Errorf("; command: %s", command.Id), command.Attributes).Error() + commandsReferenceList = append(commandsReferenceList, + resolveErrorMessageWithImportAttributes(fmt.Errorf("command: %s", command.Id), command.Attributes).Error()) } + commandsReference := strings.Join(commandsReferenceList, "; ") // example: there should be exactly one default command, currently there is more than one default command; // command: ; command: , imported from uri: http://127.0.0.1:8080, in parent overrides from main devfile" - return fmt.Errorf("there should be exactly one default command, currently there is more than one default command%s", commandsReference) + return fmt.Errorf("there should be exactly one default command, currently there is more than one default command; %s", commandsReference) } return nil diff --git a/pkg/validation/components.go b/pkg/validation/components.go index 5c4b40470..5833abc29 100644 --- a/pkg/validation/components.go +++ b/pkg/validation/components.go @@ -2,6 +2,7 @@ package validation import ( "fmt" + "strings" "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" "k8s.io/apimachinery/pkg/api/resource" @@ -102,18 +103,19 @@ func ValidateComponents(components []v1alpha2.Component) error { } // Check if the volume mounts mentioned in the containers are referenced by a volume component - var invalidVolumeMountsErr string + var invalidVolumeMountsErrList []string for componentName, volumeMountNames := range processedVolumeMounts { for _, volumeMountName := range volumeMountNames { if !processedVolumes[volumeMountName] { - missingVolumeMountErr := fmt.Errorf("\nvolume mount %s belonging to the container component %s", volumeMountName, componentName) + missingVolumeMountErr := fmt.Errorf("volume mount %s belonging to the container component %s", volumeMountName, componentName) newErr := resolveErrorMessageWithImportAttributes(missingVolumeMountErr, processedComponentWithVolumeMounts[componentName].Attributes) - invalidVolumeMountsErr += newErr.Error() + invalidVolumeMountsErrList = append(invalidVolumeMountsErrList, newErr.Error()) } } } - if len(invalidVolumeMountsErr) > 0 { + if len(invalidVolumeMountsErrList) > 0 { + invalidVolumeMountsErr := fmt.Sprintf("\n%s", strings.Join(invalidVolumeMountsErrList, "\n")) return &MissingVolumeMountError{errMsg: invalidVolumeMountsErr} } diff --git a/pkg/validation/errors.go b/pkg/validation/errors.go index a87b8d723..667565e3f 100644 --- a/pkg/validation/errors.go +++ b/pkg/validation/errors.go @@ -31,7 +31,7 @@ type InvalidCommandTypeError struct { } func (e *InvalidCommandTypeError) Error() string { - return fmt.Sprintf("command %s type is invalid", e.commandId) + return fmt.Sprintf("command %s has invalid type", e.commandId) } // ReservedEnvError returns an error if the user attempts to customize a reserved ENV in a container diff --git a/pkg/validation/events.go b/pkg/validation/events.go index 93cb084b9..20d4668b5 100644 --- a/pkg/validation/events.go +++ b/pkg/validation/events.go @@ -17,34 +17,34 @@ const ( // ValidateEvents validates all the devfile events func ValidateEvents(events v1alpha2.Events, commands []v1alpha2.Command) error { - eventErrors := "" - commandMap := getCommandsMap(commands) + var eventErrorsList []string switch { case len(events.PreStart) > 0: if preStartErr := isEventValid(events.PreStart, preStart, commandMap); preStartErr != nil { - eventErrors += fmt.Sprintf("\n%s", preStartErr.Error()) + eventErrorsList = append(eventErrorsList, preStartErr.Error()) } fallthrough case len(events.PostStart) > 0: if postStartErr := isEventValid(events.PostStart, postStart, commandMap); postStartErr != nil { - eventErrors += fmt.Sprintf("\n%s", postStartErr.Error()) + eventErrorsList = append(eventErrorsList, postStartErr.Error()) } fallthrough case len(events.PreStop) > 0: if preStopErr := isEventValid(events.PreStop, preStop, commandMap); preStopErr != nil { - eventErrors += fmt.Sprintf("\n%s", preStopErr.Error()) + eventErrorsList = append(eventErrorsList, preStopErr.Error()) } fallthrough case len(events.PostStop) > 0: if postStopErr := isEventValid(events.PostStop, postStop, commandMap); postStopErr != nil { - eventErrors += fmt.Sprintf("\n%s", postStopErr.Error()) + eventErrorsList = append(eventErrorsList, postStopErr.Error()) } } // if there is any validation error, return it - if len(eventErrors) > 0 { + if len(eventErrorsList) > 0 { + eventErrors := fmt.Sprintf("\n%s", strings.Join(eventErrorsList, "\n")) return fmt.Errorf("devfile events validation error: %s", eventErrors) } @@ -83,22 +83,23 @@ func isEventValid(eventNames []string, eventType string, commandMap map[string]v } } - var eventErrors string var err error + var eventErrorsList []string if len(invalidCommand) > 0 { - eventErrors = fmt.Sprintf("\n%s does not map to a valid devfile command", strings.Join(invalidCommand, ", ")) + eventErrorsList = append(eventErrorsList, fmt.Sprintf("%s does not map to a valid devfile command", strings.Join(invalidCommand, ", "))) } if len(invalidApplyEvents) > 0 { - eventErrors += fmt.Sprintf("\n%s should either map to an apply command or a composite command with apply commands", strings.Join(invalidApplyEvents, ", ")) + eventErrorsList = append(eventErrorsList, fmt.Sprintf("%s should either map to an apply command or a composite command with apply commands", strings.Join(invalidApplyEvents, ", "))) } if len(invalidExecEvents) > 0 { - eventErrors += fmt.Sprintf("\n%s should either map to an exec command or a composite command with exec commands", strings.Join(invalidExecEvents, ", ")) + eventErrorsList = append(eventErrorsList, fmt.Sprintf("%s should either map to an exec command or a composite command with exec commands", strings.Join(invalidExecEvents, ", "))) } - if len(eventErrors) != 0 { + if len(eventErrorsList) != 0 { + eventErrors := fmt.Sprintf("\n%s", strings.Join(eventErrorsList, "\n")) err = &InvalidEventError{eventType: eventType, errorMsg: eventErrors} } diff --git a/pkg/validation/projects.go b/pkg/validation/projects.go index b520674cd..0a2bfc2a5 100644 --- a/pkg/validation/projects.go +++ b/pkg/validation/projects.go @@ -2,6 +2,7 @@ package validation import ( "fmt" + "strings" "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" ) @@ -10,7 +11,7 @@ import ( // and if the checkout remote matches the renote configured func ValidateStarterProjects(starterProjects []v1alpha2.StarterProject) error { - var errString string + var projectErrorsList []string for _, starterProject := range starterProjects { var gitSource v1alpha2.GitLikeProjectSource if starterProject.Git != nil { @@ -23,27 +24,28 @@ func ValidateStarterProjects(starterProjects []v1alpha2.StarterProject) error { switch len(gitSource.Remotes) { case 0: - starterProjectErr := fmt.Errorf("\nstarterProject %s should have at least one remote", starterProject.Name) + starterProjectErr := fmt.Errorf("starterProject %s should have at least one remote", starterProject.Name) newErr := resolveErrorMessageWithImportAttributes(starterProjectErr, starterProject.Attributes) - errString += newErr.Error() + projectErrorsList = append(projectErrorsList, newErr.Error()) case 1: if gitSource.CheckoutFrom != nil && gitSource.CheckoutFrom.Remote != "" { err := validateRemoteMap(gitSource.Remotes, gitSource.CheckoutFrom.Remote, starterProject.Name) if err != nil { newErr := resolveErrorMessageWithImportAttributes(err, starterProject.Attributes) - errString += fmt.Sprintf("\n%s", newErr.Error()) + projectErrorsList = append(projectErrorsList, newErr.Error()) } } default: // len(gitSource.Remotes) >= 2 - starterProjectErr := fmt.Errorf("\nstarterProject %s should have one remote only", starterProject.Name) + starterProjectErr := fmt.Errorf("starterProject %s should have one remote only", starterProject.Name) newErr := resolveErrorMessageWithImportAttributes(starterProjectErr, starterProject.Attributes) - errString += newErr.Error() + projectErrorsList = append(projectErrorsList, newErr.Error()) } } var err error - if len(errString) > 0 { - err = fmt.Errorf("error validating starter projects:%s", errString) + if len(projectErrorsList) > 0 { + projectErrors := fmt.Sprintf("\n%s", strings.Join(projectErrorsList, "\n")) + err = fmt.Errorf("error validating starter projects:%s", projectErrors) } return err @@ -53,7 +55,7 @@ func ValidateStarterProjects(starterProjects []v1alpha2.StarterProject) error { // remote is mandatory and if the checkout remote matches the renote configured func ValidateProjects(projects []v1alpha2.Project) error { - var errString string + var projectErrorsList []string for _, project := range projects { var gitSource v1alpha2.GitLikeProjectSource if project.Git != nil { @@ -63,36 +65,36 @@ func ValidateProjects(projects []v1alpha2.Project) error { } else { continue } - switch len(gitSource.Remotes) { case 0: - projectErr := fmt.Errorf("\nprojects %s should have at least one remote", project.Name) + projectErr := fmt.Errorf("projects %s should have at least one remote", project.Name) newErr := resolveErrorMessageWithImportAttributes(projectErr, project.Attributes) - errString += newErr.Error() + projectErrorsList = append(projectErrorsList, newErr.Error()) case 1: if gitSource.CheckoutFrom != nil && gitSource.CheckoutFrom.Remote != "" { if err := validateRemoteMap(gitSource.Remotes, gitSource.CheckoutFrom.Remote, project.Name); err != nil { newErr := resolveErrorMessageWithImportAttributes(err, project.Attributes) - errString += fmt.Sprintf("\n%s", newErr.Error()) + projectErrorsList = append(projectErrorsList, newErr.Error()) } } default: // len(gitSource.Remotes) >= 2 if gitSource.CheckoutFrom == nil || gitSource.CheckoutFrom.Remote == "" { - projectErr := fmt.Errorf("\nproject %s has more than one remote defined, but has no checkoutfrom remote defined", project.Name) + projectErr := fmt.Errorf("project %s has more than one remote defined, but has no checkoutfrom remote defined", project.Name) newErr := resolveErrorMessageWithImportAttributes(projectErr, project.Attributes) - errString += newErr.Error() + projectErrorsList = append(projectErrorsList, newErr.Error()) continue } if err := validateRemoteMap(gitSource.Remotes, gitSource.CheckoutFrom.Remote, project.Name); err != nil { newErr := resolveErrorMessageWithImportAttributes(err, project.Attributes) - errString += fmt.Sprintf("\n%s", newErr.Error()) + projectErrorsList = append(projectErrorsList, newErr.Error()) } } } var err error - if len(errString) > 0 { - err = fmt.Errorf("error validating projects:%s", errString) + if len(projectErrorsList) > 0 { + projectErrors := fmt.Sprintf("\n%s", strings.Join(projectErrorsList, "\n")) + err = fmt.Errorf("error validating projects:%s", projectErrors) } return err