Skip to content

Commit

Permalink
validator uses multierror.append to construct the returned error (#584)
Browse files Browse the repository at this point in the history 
* validator returns []error

Signed-off-by: Stephanie <[email protected]>

* fix go fmt

Signed-off-by: Stephanie <[email protected]>

* correct typo

Signed-off-by: Stephanie <[email protected]>

* use multierror instead of []error

Signed-off-by: Stephanie <[email protected]>
  • Loading branch information
@yangcao77
yangcao77 authored Aug 12, 2021
1 parent c025a14 commit ad569a8
Show file tree
Hide file tree
Showing 11 changed files with 215 additions and 150 deletions.
34 changes: 16 additions & 18 deletions pkg/validation/commands.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,27 +5,28 @@ import (
"strings"

"github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2"
"github.com/hashicorp/go-multierror"
)

// ValidateCommands validates the devfile commands and checks:
// 1. there are no duplicate command ids
// 2. the command type is not invalid
// 3. if a command is part of a command group, there is a single default command
func ValidateCommands(commands []v1alpha2.Command, components []v1alpha2.Component) (err error) {
func ValidateCommands(commands []v1alpha2.Command, components []v1alpha2.Component) (returnedErr error) {
groupKindCommandMap := make(map[v1alpha2.CommandGroupKind][]v1alpha2.Command)
commandMap := getCommandsMap(commands)

err = v1alpha2.CheckDuplicateKeys(commands)
err := v1alpha2.CheckDuplicateKeys(commands)
if err != nil {
return err
multierror.Append(returnedErr, err)
}

for _, command := range commands {
// parentCommands is a map to keep a track of all the parent commands when validating the composite command's subcommands recursively
parentCommands := make(map[string]string)
err = validateCommand(command, parentCommands, commandMap, components)
err := validateCommand(command, parentCommands, commandMap, components)
if err != nil {
return resolveErrorMessageWithImportAttributes(err, command.Attributes)
multierror.Append(returnedErr, resolveErrorMessageWithImportAttributes(err, command.Attributes))
}

commandGroup := getGroup(command)
Expand All @@ -34,19 +35,13 @@ func ValidateCommands(commands []v1alpha2.Command, components []v1alpha2.Compone
}
}

var groupErrorsList []string
for groupKind, commands := range groupKindCommandMap {
if err = validateGroup(commands); err != nil {
groupErrorsList = append(groupErrorsList, fmt.Sprintf("command group %s error - %s", groupKind, err.Error()))
if err := validateGroup(commands, groupKind); err != nil {
multierror.Append(returnedErr, err)
}
}

if len(groupErrorsList) > 0 {
groupErrors := strings.Join(groupErrorsList, "\n")
err = fmt.Errorf("\n%s", groupErrors)
}

return err
return returnedErr
}

// validateCommand validates a given devfile command where parentCommands is a map to track all the parent commands when validating
Expand All @@ -67,7 +62,7 @@ func validateCommand(command v1alpha2.Command, parentCommands map[string]string,
// validateGroup validates commands belonging to a specific group kind. If there are multiple commands belonging to the same group:
// 1. without any default, err out
// 2. with more than one default, err out
func validateGroup(commands []v1alpha2.Command) error {
func validateGroup(commands []v1alpha2.Command, groupKind v1alpha2.CommandGroupKind) error {
defaultCommandCount := 0
var defaultCommands []v1alpha2.Command
if len(commands) > 1 {
Expand All @@ -82,17 +77,20 @@ 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")
return &MissingDefaultCmdWarning{groupKind: groupKind}
} else if defaultCommandCount > 1 {
var commandsReferenceList []string
for _, command := range defaultCommands {
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;
// example: there should be exactly one default command, currently there are multiple commands;
// command: <id1>; command: <id2>, 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 &MultipleDefaultCmdError{
groupKind: groupKind,
commandsReference: commandsReference,
}
}

return nil
Expand Down
41 changes: 27 additions & 14 deletions pkg/validation/commands_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"testing"

"github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2"
"github.com/hashicorp/go-multierror"
"github.com/stretchr/testify/assert"
)

Expand Down Expand Up @@ -75,8 +76,9 @@ func TestValidateCommands(t *testing.T) {

duplicateKeyErr := "duplicate key: somecommand1"
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"
multipleDefaultCmdErr := ".*there should be exactly one default command, currently there are multiple default commands"
invalidCmdErr := ".*command does not map to a container component"
nonExistCmdInComposite := "the command .* mentioned in the composite command does not exist in the devfile"

parentOverridesFromMainDevfile := attributes.Attributes{}.PutString(ImportSourceAttribute,
"uri: http://127.0.0.1:8080").PutString(ParentOverrideAttribute, "main devfile")
Expand All @@ -85,7 +87,7 @@ func TestValidateCommands(t *testing.T) {
tests := []struct {
name string
commands []v1alpha2.Command
wantErr *string
wantErr []string
}{
{
name: "Valid Exec Command",
Expand All @@ -107,23 +109,23 @@ func TestValidateCommands(t *testing.T) {
generateDummyExecCommand("somecommand1", component, nil),
generateDummyExecCommand("somecommand1", component, nil),
},
wantErr: &duplicateKeyErr,
wantErr: []string{duplicateKeyErr},
},
{
name: "Duplicate commands, different types",
name: "Multiple errors: Duplicate commands, non-exist command in composite command",
commands: []v1alpha2.Command{
generateDummyExecCommand("somecommand1", component, nil),
generateDummyCompositeCommand("somecommand1", []string{"fakecommand"}, &v1alpha2.CommandGroup{Kind: buildGroup, IsDefault: true}),
},
wantErr: &duplicateKeyErr,
wantErr: []string{duplicateKeyErr, nonExistCmdInComposite},
},
{
name: "Different command types belonging to the same group but no default",
commands: []v1alpha2.Command{
generateDummyExecCommand("somecommand1", component, &v1alpha2.CommandGroup{Kind: buildGroup}),
generateDummyCompositeCommand("somecommand2", []string{"somecommand1"}, &v1alpha2.CommandGroup{Kind: buildGroup}),
},
wantErr: &noDefaultCmdErr,
wantErr: []string{noDefaultCmdErr},
},
{
name: "Different command types belonging to the same group with more than one default",
Expand All @@ -132,14 +134,14 @@ func TestValidateCommands(t *testing.T) {
generateDummyExecCommand("somecommand3", component, &v1alpha2.CommandGroup{Kind: buildGroup}),
generateDummyCompositeCommand("somecommand2", []string{"somecommand1"}, &v1alpha2.CommandGroup{Kind: buildGroup, IsDefault: true}),
},
wantErr: &multipleDefaultCmdErr,
wantErr: []string{multipleDefaultCmdErr},
},
{
name: "Invalid Apply command with wrong component",
commands: []v1alpha2.Command{
generateDummyApplyCommand("command", "invalidComponent", nil, attributes.Attributes{}),
},
wantErr: &invalidCmdErr,
wantErr: []string{invalidCmdErr},
},
{
name: "Valid Composite command with one subcommand referencing the other",
Expand All @@ -155,16 +157,20 @@ func TestValidateCommands(t *testing.T) {
commands: []v1alpha2.Command{
generateDummyApplyCommand("command", "invalidComponent", nil, parentOverridesFromMainDevfile),
},
wantErr: &invalidCmdErrWithImportAttributes,
wantErr: []string{invalidCmdErrWithImportAttributes},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
err := ValidateCommands(tt.commands, components)
if tt.wantErr != nil && assert.Error(t, err) {
assert.Regexp(t, *tt.wantErr, err.Error(), "Error message should match")

if merr, ok := err.(*multierror.Error); ok && tt.wantErr != nil {
assert.Equal(t, len(tt.wantErr), len(merr.Errors), "Error list length should match")
for i := 0; i < len(merr.Errors); i++ {
assert.Regexp(t, tt.wantErr[i], merr.Errors[i].Error(), "Error message should match")
}
} else {
assert.NoError(t, err, "Expected error to be nil")
assert.Equal(t, nil, err, "Error should be nil")
}
})
}
Expand Down Expand Up @@ -320,7 +326,7 @@ func TestValidateGroup(t *testing.T) {
component := "alias1"

noDefaultCmdErr := ".*there should be exactly one default command, currently there is no default command"
multipleDefaultError := ".*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 are multiple default commands"
multipleDefaultCmdErr := multipleDefaultError + "; command: run command; command: customcommand"

parentOverridesFromMainDevfile := attributes.Attributes{}.PutString(ImportSourceAttribute,
Expand All @@ -331,6 +337,7 @@ func TestValidateGroup(t *testing.T) {
tests := []struct {
name string
commands []v1alpha2.Command
group v1alpha2.CommandGroupKind
wantErr *string
}{
{
Expand All @@ -339,6 +346,7 @@ func TestValidateGroup(t *testing.T) {
generateDummyExecCommand("run command", component, &v1alpha2.CommandGroup{Kind: runGroup, IsDefault: true}),
generateDummyExecCommand("customcommand", component, &v1alpha2.CommandGroup{Kind: runGroup, IsDefault: true}),
},
group: runGroup,
wantErr: &multipleDefaultCmdErr,
},
{
Expand All @@ -347,6 +355,7 @@ func TestValidateGroup(t *testing.T) {
generateDummyExecCommand("run command", component, &v1alpha2.CommandGroup{Kind: runGroup, IsDefault: true}),
generateDummyApplyCommand("customcommand", component, &v1alpha2.CommandGroup{Kind: runGroup, IsDefault: true}, parentOverridesFromMainDevfile),
},
group: runGroup,
wantErr: &multipleDefaultCmdErrWithImportAttributes,
},
{
Expand All @@ -355,19 +364,22 @@ func TestValidateGroup(t *testing.T) {
generateDummyExecCommand("build command", component, &v1alpha2.CommandGroup{Kind: buildGroup}),
generateDummyExecCommand("build command 2", component, &v1alpha2.CommandGroup{Kind: buildGroup}),
},
group: buildGroup,
wantErr: &noDefaultCmdErr,
},
{
name: "One command does not need default",
commands: []v1alpha2.Command{
generateDummyExecCommand("test command", component, &v1alpha2.CommandGroup{Kind: buildGroup}),
},
group: buildGroup,
},
{
name: "One command can have default",
commands: []v1alpha2.Command{
generateDummyExecCommand("test command", component, &v1alpha2.CommandGroup{Kind: buildGroup, IsDefault: true}),
},
group: buildGroup,
},
{
name: "Composite commands in group",
Expand All @@ -376,11 +388,12 @@ func TestValidateGroup(t *testing.T) {
generateDummyExecCommand("build command 2", component, &v1alpha2.CommandGroup{Kind: buildGroup}),
generateDummyCompositeCommand("composite1", []string{"build command", "build command 2"}, &v1alpha2.CommandGroup{Kind: buildGroup, IsDefault: true}),
},
group: buildGroup,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
err := validateGroup(tt.commands)
err := validateGroup(tt.commands, tt.group)
if tt.wantErr != nil && assert.Error(t, err) {
assert.Regexp(t, *tt.wantErr, err.Error(), "Error message should match")
} else {
Expand Down
41 changes: 25 additions & 16 deletions pkg/validation/components.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"strings"

"github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2"
"github.com/hashicorp/go-multierror"
"k8s.io/apimachinery/pkg/api/resource"
)

Expand All @@ -21,7 +22,7 @@ const (
// 2. makes sure the volume components are unique
// 3. checks the URI specified in openshift components and kubernetes components are with valid format
// 4. makes sure the component name is unique
func ValidateComponents(components []v1alpha2.Component) error {
func ValidateComponents(components []v1alpha2.Component) (returnedErr error) {

processedVolumes := make(map[string]bool)
processedVolumeMounts := make(map[string][]string)
Expand All @@ -31,7 +32,7 @@ func ValidateComponents(components []v1alpha2.Component) error {

err := v1alpha2.CheckDuplicateKeys(components)
if err != nil {
return err
returnedErr = multierror.Append(returnedErr, err)
}

for _, component := range components {
Expand All @@ -47,15 +48,19 @@ func ValidateComponents(components []v1alpha2.Component) error {
// Check if any containers are customizing the reserved PROJECT_SOURCE or PROJECTS_ROOT env
for _, env := range component.Container.Env {
if env.Name == EnvProjectsSrc {
return &ReservedEnvError{envName: EnvProjectsSrc, componentName: component.Name}
reservedEnvErr := &ReservedEnvError{envName: EnvProjectsSrc, componentName: component.Name}
returnedErr = multierror.Append(returnedErr, reservedEnvErr)
} else if env.Name == EnvProjectsRoot {
return &ReservedEnvError{envName: EnvProjectsRoot, componentName: component.Name}
reservedEnvErr := &ReservedEnvError{envName: EnvProjectsRoot, componentName: component.Name}
returnedErr = multierror.Append(returnedErr, reservedEnvErr)
}
}

err := validateEndpoints(component.Container.Endpoints, processedEndPointPort, processedEndPointName)
if err != nil {
return resolveErrorMessageWithImportAttributes(err, component.Attributes)
if len(err) > 0 {
for _, endpointErr := range err {
returnedErr = multierror.Append(returnedErr, resolveErrorMessageWithImportAttributes(endpointErr, component.Attributes))
}
}
case component.Volume != nil:
processedVolumes[component.Name] = true
Expand All @@ -65,37 +70,41 @@ 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 resolveErrorMessageWithImportAttributes(invalidVolErr, component.Attributes)
returnedErr = multierror.Append(returnedErr, resolveErrorMessageWithImportAttributes(invalidVolErr, component.Attributes))
}
}
case component.Openshift != nil:
if component.Openshift.Uri != "" {
err := ValidateURI(component.Openshift.Uri)
if err != nil {
return resolveErrorMessageWithImportAttributes(err, component.Attributes)
returnedErr = multierror.Append(returnedErr, resolveErrorMessageWithImportAttributes(err, component.Attributes))
}
}

err := validateEndpoints(component.Openshift.Endpoints, processedEndPointPort, processedEndPointName)
if err != nil {
return resolveErrorMessageWithImportAttributes(err, component.Attributes)
if len(err) > 0 {
for _, endpointErr := range err {
returnedErr = multierror.Append(returnedErr, resolveErrorMessageWithImportAttributes(endpointErr, component.Attributes))
}
}
case component.Kubernetes != nil:
if component.Kubernetes.Uri != "" {
err := ValidateURI(component.Kubernetes.Uri)
if err != nil {
return resolveErrorMessageWithImportAttributes(err, component.Attributes)
returnedErr = multierror.Append(returnedErr, resolveErrorMessageWithImportAttributes(err, component.Attributes))
}
}
err := validateEndpoints(component.Kubernetes.Endpoints, processedEndPointPort, processedEndPointName)
if err != nil {
return resolveErrorMessageWithImportAttributes(err, component.Attributes)
if len(err) > 0 {
for _, endpointErr := range err {
returnedErr = multierror.Append(returnedErr, resolveErrorMessageWithImportAttributes(endpointErr, component.Attributes))
}
}
case component.Plugin != nil:
if component.Plugin.RegistryUrl != "" {
err := ValidateURI(component.Plugin.RegistryUrl)
if err != nil {
return resolveErrorMessageWithImportAttributes(err, component.Attributes)
returnedErr = multierror.Append(returnedErr, resolveErrorMessageWithImportAttributes(err, component.Attributes))
}
}
}
Expand All @@ -116,8 +125,8 @@ func ValidateComponents(components []v1alpha2.Component) error {

if len(invalidVolumeMountsErrList) > 0 {
invalidVolumeMountsErr := fmt.Sprintf("\n%s", strings.Join(invalidVolumeMountsErrList, "\n"))
return &MissingVolumeMountError{errMsg: invalidVolumeMountsErr}
returnedErr = multierror.Append(returnedErr, &MissingVolumeMountError{errMsg: invalidVolumeMountsErr})
}

return nil
return returnedErr
}
Loading

0 comments on commit ad569a8

Please sign in to comment.