From 7b08c3c26ee6696716af41eada777773db562e62 Mon Sep 17 00:00:00 2001 From: Maysun J Faisal Date: Mon, 1 Mar 2021 11:59:00 -0500 Subject: [PATCH 1/7] Add Delete Comp, Command, Proj, StarterProj Signed-off-by: Maysun J Faisal --- go.mod | 2 +- go.sum | 1 + pkg/devfile/parser/data/interface.go | 4 + pkg/devfile/parser/data/v2/commands.go | 29 ++++ pkg/devfile/parser/data/v2/commands_test.go | 145 ++++++++++++++++ pkg/devfile/parser/data/v2/components.go | 29 ++++ pkg/devfile/parser/data/v2/components_test.go | 156 ++++++++++++++++++ pkg/devfile/parser/data/v2/projects.go | 42 +++++ pkg/devfile/parser/data/v2/projects_test.go | 155 +++++++++++++++++ pkg/devfile/parser/data/v2/volumes.go | 26 +-- pkg/devfile/parser/data/v2/volumes_test.go | 27 +-- 11 files changed, 571 insertions(+), 45 deletions(-) diff --git a/go.mod b/go.mod index 8f010c86..1f7f9671 100644 --- a/go.mod +++ b/go.mod @@ -15,7 +15,7 @@ require ( github.com/openshift/api v0.0.0-20200930075302-db52bc4ef99f github.com/pkg/errors v0.9.1 github.com/spf13/afero v1.2.2 - github.com/stretchr/testify v1.6.1 // indirect + github.com/stretchr/testify v1.6.1 github.com/xeipuuv/gojsonschema v1.2.0 k8s.io/api v0.19.0 k8s.io/apimachinery v0.19.0 diff --git a/go.sum b/go.sum index 252c1324..a1d9a641 100644 --- a/go.sum +++ b/go.sum @@ -494,6 +494,7 @@ gopkg.in/yaml.v2 v2.3.0 h1:clyUAQHOM3G0M3f5vQj7LuJrETvjVot3Z5el9nffUtU= gopkg.in/yaml.v2 v2.3.0/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c h1:dUUwHk2QECo/6vqA44rthZ8ie2QXMNeKRTHCNY2nXvo= gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= +gotest.tools v2.2.0+incompatible h1:VsBPFP1AI068pPrMxtb/S8Zkgf9xEmTLJjfM+P5UIEo= gotest.tools v2.2.0+incompatible/go.mod h1:DsYFclhRJ6vuDpmuTbkuFWG+y2sxOXAzmJt81HFBacw= honnef.co/go/tools v0.0.0-20190102054323-c2f93a96b099/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4= honnef.co/go/tools v0.0.0-20190106161140-3f1c8253044a/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4= diff --git a/pkg/devfile/parser/data/interface.go b/pkg/devfile/parser/data/interface.go index 6e465b9f..7f62f03c 100644 --- a/pkg/devfile/parser/data/interface.go +++ b/pkg/devfile/parser/data/interface.go @@ -26,21 +26,25 @@ type DevfileData interface { GetComponents(common.DevfileOptions) ([]v1.Component, error) AddComponents(components []v1.Component) error UpdateComponent(component v1.Component) + DeleteComponent(name string) error // project related methods GetProjects(common.DevfileOptions) ([]v1.Project, error) AddProjects(projects []v1.Project) error UpdateProject(project v1.Project) + DeleteProject(name string) error // starter projects related commands GetStarterProjects(common.DevfileOptions) ([]v1.StarterProject, error) AddStarterProjects(projects []v1.StarterProject) error UpdateStarterProject(project v1.StarterProject) + DeleteStarterProject(name string) error // command related methods GetCommands(common.DevfileOptions) ([]v1.Command, error) AddCommands(commands ...v1.Command) error UpdateCommand(command v1.Command) + DeleteCommand(id string) error // volume related methods AddVolume(volume v1.Component, path string) error diff --git a/pkg/devfile/parser/data/v2/commands.go b/pkg/devfile/parser/data/v2/commands.go index d308713c..a74e9bd1 100644 --- a/pkg/devfile/parser/data/v2/commands.go +++ b/pkg/devfile/parser/data/v2/commands.go @@ -57,3 +57,32 @@ func (d *DevfileV2) UpdateCommand(command v1.Command) { } } } + +// DeleteCommand removes the specified command +func (d *DevfileV2) DeleteCommand(id string) error { + + found := false + for i := len(d.Commands) - 1; i >= 0; i-- { + if d.Commands[i].Composite != nil && d.Commands[i].Id != id { + var subCmd []string + for _, command := range d.Commands[i].Composite.Commands { + if command != id { + subCmd = append(subCmd, command) + } + } + d.Commands[i].Composite.Commands = subCmd + } else if d.Commands[i].Id == id { + found = true + d.Commands = append(d.Commands[:i], d.Commands[i+1:]...) + } + } + + if !found { + return &common.FieldNotFoundError{ + Field: "command", + Name: id, + } + } + + return nil +} diff --git a/pkg/devfile/parser/data/v2/commands_test.go b/pkg/devfile/parser/data/v2/commands_test.go index b003f3d9..c51db8f8 100644 --- a/pkg/devfile/parser/data/v2/commands_test.go +++ b/pkg/devfile/parser/data/v2/commands_test.go @@ -7,6 +7,7 @@ import ( v1 "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" "github.com/devfile/api/v2/pkg/attributes" "github.com/devfile/library/pkg/devfile/parser/data/v2/common" + "github.com/stretchr/testify/assert" ) func TestDevfile200_GetCommands(t *testing.T) { @@ -300,3 +301,147 @@ func TestDevfile200_UpdateCommands(t *testing.T) { }) } } + +func TestDeleteCommands(t *testing.T) { + + tests := []struct { + name string + commandToDelete string + commands []v1.Command + wantCommands []v1.Command + wantErr bool + }{ + { + name: "Commands that belong to Composite Command", + commandToDelete: "command1", + commands: []v1.Command{ + { + Id: "command1", + CommandUnion: v1.CommandUnion{ + Exec: &v1.ExecCommand{}, + }, + }, + { + Id: "command2", + CommandUnion: v1.CommandUnion{ + Exec: &v1.ExecCommand{}, + }, + }, + { + Id: "command3", + CommandUnion: v1.CommandUnion{ + Composite: &v1.CompositeCommand{ + Commands: []string{"command1", "command2", "command1"}, + }, + }, + }, + }, + wantCommands: []v1.Command{ + { + Id: "command2", + CommandUnion: v1.CommandUnion{ + Exec: &v1.ExecCommand{}, + }, + }, + { + Id: "command3", + CommandUnion: v1.CommandUnion{ + Composite: &v1.CompositeCommand{ + Commands: []string{"command2"}, + }, + }, + }, + }, + wantErr: false, + }, + { + name: "Commands that do not belong to Composite Command", + commandToDelete: "command3", + commands: []v1.Command{ + { + Id: "command1", + CommandUnion: v1.CommandUnion{ + Exec: &v1.ExecCommand{}, + }, + }, + { + Id: "command2", + CommandUnion: v1.CommandUnion{ + Composite: &v1.CompositeCommand{ + Commands: []string{"command1"}, + }, + }, + }, + { + Id: "command3", + CommandUnion: v1.CommandUnion{ + Composite: &v1.CompositeCommand{ + Commands: []string{"command1"}, + }, + }, + }, + }, + wantCommands: []v1.Command{ + { + Id: "command1", + CommandUnion: v1.CommandUnion{ + Exec: &v1.ExecCommand{}, + }, + }, + { + Id: "command2", + CommandUnion: v1.CommandUnion{ + Composite: &v1.CompositeCommand{ + Commands: []string{"command1"}, + }, + }, + }, + }, + wantErr: false, + }, + { + name: "Missing Command", + commandToDelete: "command34", + commands: []v1.Command{ + { + Id: "command1", + CommandUnion: v1.CommandUnion{ + Exec: &v1.ExecCommand{}, + }, + }, + }, + wantCommands: []v1.Command{ + { + Id: "command1", + CommandUnion: v1.CommandUnion{ + Exec: &v1.ExecCommand{}, + }, + }, + }, + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + d := &DevfileV2{ + v1.Devfile{ + DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ + DevWorkspaceTemplateSpecContent: v1.DevWorkspaceTemplateSpecContent{ + Commands: tt.commands, + }, + }, + }, + } + + err := d.DeleteCommand(tt.commandToDelete) + if tt.wantErr && err == nil { + t.Errorf("Expected error from test but got nil") + } else if !tt.wantErr && err != nil { + t.Errorf("Got unexpected error: %s", err) + } else if err == nil { + assert.Equal(t, tt.wantCommands, d.Commands, "The two values should be the same.") + } + }) + } + +} diff --git a/pkg/devfile/parser/data/v2/components.go b/pkg/devfile/parser/data/v2/components.go index 2da15b73..287df1c1 100644 --- a/pkg/devfile/parser/data/v2/components.go +++ b/pkg/devfile/parser/data/v2/components.go @@ -88,3 +88,32 @@ func (d *DevfileV2) UpdateComponent(component v1.Component) { d.Components[index] = component } } + +// DeleteComponent removes the specified component +func (d *DevfileV2) DeleteComponent(name string) error { + + found := false + for i := len(d.Components) - 1; i >= 0; i-- { + if d.Components[i].Container != nil && d.Components[i].Name != name { + var tmp []v1.VolumeMount + for _, volumeMount := range d.Components[i].Container.VolumeMounts { + if volumeMount.Name != name { + tmp = append(tmp, volumeMount) + } + } + d.Components[i].Container.VolumeMounts = tmp + } else if d.Components[i].Name == name { + found = true + d.Components = append(d.Components[:i], d.Components[i+1:]...) + } + } + + if !found { + return &common.FieldNotFoundError{ + Field: "component", + Name: name, + } + } + + return nil +} diff --git a/pkg/devfile/parser/data/v2/components_test.go b/pkg/devfile/parser/data/v2/components_test.go index e625c83d..d30e24e6 100644 --- a/pkg/devfile/parser/data/v2/components_test.go +++ b/pkg/devfile/parser/data/v2/components_test.go @@ -8,6 +8,7 @@ import ( "github.com/devfile/api/v2/pkg/attributes" "github.com/devfile/library/pkg/devfile/parser/data/v2/common" "github.com/devfile/library/pkg/testingutil" + "github.com/stretchr/testify/assert" ) func TestDevfile200_AddComponent(t *testing.T) { @@ -403,3 +404,158 @@ func TestGetDevfileVolumeComponents(t *testing.T) { } } + +func TestDeleteComponents(t *testing.T) { + + tests := []struct { + name string + componentToDelete string + components []v1.Component + wantComponents []v1.Component + wantErr bool + }{ + { + name: "Volume Component with mounts", + componentToDelete: "comp3", + components: []v1.Component{ + { + Name: "comp2", + ComponentUnion: v1.ComponentUnion{ + Container: &v1.ContainerComponent{ + Container: v1.Container{ + VolumeMounts: []v1.VolumeMount{ + testingutil.GetFakeVolumeMount("comp2", "/path"), + testingutil.GetFakeVolumeMount("comp2", "/path2"), + testingutil.GetFakeVolumeMount("comp3", "/path"), + }, + }, + }, + }, + }, + { + Name: "comp2", + ComponentUnion: v1.ComponentUnion{ + Volume: &v1.VolumeComponent{}, + }, + }, + { + Name: "comp3", + ComponentUnion: v1.ComponentUnion{ + Volume: &v1.VolumeComponent{}, + }, + }, + }, + wantComponents: []v1.Component{ + { + Name: "comp2", + ComponentUnion: v1.ComponentUnion{ + Container: &v1.ContainerComponent{ + Container: v1.Container{ + VolumeMounts: []v1.VolumeMount{ + testingutil.GetFakeVolumeMount("comp2", "/path"), + testingutil.GetFakeVolumeMount("comp2", "/path2"), + }, + }, + }, + }, + }, + { + Name: "comp2", + ComponentUnion: v1.ComponentUnion{ + Volume: &v1.VolumeComponent{}, + }, + }, + }, + wantErr: false, + }, + { + name: "Non Volume Component", + componentToDelete: "comp1", + components: []v1.Component{ + { + Name: "comp1", + ComponentUnion: v1.ComponentUnion{ + Container: &v1.ContainerComponent{ + Container: v1.Container{ + VolumeMounts: []v1.VolumeMount{ + testingutil.GetFakeVolumeMount("comp2", "/path"), + }, + }, + }, + }, + }, + { + Name: "comp2", + ComponentUnion: v1.ComponentUnion{ + Volume: &v1.VolumeComponent{}, + }, + }, + { + Name: "comp3", + ComponentUnion: v1.ComponentUnion{ + Kubernetes: &v1.KubernetesComponent{}, + }, + }, + }, + wantComponents: []v1.Component{ + { + Name: "comp2", + ComponentUnion: v1.ComponentUnion{ + Volume: &v1.VolumeComponent{}, + }, + }, + { + Name: "comp3", + ComponentUnion: v1.ComponentUnion{ + Kubernetes: &v1.KubernetesComponent{}, + }, + }, + }, + wantErr: false, + }, + { + name: "Missing Component", + componentToDelete: "comp12", + components: []v1.Component{ + { + Name: "comp2", + ComponentUnion: v1.ComponentUnion{ + Volume: &v1.VolumeComponent{}, + }, + }, + }, + wantComponents: []v1.Component{ + { + Name: "comp2", + ComponentUnion: v1.ComponentUnion{ + Volume: &v1.VolumeComponent{}, + }, + }, + }, + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + d := &DevfileV2{ + v1.Devfile{ + DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ + DevWorkspaceTemplateSpecContent: v1.DevWorkspaceTemplateSpecContent{ + Components: tt.components, + }, + }, + }, + } + + err := d.DeleteComponent(tt.componentToDelete) + if tt.wantErr && err == nil { + t.Errorf("Expected error from test but got nil") + } else if !tt.wantErr && err != nil { + t.Errorf("Got unexpected error: %s", err) + } else if err == nil { + assert.Equal(t, tt.wantComponents, d.Components, "The two values should be the same.") + } + }) + } + +} diff --git a/pkg/devfile/parser/data/v2/projects.go b/pkg/devfile/parser/data/v2/projects.go index 01bee4de..e8cff3ba 100644 --- a/pkg/devfile/parser/data/v2/projects.go +++ b/pkg/devfile/parser/data/v2/projects.go @@ -55,6 +55,27 @@ func (d *DevfileV2) UpdateProject(project v1.Project) { } } +// DeleteProject removes the specified project +func (d *DevfileV2) DeleteProject(name string) error { + + found := false + for i := len(d.Projects) - 1; i >= 0; i-- { + if d.Projects[i].Name == name { + found = true + d.Projects = append(d.Projects[:i], d.Projects[i+1:]...) + } + } + + if !found { + return &common.FieldNotFoundError{ + Field: "project", + Name: name, + } + } + + return nil +} + //GetStarterProjects returns the DevfileStarterProject parsed from devfile func (d *DevfileV2) GetStarterProjects(options common.DevfileOptions) ([]v1.StarterProject, error) { if len(options.Filter) == 0 { @@ -102,3 +123,24 @@ func (d *DevfileV2) UpdateStarterProject(project v1.StarterProject) { } } } + +// DeleteStarterProject removes the specified starter project +func (d *DevfileV2) DeleteStarterProject(name string) error { + + found := false + for i := len(d.StarterProjects) - 1; i >= 0; i-- { + if d.StarterProjects[i].Name == name { + found = true + d.StarterProjects = append(d.StarterProjects[:i], d.StarterProjects[i+1:]...) + } + } + + if !found { + return &common.FieldNotFoundError{ + Field: "starter project", + Name: name, + } + } + + return nil +} diff --git a/pkg/devfile/parser/data/v2/projects_test.go b/pkg/devfile/parser/data/v2/projects_test.go index ee232b13..cc5d167c 100644 --- a/pkg/devfile/parser/data/v2/projects_test.go +++ b/pkg/devfile/parser/data/v2/projects_test.go @@ -6,6 +6,7 @@ import ( v1 "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" "github.com/kylelemons/godebug/pretty" + "github.com/stretchr/testify/assert" ) func TestDevfile200_AddProjects(t *testing.T) { @@ -188,6 +189,83 @@ func TestDevfile200_UpdateProject(t *testing.T) { } } +func TestDevfile200_DeleteProject(t *testing.T) { + + tests := []struct { + name string + projectToDelete string + projects []v1.Project + wantProjects []v1.Project + wantErr bool + }{ + { + name: "Project successfully deleted", + projectToDelete: "nodejs", + projects: []v1.Project{ + { + Name: "nodejs", + ClonePath: "/project", + }, + { + Name: "java", + ClonePath: "/project2", + }, + }, + wantProjects: []v1.Project{ + { + Name: "java", + ClonePath: "/project2", + }, + }, + wantErr: false, + }, + { + name: "Project not found", + projectToDelete: "nodejs1", + projects: []v1.Project{ + { + Name: "nodejs", + ClonePath: "/project", + }, + { + Name: "java", + ClonePath: "/project2", + }, + }, + wantProjects: []v1.Project{ + { + Name: "java", + ClonePath: "/project2", + }, + }, + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + d := &DevfileV2{ + v1.Devfile{ + DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ + DevWorkspaceTemplateSpecContent: v1.DevWorkspaceTemplateSpecContent{ + Projects: tt.projects, + }, + }, + }, + } + + err := d.DeleteProject(tt.projectToDelete) + if tt.wantErr && err == nil { + t.Errorf("Expected error from test but got nil") + } else if !tt.wantErr && err != nil { + t.Errorf("Got unexpected error: %s", err) + } else if err == nil { + assert.Equal(t, tt.wantProjects, d.Projects, "The two values should be the same.") + } + }) + } + +} + func TestDevfile200_AddStarterProjects(t *testing.T) { currentProject := []v1.StarterProject{ { @@ -370,3 +448,80 @@ func TestDevfile200_UpdateStarterProject(t *testing.T) { }) } } + +func TestDevfile200_DeleteStarterProject(t *testing.T) { + + tests := []struct { + name string + starterProjectToDelete string + starterProjects []v1.StarterProject + wantStarterProjects []v1.StarterProject + wantErr bool + }{ + { + name: "Starter Project successfully deleted", + starterProjectToDelete: "nodejs", + starterProjects: []v1.StarterProject{ + { + Name: "nodejs", + SubDir: "/project", + }, + { + Name: "java", + SubDir: "/project2", + }, + }, + wantStarterProjects: []v1.StarterProject{ + { + Name: "java", + SubDir: "/project2", + }, + }, + wantErr: false, + }, + { + name: "Starter Project not found", + starterProjectToDelete: "nodejs1", + starterProjects: []v1.StarterProject{ + { + Name: "nodejs", + SubDir: "/project", + }, + { + Name: "java", + SubDir: "/project2", + }, + }, + wantStarterProjects: []v1.StarterProject{ + { + Name: "java", + SubDir: "/project2", + }, + }, + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + d := &DevfileV2{ + v1.Devfile{ + DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ + DevWorkspaceTemplateSpecContent: v1.DevWorkspaceTemplateSpecContent{ + StarterProjects: tt.starterProjects, + }, + }, + }, + } + + err := d.DeleteStarterProject(tt.starterProjectToDelete) + if tt.wantErr && err == nil { + t.Errorf("Expected error from test but got nil") + } else if !tt.wantErr && err != nil { + t.Errorf("Got unexpected error: %s", err) + } else if err == nil { + assert.Equal(t, tt.wantStarterProjects, d.StarterProjects, "The two values should be the same.") + } + }) + } + +} diff --git a/pkg/devfile/parser/data/v2/volumes.go b/pkg/devfile/parser/data/v2/volumes.go index 2a338006..cb864f16 100644 --- a/pkg/devfile/parser/data/v2/volumes.go +++ b/pkg/devfile/parser/data/v2/volumes.go @@ -48,32 +48,8 @@ func (d *DevfileV2) AddVolume(volumeComponent v1.Component, path string) error { // DeleteVolume removes the volume from the devFile and removes all the related volume mounts func (d *DevfileV2) DeleteVolume(name string) error { - found := false - for i := len(d.Components) - 1; i >= 0; i-- { - if d.Components[i].Container != nil { - var tmp []v1.VolumeMount - for _, volumeMount := range d.Components[i].Container.VolumeMounts { - if volumeMount.Name != name { - tmp = append(tmp, volumeMount) - } - } - d.Components[i].Container.VolumeMounts = tmp - } else if d.Components[i].Volume != nil { - if d.Components[i].Name == name { - found = true - d.Components = append(d.Components[:i], d.Components[i+1:]...) - } - } - } - if !found { - return &common.FieldNotFoundError{ - Field: "volume", - Name: name, - } - } - - return nil + return d.DeleteComponent(name) } // GetVolumeMountPath gets the mount path of the required volume diff --git a/pkg/devfile/parser/data/v2/volumes_test.go b/pkg/devfile/parser/data/v2/volumes_test.go index 54b86fd4..e3105f65 100644 --- a/pkg/devfile/parser/data/v2/volumes_test.go +++ b/pkg/devfile/parser/data/v2/volumes_test.go @@ -201,14 +201,11 @@ func TestDevfile200_DeleteVolume(t *testing.T) { volume0 := "volume0" volume1 := "volume1" - type args struct { - name string - } tests := []struct { name string currentComponents []v1.Component wantComponents []v1.Component - args args + cmpName string wantErr bool }{ { @@ -267,9 +264,7 @@ func TestDevfile200_DeleteVolume(t *testing.T) { }, }, }, - args: args{ - name: volume0, - }, + cmpName: volume0, wantErr: false, }, { @@ -334,9 +329,7 @@ func TestDevfile200_DeleteVolume(t *testing.T) { }, testingutil.GetFakeVolumeComponent(volume1, "5Gi"), }, - args: args{ - name: volume0, - }, + cmpName: volume0, wantErr: false, }, { @@ -358,10 +351,8 @@ func TestDevfile200_DeleteVolume(t *testing.T) { testingutil.GetFakeVolumeComponent(volume1, "5Gi"), }, wantComponents: []v1.Component{}, - args: args{ - name: volume0, - }, - wantErr: true, + cmpName: volume0, + wantErr: true, }, { name: "case 4: volume is present but not mounted to any component", @@ -369,10 +360,8 @@ func TestDevfile200_DeleteVolume(t *testing.T) { testingutil.GetFakeVolumeComponent(volume0, "5Gi"), }, wantComponents: []v1.Component{}, - args: args{ - name: volume0, - }, - wantErr: false, + cmpName: volume0, + wantErr: false, }, } for _, tt := range tests { @@ -386,7 +375,7 @@ func TestDevfile200_DeleteVolume(t *testing.T) { }, }, } - err := d.DeleteVolume(tt.args.name) + err := d.DeleteVolume(tt.cmpName) if (err != nil) != tt.wantErr { t.Errorf("DeleteVolume() error = %v, wantErr %v", err, tt.wantErr) } From 7cb2dcdb57a021ccc48fb418cf7a690f1038e6b8 Mon Sep 17 00:00:00 2001 From: Maysun J Faisal Date: Mon, 1 Mar 2021 16:29:01 -0500 Subject: [PATCH 2/7] Cleanup of interface funcs Signed-off-by: Maysun J Faisal --- pkg/devfile/parser/data/interface.go | 2 +- pkg/devfile/parser/data/v2/commands.go | 8 +- pkg/devfile/parser/data/v2/commands_test.go | 2 +- pkg/devfile/parser/data/v2/components.go | 14 +- tests/v2/utils/command_test_utils.go | 270 ++++++++++++++++++++ 5 files changed, 279 insertions(+), 17 deletions(-) create mode 100644 tests/v2/utils/command_test_utils.go diff --git a/pkg/devfile/parser/data/interface.go b/pkg/devfile/parser/data/interface.go index 7f62f03c..eb0f72a8 100644 --- a/pkg/devfile/parser/data/interface.go +++ b/pkg/devfile/parser/data/interface.go @@ -42,7 +42,7 @@ type DevfileData interface { // command related methods GetCommands(common.DevfileOptions) ([]v1.Command, error) - AddCommands(commands ...v1.Command) error + AddCommands(commands []v1.Command) error UpdateCommand(command v1.Command) DeleteCommand(id string) error diff --git a/pkg/devfile/parser/data/v2/commands.go b/pkg/devfile/parser/data/v2/commands.go index a74e9bd1..b430ef34 100644 --- a/pkg/devfile/parser/data/v2/commands.go +++ b/pkg/devfile/parser/data/v2/commands.go @@ -31,14 +31,10 @@ func (d *DevfileV2) GetCommands(options common.DevfileOptions) ([]v1.Command, er // AddCommands adds the slice of Command objects to the Devfile's commands // if a command is already defined, error out -func (d *DevfileV2) AddCommands(commands ...v1.Command) error { - devfileCommands, err := d.GetCommands(common.DevfileOptions{}) - if err != nil { - return err - } +func (d *DevfileV2) AddCommands(commands []v1.Command) error { for _, command := range commands { - for _, devfileCommand := range devfileCommands { + for _, devfileCommand := range d.Commands { if command.Id == devfileCommand.Id { return &common.FieldAlreadyExistError{Name: command.Id, Field: "command"} } diff --git a/pkg/devfile/parser/data/v2/commands_test.go b/pkg/devfile/parser/data/v2/commands_test.go index c51db8f8..476a787f 100644 --- a/pkg/devfile/parser/data/v2/commands_test.go +++ b/pkg/devfile/parser/data/v2/commands_test.go @@ -217,7 +217,7 @@ func TestDevfile200_AddCommands(t *testing.T) { }, } - got := d.AddCommands(tt.newCommands...) + got := d.AddCommands(tt.newCommands) if !tt.wantErr && got != nil { t.Errorf("TestDevfile200_AddCommands() unexpected error - %v", got) } else if tt.wantErr && got == nil { diff --git a/pkg/devfile/parser/data/v2/components.go b/pkg/devfile/parser/data/v2/components.go index 287df1c1..7d9526e9 100644 --- a/pkg/devfile/parser/data/v2/components.go +++ b/pkg/devfile/parser/data/v2/components.go @@ -60,17 +60,13 @@ func (d *DevfileV2) GetDevfileVolumeComponents(options common.DevfileOptions) ([ // if a component is already defined, error out func (d *DevfileV2) AddComponents(components []v1.Component) error { - componentMap := make(map[string]bool) - - for _, component := range d.Components { - componentMap[component.Name] = true - } for _, component := range components { - if _, ok := componentMap[component.Name]; !ok { - d.Components = append(d.Components, component) - } else { - return &common.FieldAlreadyExistError{Name: component.Name, Field: "component"} + for _, devfileComponent := range d.Components { + if component.Name == devfileComponent.Name { + return &common.FieldAlreadyExistError{Name: component.Name, Field: "component"} + } } + d.Components = append(d.Components, component) } return nil } diff --git a/tests/v2/utils/command_test_utils.go b/tests/v2/utils/command_test_utils.go new file mode 100644 index 00000000..27bfdcae --- /dev/null +++ b/tests/v2/utils/command_test_utils.go @@ -0,0 +1,270 @@ +package utils + +import ( + "errors" + "fmt" + "io/ioutil" + + "github.com/google/go-cmp/cmp" + "sigs.k8s.io/yaml" + + schema "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" +) + +// commandAdded adds a new command to the test schema data and to the parser data +func (devfile *TestDevfile) commandAdded(command schema.Command) { + LogInfoMessage(fmt.Sprintf("command added Id: %s", command.Id)) + devfile.SchemaDevFile.Commands = append(devfile.SchemaDevFile.Commands, command) + devfile.ParserData.AddCommands([]schema.Command{command}) +} + +// commandUpdated updates a command in the parser data +func (devfile *TestDevfile) commandUpdated(command schema.Command) { + LogInfoMessage(fmt.Sprintf("command updated Id: %s", command.Id)) + devfile.ParserData.UpdateCommand(command) +} + +// addEnv creates and returns a specifed number of env attributes in a schema structure +func addEnv(numEnv int) []schema.EnvVar { + commandEnvs := make([]schema.EnvVar, numEnv) + for i := 0; i < numEnv; i++ { + commandEnvs[i].Name = "Name_" + GetRandomString(5, false) + commandEnvs[i].Value = "Value_" + GetRandomString(5, false) + LogInfoMessage(fmt.Sprintf("Add Env: %s", commandEnvs[i])) + } + return commandEnvs +} + +// addAttributes creates returns a specifed number of attributes in a schema structure +func addAttributes(numAtrributes int) map[string]string { + attributes := make(map[string]string) + for i := 0; i < numAtrributes; i++ { + AttributeName := "Name_" + GetRandomString(6, false) + attributes[AttributeName] = "Value_" + GetRandomString(6, false) + LogInfoMessage(fmt.Sprintf("Add attribute : %s = %s", AttributeName, attributes[AttributeName])) + } + return attributes +} + +// addGroup creates and returns a group in a schema structure +func (devfile *TestDevfile) addGroup() *schema.CommandGroup { + + commandGroup := schema.CommandGroup{} + commandGroup.Kind = GetRandomGroupKind() + LogInfoMessage(fmt.Sprintf("group Kind: %s, default already set %t", commandGroup.Kind, devfile.GroupDefaults[commandGroup.Kind])) + // Ensure only one and at least one of each type are labelled as default + if !devfile.GroupDefaults[commandGroup.Kind] { + devfile.GroupDefaults[commandGroup.Kind] = true + commandGroup.IsDefault = true + } else { + commandGroup.IsDefault = false + } + LogInfoMessage(fmt.Sprintf("group isDefault: %t", commandGroup.IsDefault)) + return &commandGroup +} + +// AddCommand creates a command of a specified type in a schema structure and pupulates it with random attributes +func (devfile *TestDevfile) AddCommand(commandType schema.CommandType) schema.Command { + + var command *schema.Command + if commandType == schema.ExecCommandType { + command = devfile.createExecCommand() + devfile.setExecCommandValues(command) + // command must be mentioned by a container component + command.Exec.Component = devfile.GetContainerName() + } else if commandType == schema.CompositeCommandType { + command = devfile.createCompositeCommand() + devfile.setCompositeCommandValues(command) + } + return *command +} + +// UpdateCommand randomly updates attribute values of a specified command in the devfile schema +func (devfile *TestDevfile) UpdateCommand(commandId string) error { + + var err error + testCommand, found := getSchemaCommand(devfile.SchemaDevFile.Commands, commandId) + if found { + LogInfoMessage(fmt.Sprintf("Updating command id: %s", commandId)) + if testCommand.Exec != nil { + devfile.setExecCommandValues(testCommand) + } else if testCommand.Composite != nil { + devfile.setCompositeCommandValues(testCommand) + } + } else { + err = errors.New(LogErrorMessage(fmt.Sprintf("Command not found in test : %s", commandId))) + } + return err +} + +// createExecCommand creates and returns an empty exec command in a schema structure +func (devfile *TestDevfile) createExecCommand() *schema.Command { + + LogInfoMessage("Create an exec command :") + command := schema.Command{} + command.Id = GetRandomUniqueString(8, true) + LogInfoMessage(fmt.Sprintf("command Id: %s", command.Id)) + command.Exec = &schema.ExecCommand{} + devfile.commandAdded(command) + return &command + +} + +// setExecCommandValues randomly sets exec command attribute to random values +func (devfile *TestDevfile) setExecCommandValues(command *schema.Command) { + + execCommand := command.Exec + execCommand.CommandLine = GetRandomString(4, false) + " " + GetRandomString(4, false) + LogInfoMessage(fmt.Sprintf("....... commandLine: %s", execCommand.CommandLine)) + + // If group already leave it to make sure defaults are not deleted or added + if execCommand.Group == nil { + if GetRandomDecision(2, 1) { + execCommand.Group = devfile.addGroup() + } + } + + if GetBinaryDecision() { + execCommand.Label = GetRandomString(12, false) + LogInfoMessage(fmt.Sprintf("....... label: %s", execCommand.Label)) + } else { + execCommand.Label = "" + } + + if GetBinaryDecision() { + execCommand.WorkingDir = "./tmp" + LogInfoMessage(fmt.Sprintf("....... WorkingDir: %s", execCommand.WorkingDir)) + } else { + execCommand.WorkingDir = "" + } + + execCommand.HotReloadCapable = GetBinaryDecision() + LogInfoMessage(fmt.Sprintf("....... HotReloadCapable: %t", execCommand.HotReloadCapable)) + + if GetBinaryDecision() { + execCommand.Env = addEnv(GetRandomNumber(4)) + } else { + execCommand.Env = nil + } + devfile.commandUpdated(*command) + +} + +// getSchemaCommand get a specified command from the devfile schema structure +func getSchemaCommand(commands []schema.Command, id string) (*schema.Command, bool) { + found := false + var schemaCommand schema.Command + for _, command := range commands { + if command.Id == id { + schemaCommand = command + found = true + break + } + } + return &schemaCommand, found +} + +// createCompositeCommand creates an empty composite command in a schema structure +func (devfile *TestDevfile) createCompositeCommand() *schema.Command { + + LogInfoMessage("Create a composite command :") + command := schema.Command{} + command.Id = GetRandomUniqueString(8, true) + LogInfoMessage(fmt.Sprintf("command Id: %s", command.Id)) + command.Composite = &schema.CompositeCommand{} + devfile.commandAdded(command) + + return &command +} + +// setCompositeCommandValues randomly sets composite command attribute to random values +func (devfile *TestDevfile) setCompositeCommandValues(command *schema.Command) { + + compositeCommand := command.Composite + numCommands := GetRandomNumber(3) + + for i := 0; i < numCommands; i++ { + execCommand := devfile.AddCommand(schema.ExecCommandType) + compositeCommand.Commands = append(compositeCommand.Commands, execCommand.Id) + LogInfoMessage(fmt.Sprintf("....... command %d of %d : %s", i, numCommands, execCommand.Id)) + } + + // If group already exists - leave it to make sure defaults are not deleted or added + if compositeCommand.Group == nil { + if GetRandomDecision(2, 1) { + compositeCommand.Group = devfile.addGroup() + } + } + + if GetBinaryDecision() { + compositeCommand.Label = GetRandomString(12, false) + LogInfoMessage(fmt.Sprintf("....... label: %s", compositeCommand.Label)) + } + + if GetBinaryDecision() { + compositeCommand.Parallel = true + LogInfoMessage(fmt.Sprintf("....... Parallel: %t", compositeCommand.Parallel)) + } + + devfile.commandUpdated(*command) +} + +// VerifyCommands verifies commands returned by the parser are the same as those saved in the devfile schema +func (devfile *TestDevfile) VerifyCommands(parserCommands []schema.Command) error { + + LogInfoMessage("Enter VerifyCommands") + var errorString []string + + // Compare entire array of commands + if !cmp.Equal(parserCommands, devfile.SchemaDevFile.Commands) { + errorString = append(errorString, LogErrorMessage(fmt.Sprintf("Command array compare failed."))) + // Array compare failed. Narrow down by comparing indivdual commands + for _, command := range parserCommands { + if testCommand, found := getSchemaCommand(devfile.SchemaDevFile.Commands, command.Id); found { + if !cmp.Equal(command, *testCommand) { + parserFilename := AddSuffixToFileName(devfile.FileName, "_"+command.Id+"_Parser") + testFilename := AddSuffixToFileName(devfile.FileName, "_"+command.Id+"_Test") + LogInfoMessage(fmt.Sprintf(".......marshall and write devfile %s", devfile.FileName)) + c, err := yaml.Marshal(command) + if err != nil { + errorString = append(errorString, LogErrorMessage(fmt.Sprintf(".......marshall devfile %s", parserFilename))) + } else { + err = ioutil.WriteFile(parserFilename, c, 0644) + if err != nil { + errorString = append(errorString, LogErrorMessage(fmt.Sprintf(".......write devfile %s", parserFilename))) + } + } + LogInfoMessage(fmt.Sprintf(".......marshall and write devfile %s", testFilename)) + c, err = yaml.Marshal(testCommand) + if err != nil { + errorString = append(errorString, LogErrorMessage(fmt.Sprintf(".......marshall devfile %s", testFilename))) + } else { + err = ioutil.WriteFile(testFilename, c, 0644) + if err != nil { + errorString = append(errorString, LogErrorMessage(fmt.Sprintf(".......write devfile %s", testFilename))) + } + } + errorString = append(errorString, LogInfoMessage(fmt.Sprintf("Command %s did not match, see files : %s and %s", command.Id, parserFilename, testFilename))) + } else { + LogInfoMessage(fmt.Sprintf(" --> Command matched : %s", command.Id)) + } + } else { + errorString = append(errorString, LogErrorMessage(fmt.Sprintf("Command from parser not known to test - id : %s ", command.Id))) + } + + } + for _, command := range devfile.SchemaDevFile.Commands { + if _, found := getSchemaCommand(parserCommands, command.Id); !found { + errorString = append(errorString, LogErrorMessage(fmt.Sprintf("Command from test not returned by parser : %s ", command.Id))) + } + } + } else { + LogInfoMessage(fmt.Sprintf(" --> Command structures matched")) + } + + var err error + if len(errorString) > 0 { + err = errors.New(fmt.Sprint(errorString)) + } + return err +} From ec243b10bdc91fee818543a83c0839678f8a88a4 Mon Sep 17 00:00:00 2001 From: Maysun J Faisal Date: Wed, 3 Mar 2021 15:45:56 -0500 Subject: [PATCH 3/7] Update volume mount funcs and delete funcs Signed-off-by: Maysun J Faisal --- pkg/devfile/parser/data/interface.go | 8 +- pkg/devfile/parser/data/v2/commands.go | 10 +- pkg/devfile/parser/data/v2/commands_test.go | 47 +-- pkg/devfile/parser/data/v2/components.go | 10 +- pkg/devfile/parser/data/v2/components_test.go | 46 +-- pkg/devfile/parser/data/v2/volumes.go | 85 +++-- pkg/devfile/parser/data/v2/volumes_test.go | 327 ++++++------------ 7 files changed, 170 insertions(+), 363 deletions(-) diff --git a/pkg/devfile/parser/data/interface.go b/pkg/devfile/parser/data/interface.go index eb0f72a8..250d8ecc 100644 --- a/pkg/devfile/parser/data/interface.go +++ b/pkg/devfile/parser/data/interface.go @@ -46,10 +46,10 @@ type DevfileData interface { UpdateCommand(command v1.Command) DeleteCommand(id string) error - // volume related methods - AddVolume(volume v1.Component, path string) error - DeleteVolume(name string) error - GetVolumeMountPath(name string) (string, error) + // volume mount related methods + AddVolumeMount(componentName, name, path string) error + DeleteVolumeMount(name string) error + GetVolumeMountPath(mountName, componentName string) (string, error) // workspace related methods GetDevfileWorkspace() *v1.DevWorkspaceTemplateSpecContent diff --git a/pkg/devfile/parser/data/v2/commands.go b/pkg/devfile/parser/data/v2/commands.go index b430ef34..8db7630d 100644 --- a/pkg/devfile/parser/data/v2/commands.go +++ b/pkg/devfile/parser/data/v2/commands.go @@ -59,15 +59,7 @@ func (d *DevfileV2) DeleteCommand(id string) error { found := false for i := len(d.Commands) - 1; i >= 0; i-- { - if d.Commands[i].Composite != nil && d.Commands[i].Id != id { - var subCmd []string - for _, command := range d.Commands[i].Composite.Commands { - if command != id { - subCmd = append(subCmd, command) - } - } - d.Commands[i].Composite.Commands = subCmd - } else if d.Commands[i].Id == id { + if d.Commands[i].Id == id { found = true d.Commands = append(d.Commands[:i], d.Commands[i+1:]...) } diff --git a/pkg/devfile/parser/data/v2/commands_test.go b/pkg/devfile/parser/data/v2/commands_test.go index 476a787f..bc23be27 100644 --- a/pkg/devfile/parser/data/v2/commands_test.go +++ b/pkg/devfile/parser/data/v2/commands_test.go @@ -347,52 +347,7 @@ func TestDeleteCommands(t *testing.T) { Id: "command3", CommandUnion: v1.CommandUnion{ Composite: &v1.CompositeCommand{ - Commands: []string{"command2"}, - }, - }, - }, - }, - wantErr: false, - }, - { - name: "Commands that do not belong to Composite Command", - commandToDelete: "command3", - commands: []v1.Command{ - { - Id: "command1", - CommandUnion: v1.CommandUnion{ - Exec: &v1.ExecCommand{}, - }, - }, - { - Id: "command2", - CommandUnion: v1.CommandUnion{ - Composite: &v1.CompositeCommand{ - Commands: []string{"command1"}, - }, - }, - }, - { - Id: "command3", - CommandUnion: v1.CommandUnion{ - Composite: &v1.CompositeCommand{ - Commands: []string{"command1"}, - }, - }, - }, - }, - wantCommands: []v1.Command{ - { - Id: "command1", - CommandUnion: v1.CommandUnion{ - Exec: &v1.ExecCommand{}, - }, - }, - { - Id: "command2", - CommandUnion: v1.CommandUnion{ - Composite: &v1.CompositeCommand{ - Commands: []string{"command1"}, + Commands: []string{"command1", "command2", "command1"}, }, }, }, diff --git a/pkg/devfile/parser/data/v2/components.go b/pkg/devfile/parser/data/v2/components.go index 7d9526e9..75c20d72 100644 --- a/pkg/devfile/parser/data/v2/components.go +++ b/pkg/devfile/parser/data/v2/components.go @@ -90,15 +90,7 @@ func (d *DevfileV2) DeleteComponent(name string) error { found := false for i := len(d.Components) - 1; i >= 0; i-- { - if d.Components[i].Container != nil && d.Components[i].Name != name { - var tmp []v1.VolumeMount - for _, volumeMount := range d.Components[i].Container.VolumeMounts { - if volumeMount.Name != name { - tmp = append(tmp, volumeMount) - } - } - d.Components[i].Container.VolumeMounts = tmp - } else if d.Components[i].Name == name { + if d.Components[i].Name == name { found = true d.Components = append(d.Components[:i], d.Components[i+1:]...) } diff --git a/pkg/devfile/parser/data/v2/components_test.go b/pkg/devfile/parser/data/v2/components_test.go index d30e24e6..f69f1390 100644 --- a/pkg/devfile/parser/data/v2/components_test.go +++ b/pkg/devfile/parser/data/v2/components_test.go @@ -454,6 +454,7 @@ func TestDeleteComponents(t *testing.T) { VolumeMounts: []v1.VolumeMount{ testingutil.GetFakeVolumeMount("comp2", "/path"), testingutil.GetFakeVolumeMount("comp2", "/path2"), + testingutil.GetFakeVolumeMount("comp3", "/path"), }, }, }, @@ -468,51 +469,6 @@ func TestDeleteComponents(t *testing.T) { }, wantErr: false, }, - { - name: "Non Volume Component", - componentToDelete: "comp1", - components: []v1.Component{ - { - Name: "comp1", - ComponentUnion: v1.ComponentUnion{ - Container: &v1.ContainerComponent{ - Container: v1.Container{ - VolumeMounts: []v1.VolumeMount{ - testingutil.GetFakeVolumeMount("comp2", "/path"), - }, - }, - }, - }, - }, - { - Name: "comp2", - ComponentUnion: v1.ComponentUnion{ - Volume: &v1.VolumeComponent{}, - }, - }, - { - Name: "comp3", - ComponentUnion: v1.ComponentUnion{ - Kubernetes: &v1.KubernetesComponent{}, - }, - }, - }, - wantComponents: []v1.Component{ - { - Name: "comp2", - ComponentUnion: v1.ComponentUnion{ - Volume: &v1.VolumeComponent{}, - }, - }, - { - Name: "comp3", - ComponentUnion: v1.ComponentUnion{ - Kubernetes: &v1.KubernetesComponent{}, - }, - }, - }, - wantErr: false, - }, { name: "Missing Component", componentToDelete: "comp12", diff --git a/pkg/devfile/parser/data/v2/volumes.go b/pkg/devfile/parser/data/v2/volumes.go index cb864f16..53a93232 100644 --- a/pkg/devfile/parser/data/v2/volumes.go +++ b/pkg/devfile/parser/data/v2/volumes.go @@ -8,75 +8,90 @@ import ( "github.com/devfile/library/pkg/devfile/parser/data/v2/common" ) -// AddVolume adds the volume to the devFile and mounts it to all the container components -func (d *DevfileV2) AddVolume(volumeComponent v1.Component, path string) error { - volumeExists := false +// AddVolumeMount adds the volume mount to the specified container component +func (d *DevfileV2) AddVolumeMount(componentName, name, path string) error { var pathErrorContainers []string + found := false for _, component := range d.Components { - if component.Container != nil { + if component.Container != nil && component.Name == componentName { + found = true for _, volumeMount := range component.Container.VolumeMounts { if volumeMount.Path == path { - var err = fmt.Errorf("another volume, %s, is mounted to the same path: %s, on the container: %s", volumeMount.Name, path, component.Name) + var err = fmt.Errorf("another volume, %s, is mounted to the same path: %s, in the container: %s", volumeMount.Name, path, component.Name) pathErrorContainers = append(pathErrorContainers, err.Error()) } } component.Container.VolumeMounts = append(component.Container.VolumeMounts, v1.VolumeMount{ - Name: volumeComponent.Name, + Name: name, Path: path, }) - } else if component.Volume != nil && component.Name == volumeComponent.Name { - volumeExists = true - break } } - if volumeExists { - return &common.FieldAlreadyExistError{ - Field: "volume", - Name: volumeComponent.Name, + if !found { + return &common.FieldNotFoundError{ + Field: "container component", + Name: componentName, } } if len(pathErrorContainers) > 0 { - return fmt.Errorf("errors while creating volume:\n%s", strings.Join(pathErrorContainers, "\n")) + return fmt.Errorf("errors while adding volume mounts:\n%s", strings.Join(pathErrorContainers, "\n")) } - d.Components = append(d.Components, volumeComponent) - return nil } -// DeleteVolume removes the volume from the devFile and removes all the related volume mounts -func (d *DevfileV2) DeleteVolume(name string) error { +// DeleteVolumeMount deletes the volume mount from container components +func (d *DevfileV2) DeleteVolumeMount(name string) error { + found := false + for i := range d.Components { + if d.Components[i].Container != nil && d.Components[i].Name != name { + for j := len(d.Components[i].Container.VolumeMounts) - 1; j >= 0; j-- { + if d.Components[i].Container.VolumeMounts[j].Name == name { + found = true + d.Components[i].Container.VolumeMounts = append(d.Components[i].Container.VolumeMounts[:j], d.Components[i].Container.VolumeMounts[j+1:]...) + } + } + } + } - return d.DeleteComponent(name) + if !found { + return &common.FieldNotFoundError{ + Field: "volume mount", + Name: name, + } + } + + return nil } -// GetVolumeMountPath gets the mount path of the required volume -func (d *DevfileV2) GetVolumeMountPath(name string) (string, error) { - volumeFound := false +// GetVolumeMountPath gets the mount path of the specified volume mount from the specified container component +func (d *DevfileV2) GetVolumeMountPath(mountName, componentName string) (string, error) { mountFound := false - path := "" + componentFound := false + var path string for _, component := range d.Components { - if component.Container != nil { + if component.Container != nil && component.Name == componentName { + componentFound = true for _, volumeMount := range component.Container.VolumeMounts { - if volumeMount.Name == name { + if volumeMount.Name == mountName { mountFound = true path = volumeMount.Path } } - } else if component.Volume != nil { - volumeFound = true } } - if volumeFound && mountFound { - return path, nil - } else if !mountFound && volumeFound { - return "", fmt.Errorf("volume not mounted to any component") - } - return "", &common.FieldNotFoundError{ - Field: "volume", - Name: "name", + + if !componentFound { + return "", &common.FieldNotFoundError{ + Field: "container component", + Name: componentName, + } + } else if !mountFound { + return "", fmt.Errorf("volume %s not mounted to component %s", mountName, componentName) } + + return path, nil } diff --git a/pkg/devfile/parser/data/v2/volumes_test.go b/pkg/devfile/parser/data/v2/volumes_test.go index e3105f65..22e5c3d1 100644 --- a/pkg/devfile/parser/data/v2/volumes_test.go +++ b/pkg/devfile/parser/data/v2/volumes_test.go @@ -1,27 +1,26 @@ package v2 import ( - "reflect" "testing" v1 "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" "github.com/devfile/library/pkg/testingutil" - "github.com/kylelemons/godebug/pretty" + "github.com/stretchr/testify/assert" ) -func TestDevfile200_AddVolume(t *testing.T) { +func TestDevfile200_AddVolumeMount(t *testing.T) { image0 := "some-image-0" - container0 := "container0" - image1 := "some-image-1" + container0 := "container0" container1 := "container1" volume0 := "volume0" volume1 := "volume1" type args struct { - volumeComponent v1.Component - path string + componentName string + name string + path string } tests := []struct { name string @@ -31,7 +30,7 @@ func TestDevfile200_AddVolume(t *testing.T) { wantErr bool }{ { - name: "case 1: it should add the volume to all the containers", + name: "add the volume mount when other mounts are present", currentComponents: []v1.Component{ { Name: container0, @@ -39,6 +38,9 @@ func TestDevfile200_AddVolume(t *testing.T) { Container: &v1.ContainerComponent{ Container: v1.Container{ Image: image0, + VolumeMounts: []v1.VolumeMount{ + testingutil.GetFakeVolumeMount(volume1, "/data"), + }, }, }, }, @@ -48,15 +50,19 @@ func TestDevfile200_AddVolume(t *testing.T) { ComponentUnion: v1.ComponentUnion{ Container: &v1.ContainerComponent{ Container: v1.Container{ - Image: image1, + Image: image0, + VolumeMounts: []v1.VolumeMount{ + testingutil.GetFakeVolumeMount(volume1, "/data"), + }, }, }, }, }, }, args: args{ - volumeComponent: testingutil.GetFakeVolumeComponent(volume0, "5Gi"), - path: "/path", + name: volume0, + path: "/path0", + componentName: container0, }, wantComponents: []v1.Component{ { @@ -66,7 +72,8 @@ func TestDevfile200_AddVolume(t *testing.T) { Container: v1.Container{ Image: image0, VolumeMounts: []v1.VolumeMount{ - testingutil.GetFakeVolumeMount(volume0, "/path"), + testingutil.GetFakeVolumeMount(volume1, "/data"), + testingutil.GetFakeVolumeMount(volume0, "/path0"), }, }, }, @@ -77,19 +84,18 @@ func TestDevfile200_AddVolume(t *testing.T) { ComponentUnion: v1.ComponentUnion{ Container: &v1.ContainerComponent{ Container: v1.Container{ - Image: image1, + Image: image0, VolumeMounts: []v1.VolumeMount{ - testingutil.GetFakeVolumeMount(volume0, "/path"), + testingutil.GetFakeVolumeMount(volume1, "/data"), }, }, }, }, }, - testingutil.GetFakeVolumeComponent(volume0, "5Gi"), }, }, { - name: "case 2: it should add the volume when other volumes are present", + name: "error out when same path is present in the container", currentComponents: []v1.Component{ { Name: container0, @@ -106,40 +112,14 @@ func TestDevfile200_AddVolume(t *testing.T) { }, }, args: args{ - volumeComponent: testingutil.GetFakeVolumeComponent(volume0, "5Gi"), - path: "/path", - }, - wantComponents: []v1.Component{ - { - Name: container0, - ComponentUnion: v1.ComponentUnion{ - Container: &v1.ContainerComponent{ - Container: v1.Container{ - Image: image0, - VolumeMounts: []v1.VolumeMount{ - testingutil.GetFakeVolumeMount(volume1, "/data"), - testingutil.GetFakeVolumeMount(volume0, "/path"), - }, - }, - }, - }, - }, - testingutil.GetFakeVolumeComponent(volume0, "5Gi"), - }, - }, - { - name: "case 3: error out when same volume is present", - currentComponents: []v1.Component{ - testingutil.GetFakeVolumeComponent(volume0, "1Gi"), - }, - args: args{ - volumeComponent: testingutil.GetFakeVolumeComponent(volume0, "5Gi"), - path: "/path", + name: volume0, + path: "/data", + componentName: container0, }, wantErr: true, }, { - name: "case 4: it should error out when another volume is mounted to the same path", + name: "error out when the specified container is not found", currentComponents: []v1.Component{ { Name: container0, @@ -148,17 +128,17 @@ func TestDevfile200_AddVolume(t *testing.T) { Container: v1.Container{ Image: image0, VolumeMounts: []v1.VolumeMount{ - testingutil.GetFakeVolumeMount(volume1, "/path"), + testingutil.GetFakeVolumeMount(volume1, "/data"), }, }, }, }, }, - testingutil.GetFakeVolumeComponent(volume1, "5Gi"), }, args: args{ - volumeComponent: testingutil.GetFakeVolumeComponent(volume0, "5Gi"), - path: "/path", + name: volume0, + path: "/data", + componentName: container1, }, wantErr: true, }, @@ -175,193 +155,116 @@ func TestDevfile200_AddVolume(t *testing.T) { }, } - err := d.AddVolume(tt.args.volumeComponent, tt.args.path) - if (err != nil) != tt.wantErr { - t.Errorf("AddVolume() error = %v, wantErr %v", err, tt.wantErr) - } - - if err != nil && tt.wantErr { - return - } - - if !reflect.DeepEqual(d.Components, tt.wantComponents) { - t.Errorf("wanted: %v, got: %v, difference at %v", tt.wantComponents, d.Components, pretty.Compare(tt.wantComponents, d.Components)) + err := d.AddVolumeMount(tt.args.componentName, tt.args.name, tt.args.path) + if tt.wantErr && err == nil { + t.Errorf("Expected error from test but got nil") + } else if !tt.wantErr && err != nil { + t.Errorf("Got unexpected error: %s", err) + } else if err == nil { + assert.Equal(t, tt.wantComponents, d.Components, "The two values should be the same.") } }) } } -func TestDevfile200_DeleteVolume(t *testing.T) { - image0 := "some-image-0" - container0 := "container0" - - image1 := "some-image-1" - container1 := "container1" - - volume0 := "volume0" - volume1 := "volume1" +func TestDevfile200_DeleteVolumeMounts(t *testing.T) { tests := []struct { - name string - currentComponents []v1.Component - wantComponents []v1.Component - cmpName string - wantErr bool + name string + volMountToDelete string + components []v1.Component + wantComponents []v1.Component + wantErr bool }{ { - name: "case 1: volume is present and mounted to multiple components", - currentComponents: []v1.Component{ + name: "Volume Component with mounts", + volMountToDelete: "comp2", + components: []v1.Component{ { - Name: container0, + Name: "comp1", ComponentUnion: v1.ComponentUnion{ Container: &v1.ContainerComponent{ Container: v1.Container{ - Image: image0, VolumeMounts: []v1.VolumeMount{ - testingutil.GetFakeVolumeMount(volume0, "/path"), + testingutil.GetFakeVolumeMount("comp2", "/path"), + testingutil.GetFakeVolumeMount("comp2", "/path2"), + testingutil.GetFakeVolumeMount("comp3", "/path"), }, }, }, }, }, { - Name: container1, + Name: "comp4", ComponentUnion: v1.ComponentUnion{ Container: &v1.ContainerComponent{ Container: v1.Container{ - Image: image1, VolumeMounts: []v1.VolumeMount{ - { - Name: volume0, - Path: "/path", - }, + testingutil.GetFakeVolumeMount("comp2", "/path"), }, }, }, }, }, - testingutil.GetFakeVolumeComponent(volume0, "5Gi"), }, wantComponents: []v1.Component{ { - Name: container0, + Name: "comp1", ComponentUnion: v1.ComponentUnion{ Container: &v1.ContainerComponent{ Container: v1.Container{ - Image: image0, + VolumeMounts: []v1.VolumeMount{ + testingutil.GetFakeVolumeMount("comp3", "/path"), + }, }, }, }, }, { - Name: container1, + Name: "comp4", ComponentUnion: v1.ComponentUnion{ Container: &v1.ContainerComponent{ Container: v1.Container{ - Image: image1, + VolumeMounts: []v1.VolumeMount{}, }, }, }, }, }, - cmpName: volume0, wantErr: false, }, { - name: "case 2: delete only the required volume in case of multiples", - currentComponents: []v1.Component{ - { - Name: container0, - ComponentUnion: v1.ComponentUnion{ - Container: &v1.ContainerComponent{ - Container: v1.Container{ - Image: image0, - VolumeMounts: []v1.VolumeMount{ - testingutil.GetFakeVolumeMount(volume0, "/path"), - testingutil.GetFakeVolumeMount(volume1, "/data"), - }, - }, - }, - }, - }, + name: "Missing mount name", + volMountToDelete: "comp1", + components: []v1.Component{ { - Name: container1, + Name: "comp4", ComponentUnion: v1.ComponentUnion{ Container: &v1.ContainerComponent{ Container: v1.Container{ - Image: image1, VolumeMounts: []v1.VolumeMount{ - testingutil.GetFakeVolumeMount(volume1, "/data"), + testingutil.GetFakeVolumeMount("comp2", "/path"), }, }, }, }, }, - testingutil.GetFakeVolumeComponent(volume0, "5Gi"), - testingutil.GetFakeVolumeComponent(volume1, "5Gi"), }, wantComponents: []v1.Component{ { - Name: container0, - ComponentUnion: v1.ComponentUnion{ - Container: &v1.ContainerComponent{ - Container: v1.Container{ - Image: image0, - VolumeMounts: []v1.VolumeMount{ - testingutil.GetFakeVolumeMount(volume1, "/data"), - }, - }, - }, - }, - }, - { - Name: container1, - ComponentUnion: v1.ComponentUnion{ - Container: &v1.ContainerComponent{ - Container: v1.Container{ - Image: image1, - VolumeMounts: []v1.VolumeMount{ - testingutil.GetFakeVolumeMount(volume1, "/data"), - }, - }, - }, - }, - }, - testingutil.GetFakeVolumeComponent(volume1, "5Gi"), - }, - cmpName: volume0, - wantErr: false, - }, - { - name: "case 3: volume is not present", - currentComponents: []v1.Component{ - { - Name: container0, + Name: "comp4", ComponentUnion: v1.ComponentUnion{ Container: &v1.ContainerComponent{ Container: v1.Container{ - Image: image0, VolumeMounts: []v1.VolumeMount{ - testingutil.GetFakeVolumeMount(volume1, "/data"), + testingutil.GetFakeVolumeMount("comp2", "/path"), }, }, }, }, }, - testingutil.GetFakeVolumeComponent(volume1, "5Gi"), }, - wantComponents: []v1.Component{}, - cmpName: volume0, - wantErr: true, - }, - { - name: "case 4: volume is present but not mounted to any component", - currentComponents: []v1.Component{ - testingutil.GetFakeVolumeComponent(volume0, "5Gi"), - }, - wantComponents: []v1.Component{}, - cmpName: volume0, - wantErr: false, + wantErr: true, }, } for _, tt := range tests { @@ -370,42 +273,39 @@ func TestDevfile200_DeleteVolume(t *testing.T) { v1.Devfile{ DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ DevWorkspaceTemplateSpecContent: v1.DevWorkspaceTemplateSpecContent{ - Components: tt.currentComponents, + Components: tt.components, }, }, }, } - err := d.DeleteVolume(tt.cmpName) - if (err != nil) != tt.wantErr { - t.Errorf("DeleteVolume() error = %v, wantErr %v", err, tt.wantErr) - } - if err != nil && tt.wantErr { - return - } - - if !reflect.DeepEqual(d.Components, tt.wantComponents) { - t.Errorf("wanted: %v, got: %v, difference at %v", tt.wantComponents, d.Components, pretty.Compare(tt.wantComponents, d.Components)) + err := d.DeleteVolumeMount(tt.volMountToDelete) + if tt.wantErr && err == nil { + t.Errorf("Expected error from test but got nil") + } else if !tt.wantErr && err != nil { + t.Errorf("Got unexpected error: %s", err) + } else if err == nil { + assert.Equal(t, tt.wantComponents, d.Components, "The two values should be the same.") } }) } + } func TestDevfile200_GetVolumeMountPath(t *testing.T) { volume1 := "volume1" + component1 := "component1" - type args struct { - name string - } tests := []struct { name string currentComponents []v1.Component + mountName string + componentName string wantPath string - args args wantErr bool }{ { - name: "case 1: volume is present and mounted on a component", + name: "vol is mounted on the specified container component", currentComponents: []v1.Component{ { ComponentUnion: v1.ComponentUnion{ @@ -417,17 +317,16 @@ func TestDevfile200_GetVolumeMountPath(t *testing.T) { }, }, }, + Name: component1, }, - testingutil.GetFakeVolumeComponent(volume1, "5Gi"), - }, - wantPath: "/path", - args: args{ - name: volume1, }, - wantErr: false, + wantPath: "/path", + mountName: volume1, + componentName: component1, + wantErr: false, }, { - name: "case 2: volume is not present but mounted on a component", + name: "vol is not mounted on the specified container component", currentComponents: []v1.Component{ { ComponentUnion: v1.ComponentUnion{ @@ -439,30 +338,32 @@ func TestDevfile200_GetVolumeMountPath(t *testing.T) { }, }, }, + Name: component1, }, }, - args: args{ - name: volume1, - }, - wantErr: true, - }, - { - name: "case 3: volume is not present and not mounted on a component", - currentComponents: []v1.Component{}, - args: args{ - name: volume1, - }, - wantErr: true, + mountName: "volume2", + componentName: component1, + wantErr: true, }, { - name: "case 4: volume is present but not mounted", + name: "invalid specified container", currentComponents: []v1.Component{ - testingutil.GetFakeVolumeComponent(volume1, "5Gi"), - }, - args: args{ - name: volume1, + { + ComponentUnion: v1.ComponentUnion{ + Container: &v1.ContainerComponent{ + Container: v1.Container{ + VolumeMounts: []v1.VolumeMount{ + testingutil.GetFakeVolumeMount(volume1, "/path"), + }, + }, + }, + }, + Name: component1, + }, }, - wantErr: true, + mountName: volume1, + componentName: "component2", + wantErr: true, }, } for _, tt := range tests { @@ -476,17 +377,13 @@ func TestDevfile200_GetVolumeMountPath(t *testing.T) { }, }, } - got, err := d.GetVolumeMountPath(tt.args.name) - if (err != nil) != tt.wantErr { - t.Errorf("GetVolumeMountPath() error = %v, wantErr %v", err, tt.wantErr) - } - - if err != nil && tt.wantErr { - return - } - - if !reflect.DeepEqual(got, tt.wantPath) { - t.Errorf("wanted: %v, got: %v, difference at %v", tt.wantPath, got, pretty.Compare(tt.wantPath, got)) + gotPath, err := d.GetVolumeMountPath(tt.mountName, tt.componentName) + if tt.wantErr && err == nil { + t.Errorf("Expected error from test but got nil") + } else if !tt.wantErr && err != nil { + t.Errorf("Got unexpected error: %s", err) + } else if err == nil { + assert.Equal(t, tt.wantPath, gotPath, "The two values should be the same.") } }) } From da1250997a58dccf3c5decc8c317d98ea4de5f83 Mon Sep 17 00:00:00 2001 From: Maysun J Faisal Date: Fri, 5 Mar 2021 16:00:17 -0500 Subject: [PATCH 4/7] address PR feedback Signed-off-by: Maysun J Faisal --- pkg/devfile/parser/data/interface.go | 2 +- pkg/devfile/parser/data/v2/commands.go | 3 +- pkg/devfile/parser/data/v2/commands_test.go | 92 ++++------ pkg/devfile/parser/data/v2/components.go | 3 +- pkg/devfile/parser/data/v2/components_test.go | 104 +++++------- pkg/devfile/parser/data/v2/projects.go | 6 +- pkg/devfile/parser/data/v2/projects_test.go | 132 ++++++--------- pkg/devfile/parser/data/v2/volumes.go | 20 +-- pkg/devfile/parser/data/v2/volumes_test.go | 159 ++++++++---------- 9 files changed, 214 insertions(+), 307 deletions(-) diff --git a/pkg/devfile/parser/data/interface.go b/pkg/devfile/parser/data/interface.go index 250d8ecc..5883d8f6 100644 --- a/pkg/devfile/parser/data/interface.go +++ b/pkg/devfile/parser/data/interface.go @@ -47,7 +47,7 @@ type DevfileData interface { DeleteCommand(id string) error // volume mount related methods - AddVolumeMount(componentName, name, path string) error + AddVolumeMount(componentName string, volumeMounts []v1.VolumeMount) error DeleteVolumeMount(name string) error GetVolumeMountPath(mountName, componentName string) (string, error) diff --git a/pkg/devfile/parser/data/v2/commands.go b/pkg/devfile/parser/data/v2/commands.go index 8db7630d..998b1caa 100644 --- a/pkg/devfile/parser/data/v2/commands.go +++ b/pkg/devfile/parser/data/v2/commands.go @@ -58,10 +58,11 @@ func (d *DevfileV2) UpdateCommand(command v1.Command) { func (d *DevfileV2) DeleteCommand(id string) error { found := false - for i := len(d.Commands) - 1; i >= 0; i-- { + for i := range d.Commands { if d.Commands[i].Id == id { found = true d.Commands = append(d.Commands[:i], d.Commands[i+1:]...) + break } } diff --git a/pkg/devfile/parser/data/v2/commands_test.go b/pkg/devfile/parser/data/v2/commands_test.go index bc23be27..b2d39502 100644 --- a/pkg/devfile/parser/data/v2/commands_test.go +++ b/pkg/devfile/parser/data/v2/commands_test.go @@ -304,38 +304,46 @@ func TestDevfile200_UpdateCommands(t *testing.T) { func TestDeleteCommands(t *testing.T) { + d := &DevfileV2{ + v1.Devfile{ + DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ + DevWorkspaceTemplateSpecContent: v1.DevWorkspaceTemplateSpecContent{ + Commands: []v1.Command{ + { + Id: "command1", + CommandUnion: v1.CommandUnion{ + Exec: &v1.ExecCommand{}, + }, + }, + { + Id: "command2", + CommandUnion: v1.CommandUnion{ + Exec: &v1.ExecCommand{}, + }, + }, + { + Id: "command3", + CommandUnion: v1.CommandUnion{ + Composite: &v1.CompositeCommand{ + Commands: []string{"command1", "command2", "command1"}, + }, + }, + }, + }, + }, + }, + }, + } + tests := []struct { name string commandToDelete string - commands []v1.Command wantCommands []v1.Command wantErr bool }{ { - name: "Commands that belong to Composite Command", + name: "Successfully delete command", commandToDelete: "command1", - commands: []v1.Command{ - { - Id: "command1", - CommandUnion: v1.CommandUnion{ - Exec: &v1.ExecCommand{}, - }, - }, - { - Id: "command2", - CommandUnion: v1.CommandUnion{ - Exec: &v1.ExecCommand{}, - }, - }, - { - Id: "command3", - CommandUnion: v1.CommandUnion{ - Composite: &v1.CompositeCommand{ - Commands: []string{"command1", "command2", "command1"}, - }, - }, - }, - }, wantCommands: []v1.Command{ { Id: "command2", @@ -357,43 +365,17 @@ func TestDeleteCommands(t *testing.T) { { name: "Missing Command", commandToDelete: "command34", - commands: []v1.Command{ - { - Id: "command1", - CommandUnion: v1.CommandUnion{ - Exec: &v1.ExecCommand{}, - }, - }, - }, - wantCommands: []v1.Command{ - { - Id: "command1", - CommandUnion: v1.CommandUnion{ - Exec: &v1.ExecCommand{}, - }, - }, - }, - wantErr: true, + wantErr: true, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - d := &DevfileV2{ - v1.Devfile{ - DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ - DevWorkspaceTemplateSpecContent: v1.DevWorkspaceTemplateSpecContent{ - Commands: tt.commands, - }, - }, - }, + err := d.DeleteCommand(tt.commandToDelete) + if (err != nil) != tt.wantErr { + t.Errorf("DeleteCommand() error = %v, wantErr %v", err, tt.wantErr) } - err := d.DeleteCommand(tt.commandToDelete) - if tt.wantErr && err == nil { - t.Errorf("Expected error from test but got nil") - } else if !tt.wantErr && err != nil { - t.Errorf("Got unexpected error: %s", err) - } else if err == nil { + if err == nil && !tt.wantErr { assert.Equal(t, tt.wantCommands, d.Commands, "The two values should be the same.") } }) diff --git a/pkg/devfile/parser/data/v2/components.go b/pkg/devfile/parser/data/v2/components.go index 75c20d72..92a30aba 100644 --- a/pkg/devfile/parser/data/v2/components.go +++ b/pkg/devfile/parser/data/v2/components.go @@ -89,10 +89,11 @@ func (d *DevfileV2) UpdateComponent(component v1.Component) { func (d *DevfileV2) DeleteComponent(name string) error { found := false - for i := len(d.Components) - 1; i >= 0; i-- { + for i := range d.Components { if d.Components[i].Name == name { found = true d.Components = append(d.Components[:i], d.Components[i+1:]...) + break } } diff --git a/pkg/devfile/parser/data/v2/components_test.go b/pkg/devfile/parser/data/v2/components_test.go index f69f1390..929c9f97 100644 --- a/pkg/devfile/parser/data/v2/components_test.go +++ b/pkg/devfile/parser/data/v2/components_test.go @@ -407,44 +407,52 @@ func TestGetDevfileVolumeComponents(t *testing.T) { func TestDeleteComponents(t *testing.T) { + d := &DevfileV2{ + v1.Devfile{ + DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ + DevWorkspaceTemplateSpecContent: v1.DevWorkspaceTemplateSpecContent{ + Components: []v1.Component{ + { + Name: "comp2", + ComponentUnion: v1.ComponentUnion{ + Container: &v1.ContainerComponent{ + Container: v1.Container{ + VolumeMounts: []v1.VolumeMount{ + testingutil.GetFakeVolumeMount("comp2", "/path"), + testingutil.GetFakeVolumeMount("comp2", "/path2"), + testingutil.GetFakeVolumeMount("comp3", "/path"), + }, + }, + }, + }, + }, + { + Name: "comp2", + ComponentUnion: v1.ComponentUnion{ + Volume: &v1.VolumeComponent{}, + }, + }, + { + Name: "comp3", + ComponentUnion: v1.ComponentUnion{ + Volume: &v1.VolumeComponent{}, + }, + }, + }, + }, + }, + }, + } + tests := []struct { name string componentToDelete string - components []v1.Component wantComponents []v1.Component wantErr bool }{ { - name: "Volume Component with mounts", + name: "Successfully delete a Component", componentToDelete: "comp3", - components: []v1.Component{ - { - Name: "comp2", - ComponentUnion: v1.ComponentUnion{ - Container: &v1.ContainerComponent{ - Container: v1.Container{ - VolumeMounts: []v1.VolumeMount{ - testingutil.GetFakeVolumeMount("comp2", "/path"), - testingutil.GetFakeVolumeMount("comp2", "/path2"), - testingutil.GetFakeVolumeMount("comp3", "/path"), - }, - }, - }, - }, - }, - { - Name: "comp2", - ComponentUnion: v1.ComponentUnion{ - Volume: &v1.VolumeComponent{}, - }, - }, - { - Name: "comp3", - ComponentUnion: v1.ComponentUnion{ - Volume: &v1.VolumeComponent{}, - }, - }, - }, wantComponents: []v1.Component{ { Name: "comp2", @@ -472,43 +480,17 @@ func TestDeleteComponents(t *testing.T) { { name: "Missing Component", componentToDelete: "comp12", - components: []v1.Component{ - { - Name: "comp2", - ComponentUnion: v1.ComponentUnion{ - Volume: &v1.VolumeComponent{}, - }, - }, - }, - wantComponents: []v1.Component{ - { - Name: "comp2", - ComponentUnion: v1.ComponentUnion{ - Volume: &v1.VolumeComponent{}, - }, - }, - }, - wantErr: true, + wantErr: true, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - d := &DevfileV2{ - v1.Devfile{ - DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ - DevWorkspaceTemplateSpecContent: v1.DevWorkspaceTemplateSpecContent{ - Components: tt.components, - }, - }, - }, + err := d.DeleteComponent(tt.componentToDelete) + if (err != nil) != tt.wantErr { + t.Errorf("DeleteComponent() error = %v, wantErr %v", err, tt.wantErr) } - err := d.DeleteComponent(tt.componentToDelete) - if tt.wantErr && err == nil { - t.Errorf("Expected error from test but got nil") - } else if !tt.wantErr && err != nil { - t.Errorf("Got unexpected error: %s", err) - } else if err == nil { + if err == nil && !tt.wantErr { assert.Equal(t, tt.wantComponents, d.Components, "The two values should be the same.") } }) diff --git a/pkg/devfile/parser/data/v2/projects.go b/pkg/devfile/parser/data/v2/projects.go index e8cff3ba..676f243d 100644 --- a/pkg/devfile/parser/data/v2/projects.go +++ b/pkg/devfile/parser/data/v2/projects.go @@ -59,10 +59,11 @@ func (d *DevfileV2) UpdateProject(project v1.Project) { func (d *DevfileV2) DeleteProject(name string) error { found := false - for i := len(d.Projects) - 1; i >= 0; i-- { + for i := range d.Projects { if d.Projects[i].Name == name { found = true d.Projects = append(d.Projects[:i], d.Projects[i+1:]...) + break } } @@ -128,10 +129,11 @@ func (d *DevfileV2) UpdateStarterProject(project v1.StarterProject) { func (d *DevfileV2) DeleteStarterProject(name string) error { found := false - for i := len(d.StarterProjects) - 1; i >= 0; i-- { + for i := range d.StarterProjects { if d.StarterProjects[i].Name == name { found = true d.StarterProjects = append(d.StarterProjects[:i], d.StarterProjects[i+1:]...) + break } } diff --git a/pkg/devfile/parser/data/v2/projects_test.go b/pkg/devfile/parser/data/v2/projects_test.go index cc5d167c..554646a8 100644 --- a/pkg/devfile/parser/data/v2/projects_test.go +++ b/pkg/devfile/parser/data/v2/projects_test.go @@ -191,26 +191,34 @@ func TestDevfile200_UpdateProject(t *testing.T) { func TestDevfile200_DeleteProject(t *testing.T) { + d := &DevfileV2{ + v1.Devfile{ + DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ + DevWorkspaceTemplateSpecContent: v1.DevWorkspaceTemplateSpecContent{ + Projects: []v1.Project{ + { + Name: "nodejs", + ClonePath: "/project", + }, + { + Name: "java", + ClonePath: "/project2", + }, + }, + }, + }, + }, + } + tests := []struct { name string projectToDelete string - projects []v1.Project wantProjects []v1.Project wantErr bool }{ { name: "Project successfully deleted", projectToDelete: "nodejs", - projects: []v1.Project{ - { - Name: "nodejs", - ClonePath: "/project", - }, - { - Name: "java", - ClonePath: "/project2", - }, - }, wantProjects: []v1.Project{ { Name: "java", @@ -222,43 +230,17 @@ func TestDevfile200_DeleteProject(t *testing.T) { { name: "Project not found", projectToDelete: "nodejs1", - projects: []v1.Project{ - { - Name: "nodejs", - ClonePath: "/project", - }, - { - Name: "java", - ClonePath: "/project2", - }, - }, - wantProjects: []v1.Project{ - { - Name: "java", - ClonePath: "/project2", - }, - }, - wantErr: true, + wantErr: true, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - d := &DevfileV2{ - v1.Devfile{ - DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ - DevWorkspaceTemplateSpecContent: v1.DevWorkspaceTemplateSpecContent{ - Projects: tt.projects, - }, - }, - }, + err := d.DeleteProject(tt.projectToDelete) + if (err != nil) != tt.wantErr { + t.Errorf("DeleteProject() error = %v, wantErr %v", err, tt.wantErr) } - err := d.DeleteProject(tt.projectToDelete) - if tt.wantErr && err == nil { - t.Errorf("Expected error from test but got nil") - } else if !tt.wantErr && err != nil { - t.Errorf("Got unexpected error: %s", err) - } else if err == nil { + if err == nil && !tt.wantErr { assert.Equal(t, tt.wantProjects, d.Projects, "The two values should be the same.") } }) @@ -451,26 +433,34 @@ func TestDevfile200_UpdateStarterProject(t *testing.T) { func TestDevfile200_DeleteStarterProject(t *testing.T) { + d := &DevfileV2{ + v1.Devfile{ + DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ + DevWorkspaceTemplateSpecContent: v1.DevWorkspaceTemplateSpecContent{ + StarterProjects: []v1.StarterProject{ + { + Name: "nodejs", + SubDir: "/project", + }, + { + Name: "java", + SubDir: "/project2", + }, + }, + }, + }, + }, + } + tests := []struct { name string starterProjectToDelete string - starterProjects []v1.StarterProject wantStarterProjects []v1.StarterProject wantErr bool }{ { name: "Starter Project successfully deleted", starterProjectToDelete: "nodejs", - starterProjects: []v1.StarterProject{ - { - Name: "nodejs", - SubDir: "/project", - }, - { - Name: "java", - SubDir: "/project2", - }, - }, wantStarterProjects: []v1.StarterProject{ { Name: "java", @@ -482,43 +472,17 @@ func TestDevfile200_DeleteStarterProject(t *testing.T) { { name: "Starter Project not found", starterProjectToDelete: "nodejs1", - starterProjects: []v1.StarterProject{ - { - Name: "nodejs", - SubDir: "/project", - }, - { - Name: "java", - SubDir: "/project2", - }, - }, - wantStarterProjects: []v1.StarterProject{ - { - Name: "java", - SubDir: "/project2", - }, - }, - wantErr: true, + wantErr: true, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - d := &DevfileV2{ - v1.Devfile{ - DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ - DevWorkspaceTemplateSpecContent: v1.DevWorkspaceTemplateSpecContent{ - StarterProjects: tt.starterProjects, - }, - }, - }, + err := d.DeleteStarterProject(tt.starterProjectToDelete) + if (err != nil) != tt.wantErr { + t.Errorf("DeleteStarterProject() error = %v, wantErr %v", err, tt.wantErr) } - err := d.DeleteStarterProject(tt.starterProjectToDelete) - if tt.wantErr && err == nil { - t.Errorf("Expected error from test but got nil") - } else if !tt.wantErr && err != nil { - t.Errorf("Got unexpected error: %s", err) - } else if err == nil { + if err == nil && !tt.wantErr { assert.Equal(t, tt.wantStarterProjects, d.StarterProjects, "The two values should be the same.") } }) diff --git a/pkg/devfile/parser/data/v2/volumes.go b/pkg/devfile/parser/data/v2/volumes.go index 53a93232..8ffa8acd 100644 --- a/pkg/devfile/parser/data/v2/volumes.go +++ b/pkg/devfile/parser/data/v2/volumes.go @@ -8,23 +8,23 @@ import ( "github.com/devfile/library/pkg/devfile/parser/data/v2/common" ) -// AddVolumeMount adds the volume mount to the specified container component -func (d *DevfileV2) AddVolumeMount(componentName, name, path string) error { +// AddVolumeMount adds the volume mounts to the specified container component +func (d *DevfileV2) AddVolumeMount(componentName string, volumeMounts []v1.VolumeMount) error { var pathErrorContainers []string found := false for _, component := range d.Components { if component.Container != nil && component.Name == componentName { found = true - for _, volumeMount := range component.Container.VolumeMounts { - if volumeMount.Path == path { - var err = fmt.Errorf("another volume, %s, is mounted to the same path: %s, in the container: %s", volumeMount.Name, path, component.Name) - pathErrorContainers = append(pathErrorContainers, err.Error()) + for _, devfileVolumeMount := range component.Container.VolumeMounts { + for _, volumeMount := range volumeMounts { + if devfileVolumeMount.Path == volumeMount.Path { + pathErrorContainers = append(pathErrorContainers, fmt.Sprintf("unable to mount volume %s, as another volume %s is mounted to the same path %s in the container %s", volumeMount.Name, devfileVolumeMount.Name, volumeMount.Path, component.Name)) + } } } - component.Container.VolumeMounts = append(component.Container.VolumeMounts, v1.VolumeMount{ - Name: name, - Path: path, - }) + if len(pathErrorContainers) == 0 { + component.Container.VolumeMounts = append(component.Container.VolumeMounts, volumeMounts...) + } } } diff --git a/pkg/devfile/parser/data/v2/volumes_test.go b/pkg/devfile/parser/data/v2/volumes_test.go index 22e5c3d1..562cf273 100644 --- a/pkg/devfile/parser/data/v2/volumes_test.go +++ b/pkg/devfile/parser/data/v2/volumes_test.go @@ -19,8 +19,7 @@ func TestDevfile200_AddVolumeMount(t *testing.T) { type args struct { componentName string - name string - path string + volumeMounts []v1.VolumeMount } tests := []struct { name string @@ -60,8 +59,9 @@ func TestDevfile200_AddVolumeMount(t *testing.T) { }, }, args: args{ - name: volume0, - path: "/path0", + volumeMounts: []v1.VolumeMount{ + testingutil.GetFakeVolumeMount(volume0, "/path0"), + }, componentName: container0, }, wantComponents: []v1.Component{ @@ -104,7 +104,8 @@ func TestDevfile200_AddVolumeMount(t *testing.T) { Container: v1.Container{ Image: image0, VolumeMounts: []v1.VolumeMount{ - testingutil.GetFakeVolumeMount(volume1, "/data"), + testingutil.GetFakeVolumeMount(volume0, "/data0"), + testingutil.GetFakeVolumeMount(volume1, "/data1"), }, }, }, @@ -112,8 +113,10 @@ func TestDevfile200_AddVolumeMount(t *testing.T) { }, }, args: args{ - name: volume0, - path: "/data", + volumeMounts: []v1.VolumeMount{ + testingutil.GetFakeVolumeMount(volume0, "/data1"), + testingutil.GetFakeVolumeMount(volume1, "/data0"), + }, componentName: container0, }, wantErr: true, @@ -136,8 +139,9 @@ func TestDevfile200_AddVolumeMount(t *testing.T) { }, }, args: args{ - name: volume0, - path: "/data", + volumeMounts: []v1.VolumeMount{ + testingutil.GetFakeVolumeMount(volume0, "/data"), + }, componentName: container1, }, wantErr: true, @@ -155,12 +159,12 @@ func TestDevfile200_AddVolumeMount(t *testing.T) { }, } - err := d.AddVolumeMount(tt.args.componentName, tt.args.name, tt.args.path) - if tt.wantErr && err == nil { - t.Errorf("Expected error from test but got nil") - } else if !tt.wantErr && err != nil { - t.Errorf("Got unexpected error: %s", err) - } else if err == nil { + err := d.AddVolumeMount(tt.args.componentName, tt.args.volumeMounts) + if (err != nil) != tt.wantErr { + t.Errorf("AddVolumeMount() error = %v, wantErr %v", err, tt.wantErr) + } + + if err == nil && !tt.wantErr { assert.Equal(t, tt.wantComponents, d.Components, "The two values should be the same.") } }) @@ -169,44 +173,53 @@ func TestDevfile200_AddVolumeMount(t *testing.T) { func TestDevfile200_DeleteVolumeMounts(t *testing.T) { - tests := []struct { - name string - volMountToDelete string - components []v1.Component - wantComponents []v1.Component - wantErr bool - }{ - { - name: "Volume Component with mounts", - volMountToDelete: "comp2", - components: []v1.Component{ - { - Name: "comp1", - ComponentUnion: v1.ComponentUnion{ - Container: &v1.ContainerComponent{ - Container: v1.Container{ - VolumeMounts: []v1.VolumeMount{ - testingutil.GetFakeVolumeMount("comp2", "/path"), - testingutil.GetFakeVolumeMount("comp2", "/path2"), - testingutil.GetFakeVolumeMount("comp3", "/path"), + d := &DevfileV2{ + v1.Devfile{ + DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ + DevWorkspaceTemplateSpecContent: v1.DevWorkspaceTemplateSpecContent{ + Components: []v1.Component{ + { + Name: "comp1", + ComponentUnion: v1.ComponentUnion{ + Container: &v1.ContainerComponent{ + Container: v1.Container{ + VolumeMounts: []v1.VolumeMount{ + testingutil.GetFakeVolumeMount("comp2", "/path"), + testingutil.GetFakeVolumeMount("comp2", "/path2"), + testingutil.GetFakeVolumeMount("comp3", "/path"), + testingutil.GetFakeVolumeMount("comp2", "/path3"), + }, + }, }, }, }, - }, - }, - { - Name: "comp4", - ComponentUnion: v1.ComponentUnion{ - Container: &v1.ContainerComponent{ - Container: v1.Container{ - VolumeMounts: []v1.VolumeMount{ - testingutil.GetFakeVolumeMount("comp2", "/path"), + { + Name: "comp4", + ComponentUnion: v1.ComponentUnion{ + Container: &v1.ContainerComponent{ + Container: v1.Container{ + VolumeMounts: []v1.VolumeMount{ + testingutil.GetFakeVolumeMount("comp2", "/path"), + }, + }, }, }, }, }, }, }, + }, + } + + tests := []struct { + name string + volMountToDelete string + wantComponents []v1.Component + wantErr bool + }{ + { + name: "Volume Component with mounts", + volMountToDelete: "comp2", wantComponents: []v1.Component{ { Name: "comp1", @@ -236,55 +249,17 @@ func TestDevfile200_DeleteVolumeMounts(t *testing.T) { { name: "Missing mount name", volMountToDelete: "comp1", - components: []v1.Component{ - { - Name: "comp4", - ComponentUnion: v1.ComponentUnion{ - Container: &v1.ContainerComponent{ - Container: v1.Container{ - VolumeMounts: []v1.VolumeMount{ - testingutil.GetFakeVolumeMount("comp2", "/path"), - }, - }, - }, - }, - }, - }, - wantComponents: []v1.Component{ - { - Name: "comp4", - ComponentUnion: v1.ComponentUnion{ - Container: &v1.ContainerComponent{ - Container: v1.Container{ - VolumeMounts: []v1.VolumeMount{ - testingutil.GetFakeVolumeMount("comp2", "/path"), - }, - }, - }, - }, - }, - }, - wantErr: true, + wantErr: true, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - d := &DevfileV2{ - v1.Devfile{ - DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ - DevWorkspaceTemplateSpecContent: v1.DevWorkspaceTemplateSpecContent{ - Components: tt.components, - }, - }, - }, + err := d.DeleteVolumeMount(tt.volMountToDelete) + if (err != nil) != tt.wantErr { + t.Errorf("DeleteVolumeMount() error = %v, wantErr %v", err, tt.wantErr) } - err := d.DeleteVolumeMount(tt.volMountToDelete) - if tt.wantErr && err == nil { - t.Errorf("Expected error from test but got nil") - } else if !tt.wantErr && err != nil { - t.Errorf("Got unexpected error: %s", err) - } else if err == nil { + if err == nil && !tt.wantErr { assert.Equal(t, tt.wantComponents, d.Components, "The two values should be the same.") } }) @@ -378,11 +353,11 @@ func TestDevfile200_GetVolumeMountPath(t *testing.T) { }, } gotPath, err := d.GetVolumeMountPath(tt.mountName, tt.componentName) - if tt.wantErr && err == nil { - t.Errorf("Expected error from test but got nil") - } else if !tt.wantErr && err != nil { - t.Errorf("Got unexpected error: %s", err) - } else if err == nil { + if (err != nil) != tt.wantErr { + t.Errorf("GetVolumeMountPath() error = %v, wantErr %v", err, tt.wantErr) + } + + if err == nil && !tt.wantErr { assert.Equal(t, tt.wantPath, gotPath, "The two values should be the same.") } }) From a493b47d01df76fd1600ba93c92cebbd2a54fa8c Mon Sep 17 00:00:00 2001 From: Maysun J Faisal Date: Fri, 5 Mar 2021 16:07:28 -0500 Subject: [PATCH 5/7] Rebase and update Signed-off-by: Maysun J Faisal --- tests/v2/utils/command_test_utils.go | 270 --------------------------- tests/v2/utils/library/test_utils.go | 2 +- 2 files changed, 1 insertion(+), 271 deletions(-) delete mode 100644 tests/v2/utils/command_test_utils.go diff --git a/tests/v2/utils/command_test_utils.go b/tests/v2/utils/command_test_utils.go deleted file mode 100644 index 27bfdcae..00000000 --- a/tests/v2/utils/command_test_utils.go +++ /dev/null @@ -1,270 +0,0 @@ -package utils - -import ( - "errors" - "fmt" - "io/ioutil" - - "github.com/google/go-cmp/cmp" - "sigs.k8s.io/yaml" - - schema "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" -) - -// commandAdded adds a new command to the test schema data and to the parser data -func (devfile *TestDevfile) commandAdded(command schema.Command) { - LogInfoMessage(fmt.Sprintf("command added Id: %s", command.Id)) - devfile.SchemaDevFile.Commands = append(devfile.SchemaDevFile.Commands, command) - devfile.ParserData.AddCommands([]schema.Command{command}) -} - -// commandUpdated updates a command in the parser data -func (devfile *TestDevfile) commandUpdated(command schema.Command) { - LogInfoMessage(fmt.Sprintf("command updated Id: %s", command.Id)) - devfile.ParserData.UpdateCommand(command) -} - -// addEnv creates and returns a specifed number of env attributes in a schema structure -func addEnv(numEnv int) []schema.EnvVar { - commandEnvs := make([]schema.EnvVar, numEnv) - for i := 0; i < numEnv; i++ { - commandEnvs[i].Name = "Name_" + GetRandomString(5, false) - commandEnvs[i].Value = "Value_" + GetRandomString(5, false) - LogInfoMessage(fmt.Sprintf("Add Env: %s", commandEnvs[i])) - } - return commandEnvs -} - -// addAttributes creates returns a specifed number of attributes in a schema structure -func addAttributes(numAtrributes int) map[string]string { - attributes := make(map[string]string) - for i := 0; i < numAtrributes; i++ { - AttributeName := "Name_" + GetRandomString(6, false) - attributes[AttributeName] = "Value_" + GetRandomString(6, false) - LogInfoMessage(fmt.Sprintf("Add attribute : %s = %s", AttributeName, attributes[AttributeName])) - } - return attributes -} - -// addGroup creates and returns a group in a schema structure -func (devfile *TestDevfile) addGroup() *schema.CommandGroup { - - commandGroup := schema.CommandGroup{} - commandGroup.Kind = GetRandomGroupKind() - LogInfoMessage(fmt.Sprintf("group Kind: %s, default already set %t", commandGroup.Kind, devfile.GroupDefaults[commandGroup.Kind])) - // Ensure only one and at least one of each type are labelled as default - if !devfile.GroupDefaults[commandGroup.Kind] { - devfile.GroupDefaults[commandGroup.Kind] = true - commandGroup.IsDefault = true - } else { - commandGroup.IsDefault = false - } - LogInfoMessage(fmt.Sprintf("group isDefault: %t", commandGroup.IsDefault)) - return &commandGroup -} - -// AddCommand creates a command of a specified type in a schema structure and pupulates it with random attributes -func (devfile *TestDevfile) AddCommand(commandType schema.CommandType) schema.Command { - - var command *schema.Command - if commandType == schema.ExecCommandType { - command = devfile.createExecCommand() - devfile.setExecCommandValues(command) - // command must be mentioned by a container component - command.Exec.Component = devfile.GetContainerName() - } else if commandType == schema.CompositeCommandType { - command = devfile.createCompositeCommand() - devfile.setCompositeCommandValues(command) - } - return *command -} - -// UpdateCommand randomly updates attribute values of a specified command in the devfile schema -func (devfile *TestDevfile) UpdateCommand(commandId string) error { - - var err error - testCommand, found := getSchemaCommand(devfile.SchemaDevFile.Commands, commandId) - if found { - LogInfoMessage(fmt.Sprintf("Updating command id: %s", commandId)) - if testCommand.Exec != nil { - devfile.setExecCommandValues(testCommand) - } else if testCommand.Composite != nil { - devfile.setCompositeCommandValues(testCommand) - } - } else { - err = errors.New(LogErrorMessage(fmt.Sprintf("Command not found in test : %s", commandId))) - } - return err -} - -// createExecCommand creates and returns an empty exec command in a schema structure -func (devfile *TestDevfile) createExecCommand() *schema.Command { - - LogInfoMessage("Create an exec command :") - command := schema.Command{} - command.Id = GetRandomUniqueString(8, true) - LogInfoMessage(fmt.Sprintf("command Id: %s", command.Id)) - command.Exec = &schema.ExecCommand{} - devfile.commandAdded(command) - return &command - -} - -// setExecCommandValues randomly sets exec command attribute to random values -func (devfile *TestDevfile) setExecCommandValues(command *schema.Command) { - - execCommand := command.Exec - execCommand.CommandLine = GetRandomString(4, false) + " " + GetRandomString(4, false) - LogInfoMessage(fmt.Sprintf("....... commandLine: %s", execCommand.CommandLine)) - - // If group already leave it to make sure defaults are not deleted or added - if execCommand.Group == nil { - if GetRandomDecision(2, 1) { - execCommand.Group = devfile.addGroup() - } - } - - if GetBinaryDecision() { - execCommand.Label = GetRandomString(12, false) - LogInfoMessage(fmt.Sprintf("....... label: %s", execCommand.Label)) - } else { - execCommand.Label = "" - } - - if GetBinaryDecision() { - execCommand.WorkingDir = "./tmp" - LogInfoMessage(fmt.Sprintf("....... WorkingDir: %s", execCommand.WorkingDir)) - } else { - execCommand.WorkingDir = "" - } - - execCommand.HotReloadCapable = GetBinaryDecision() - LogInfoMessage(fmt.Sprintf("....... HotReloadCapable: %t", execCommand.HotReloadCapable)) - - if GetBinaryDecision() { - execCommand.Env = addEnv(GetRandomNumber(4)) - } else { - execCommand.Env = nil - } - devfile.commandUpdated(*command) - -} - -// getSchemaCommand get a specified command from the devfile schema structure -func getSchemaCommand(commands []schema.Command, id string) (*schema.Command, bool) { - found := false - var schemaCommand schema.Command - for _, command := range commands { - if command.Id == id { - schemaCommand = command - found = true - break - } - } - return &schemaCommand, found -} - -// createCompositeCommand creates an empty composite command in a schema structure -func (devfile *TestDevfile) createCompositeCommand() *schema.Command { - - LogInfoMessage("Create a composite command :") - command := schema.Command{} - command.Id = GetRandomUniqueString(8, true) - LogInfoMessage(fmt.Sprintf("command Id: %s", command.Id)) - command.Composite = &schema.CompositeCommand{} - devfile.commandAdded(command) - - return &command -} - -// setCompositeCommandValues randomly sets composite command attribute to random values -func (devfile *TestDevfile) setCompositeCommandValues(command *schema.Command) { - - compositeCommand := command.Composite - numCommands := GetRandomNumber(3) - - for i := 0; i < numCommands; i++ { - execCommand := devfile.AddCommand(schema.ExecCommandType) - compositeCommand.Commands = append(compositeCommand.Commands, execCommand.Id) - LogInfoMessage(fmt.Sprintf("....... command %d of %d : %s", i, numCommands, execCommand.Id)) - } - - // If group already exists - leave it to make sure defaults are not deleted or added - if compositeCommand.Group == nil { - if GetRandomDecision(2, 1) { - compositeCommand.Group = devfile.addGroup() - } - } - - if GetBinaryDecision() { - compositeCommand.Label = GetRandomString(12, false) - LogInfoMessage(fmt.Sprintf("....... label: %s", compositeCommand.Label)) - } - - if GetBinaryDecision() { - compositeCommand.Parallel = true - LogInfoMessage(fmt.Sprintf("....... Parallel: %t", compositeCommand.Parallel)) - } - - devfile.commandUpdated(*command) -} - -// VerifyCommands verifies commands returned by the parser are the same as those saved in the devfile schema -func (devfile *TestDevfile) VerifyCommands(parserCommands []schema.Command) error { - - LogInfoMessage("Enter VerifyCommands") - var errorString []string - - // Compare entire array of commands - if !cmp.Equal(parserCommands, devfile.SchemaDevFile.Commands) { - errorString = append(errorString, LogErrorMessage(fmt.Sprintf("Command array compare failed."))) - // Array compare failed. Narrow down by comparing indivdual commands - for _, command := range parserCommands { - if testCommand, found := getSchemaCommand(devfile.SchemaDevFile.Commands, command.Id); found { - if !cmp.Equal(command, *testCommand) { - parserFilename := AddSuffixToFileName(devfile.FileName, "_"+command.Id+"_Parser") - testFilename := AddSuffixToFileName(devfile.FileName, "_"+command.Id+"_Test") - LogInfoMessage(fmt.Sprintf(".......marshall and write devfile %s", devfile.FileName)) - c, err := yaml.Marshal(command) - if err != nil { - errorString = append(errorString, LogErrorMessage(fmt.Sprintf(".......marshall devfile %s", parserFilename))) - } else { - err = ioutil.WriteFile(parserFilename, c, 0644) - if err != nil { - errorString = append(errorString, LogErrorMessage(fmt.Sprintf(".......write devfile %s", parserFilename))) - } - } - LogInfoMessage(fmt.Sprintf(".......marshall and write devfile %s", testFilename)) - c, err = yaml.Marshal(testCommand) - if err != nil { - errorString = append(errorString, LogErrorMessage(fmt.Sprintf(".......marshall devfile %s", testFilename))) - } else { - err = ioutil.WriteFile(testFilename, c, 0644) - if err != nil { - errorString = append(errorString, LogErrorMessage(fmt.Sprintf(".......write devfile %s", testFilename))) - } - } - errorString = append(errorString, LogInfoMessage(fmt.Sprintf("Command %s did not match, see files : %s and %s", command.Id, parserFilename, testFilename))) - } else { - LogInfoMessage(fmt.Sprintf(" --> Command matched : %s", command.Id)) - } - } else { - errorString = append(errorString, LogErrorMessage(fmt.Sprintf("Command from parser not known to test - id : %s ", command.Id))) - } - - } - for _, command := range devfile.SchemaDevFile.Commands { - if _, found := getSchemaCommand(parserCommands, command.Id); !found { - errorString = append(errorString, LogErrorMessage(fmt.Sprintf("Command from test not returned by parser : %s ", command.Id))) - } - } - } else { - LogInfoMessage(fmt.Sprintf(" --> Command structures matched")) - } - - var err error - if len(errorString) > 0 { - err = errors.New(fmt.Sprint(errorString)) - } - return err -} diff --git a/tests/v2/utils/library/test_utils.go b/tests/v2/utils/library/test_utils.go index c5262ad5..26cd93a2 100644 --- a/tests/v2/utils/library/test_utils.go +++ b/tests/v2/utils/library/test_utils.go @@ -55,7 +55,7 @@ type DevfileFollower struct { // AddCommand adds the specified command to the library data func (devfileFollower DevfileFollower) AddCommand(command schema.Command) error { - return devfileFollower.LibraryData.AddCommands(command) + return devfileFollower.LibraryData.AddCommands([]schema.Command{command}) } // UpdateCommand updates the specified command in the library data From 6f010e175c8e2e223f470e78318ae182b68f2ff3 Mon Sep 17 00:00:00 2001 From: Maysun J Faisal Date: Fri, 5 Mar 2021 16:51:20 -0500 Subject: [PATCH 6/7] Update events func for checking pointers Signed-off-by: Maysun J Faisal --- pkg/devfile/parser/data/interface.go | 2 +- pkg/devfile/parser/data/v2/events.go | 18 +++++-- pkg/devfile/parser/data/v2/events_test.go | 61 +++++++++++++++++----- pkg/devfile/parser/data/v2/volumes.go | 4 +- pkg/devfile/parser/data/v2/volumes_test.go | 4 +- 5 files changed, 66 insertions(+), 23 deletions(-) diff --git a/pkg/devfile/parser/data/interface.go b/pkg/devfile/parser/data/interface.go index 5883d8f6..3086b78d 100644 --- a/pkg/devfile/parser/data/interface.go +++ b/pkg/devfile/parser/data/interface.go @@ -47,7 +47,7 @@ type DevfileData interface { DeleteCommand(id string) error // volume mount related methods - AddVolumeMount(componentName string, volumeMounts []v1.VolumeMount) error + AddVolumeMounts(componentName string, volumeMounts []v1.VolumeMount) error DeleteVolumeMount(name string) error GetVolumeMountPath(mountName, componentName string) (string, error) diff --git a/pkg/devfile/parser/data/v2/events.go b/pkg/devfile/parser/data/v2/events.go index 621a8d05..eb3e3864 100644 --- a/pkg/devfile/parser/data/v2/events.go +++ b/pkg/devfile/parser/data/v2/events.go @@ -16,6 +16,11 @@ func (d *DevfileV2) GetEvents() v1.Events { // AddEvents adds the Events Object to the devfile's events // if the event is already defined in the devfile, error out func (d *DevfileV2) AddEvents(events v1.Events) error { + + if d.Events == nil { + d.Events = &v1.Events{} + } + if len(events.PreStop) > 0 { if len(d.Events.PreStop) > 0 { return &common.FieldAlreadyExistError{Field: "pre stop"} @@ -50,16 +55,21 @@ func (d *DevfileV2) AddEvents(events v1.Events) error { // UpdateEvents updates the devfile's events // it only updates the events passed to it func (d *DevfileV2) UpdateEvents(postStart, postStop, preStart, preStop []string) { - if len(postStart) != 0 { + + if d.Events == nil { + d.Events = &v1.Events{} + } + + if postStart != nil { d.Events.PostStart = postStart } - if len(postStop) != 0 { + if postStop != nil { d.Events.PostStop = postStop } - if len(preStart) != 0 { + if preStart != nil { d.Events.PreStart = preStart } - if len(preStop) != 0 { + if preStop != nil { d.Events.PreStop = preStop } } diff --git a/pkg/devfile/parser/data/v2/events_test.go b/pkg/devfile/parser/data/v2/events_test.go index 2cbf0715..50b3018e 100644 --- a/pkg/devfile/parser/data/v2/events_test.go +++ b/pkg/devfile/parser/data/v2/events_test.go @@ -11,13 +11,13 @@ func TestDevfile200_AddEvents(t *testing.T) { tests := []struct { name string - currentEvents v1.Events + currentEvents *v1.Events newEvents v1.Events wantErr bool }{ { - name: "case 1: successfully add the events", - currentEvents: v1.Events{ + name: "successfully add the events", + currentEvents: &v1.Events{ WorkspaceEvents: v1.WorkspaceEvents{ PreStart: []string{"preStart1"}, }, @@ -30,8 +30,18 @@ func TestDevfile200_AddEvents(t *testing.T) { wantErr: false, }, { - name: "case 2: event already present", - currentEvents: v1.Events{ + name: "successfully add the events to empty devfile event", + currentEvents: nil, + newEvents: v1.Events{ + WorkspaceEvents: v1.WorkspaceEvents{ + PostStart: []string{"postStart1"}, + }, + }, + wantErr: false, + }, + { + name: "event already present", + currentEvents: &v1.Events{ WorkspaceEvents: v1.WorkspaceEvents{ PreStart: []string{"preStart1"}, }, @@ -50,17 +60,17 @@ func TestDevfile200_AddEvents(t *testing.T) { v1.Devfile{ DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ DevWorkspaceTemplateSpecContent: v1.DevWorkspaceTemplateSpecContent{ - Events: &tt.currentEvents, + Events: tt.currentEvents, }, }, }, } - got := d.AddEvents(tt.newEvents) + err := d.AddEvents(tt.newEvents) - if !tt.wantErr && got != nil { - t.Errorf("TestDevfile200_AddEvents() unexpected error - %+v", got) - } else if tt.wantErr && got == nil { + if !tt.wantErr && err != nil { + t.Errorf("TestDevfile200_AddEvents() unexpected error - %+v", err) + } else if tt.wantErr && err == nil { t.Errorf("TestDevfile200_AddEvents() expected error but got nil") } @@ -72,16 +82,39 @@ func TestDevfile200_UpdateEvents(t *testing.T) { tests := []struct { name string - currentEvents v1.Events + currentEvents *v1.Events newEvents v1.Events }{ { - name: "case 1: successfully add the events", - currentEvents: v1.Events{ + name: "successfully add/update the events", + currentEvents: &v1.Events{ + WorkspaceEvents: v1.WorkspaceEvents{ + PreStart: []string{"preStart1"}, + }, + }, + newEvents: v1.Events{ + WorkspaceEvents: v1.WorkspaceEvents{ + PreStart: []string{"preStart2"}, + PostStart: []string{"postStart2"}, + }, + }, + }, + { + name: "successfully update the events to empty", + currentEvents: &v1.Events{ WorkspaceEvents: v1.WorkspaceEvents{ PreStart: []string{"preStart1"}, }, }, + newEvents: v1.Events{ + WorkspaceEvents: v1.WorkspaceEvents{ + PreStart: []string{""}, + }, + }, + }, + { + name: "successfully add the events to empty devfile events", + currentEvents: nil, newEvents: v1.Events{ WorkspaceEvents: v1.WorkspaceEvents{ PreStart: []string{"preStart2"}, @@ -96,7 +129,7 @@ func TestDevfile200_UpdateEvents(t *testing.T) { v1.Devfile{ DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{ DevWorkspaceTemplateSpecContent: v1.DevWorkspaceTemplateSpecContent{ - Events: &tt.currentEvents, + Events: tt.currentEvents, }, }, }, diff --git a/pkg/devfile/parser/data/v2/volumes.go b/pkg/devfile/parser/data/v2/volumes.go index 8ffa8acd..4746ebcd 100644 --- a/pkg/devfile/parser/data/v2/volumes.go +++ b/pkg/devfile/parser/data/v2/volumes.go @@ -8,8 +8,8 @@ import ( "github.com/devfile/library/pkg/devfile/parser/data/v2/common" ) -// AddVolumeMount adds the volume mounts to the specified container component -func (d *DevfileV2) AddVolumeMount(componentName string, volumeMounts []v1.VolumeMount) error { +// AddVolumeMounts adds the volume mounts to the specified container component +func (d *DevfileV2) AddVolumeMounts(componentName string, volumeMounts []v1.VolumeMount) error { var pathErrorContainers []string found := false for _, component := range d.Components { diff --git a/pkg/devfile/parser/data/v2/volumes_test.go b/pkg/devfile/parser/data/v2/volumes_test.go index 562cf273..9df82118 100644 --- a/pkg/devfile/parser/data/v2/volumes_test.go +++ b/pkg/devfile/parser/data/v2/volumes_test.go @@ -159,9 +159,9 @@ func TestDevfile200_AddVolumeMount(t *testing.T) { }, } - err := d.AddVolumeMount(tt.args.componentName, tt.args.volumeMounts) + err := d.AddVolumeMounts(tt.args.componentName, tt.args.volumeMounts) if (err != nil) != tt.wantErr { - t.Errorf("AddVolumeMount() error = %v, wantErr %v", err, tt.wantErr) + t.Errorf("AddVolumeMounts() error = %v, wantErr %v", err, tt.wantErr) } if err == nil && !tt.wantErr { From 5e32c06e9c0a75b7d4ee66206140dbea0af63f86 Mon Sep 17 00:00:00 2001 From: Maysun J Faisal Date: Mon, 8 Mar 2021 17:28:40 -0500 Subject: [PATCH 7/7] Address review feedback 2 Signed-off-by: Maysun J Faisal --- pkg/devfile/parser/data/v2/commands.go | 14 +++------- pkg/devfile/parser/data/v2/commands_test.go | 5 ++-- pkg/devfile/parser/data/v2/components.go | 14 +++------- pkg/devfile/parser/data/v2/components_test.go | 4 +-- pkg/devfile/parser/data/v2/events_test.go | 6 ++-- pkg/devfile/parser/data/v2/projects.go | 28 ++++++------------- pkg/devfile/parser/data/v2/projects_test.go | 8 ++---- pkg/devfile/parser/data/v2/volumes.go | 13 ++++----- pkg/devfile/parser/data/v2/volumes_test.go | 12 ++------ 9 files changed, 32 insertions(+), 72 deletions(-) diff --git a/pkg/devfile/parser/data/v2/commands.go b/pkg/devfile/parser/data/v2/commands.go index 998b1caa..bcdb3f49 100644 --- a/pkg/devfile/parser/data/v2/commands.go +++ b/pkg/devfile/parser/data/v2/commands.go @@ -57,21 +57,15 @@ func (d *DevfileV2) UpdateCommand(command v1.Command) { // DeleteCommand removes the specified command func (d *DevfileV2) DeleteCommand(id string) error { - found := false for i := range d.Commands { if d.Commands[i].Id == id { - found = true d.Commands = append(d.Commands[:i], d.Commands[i+1:]...) - break + return nil } } - if !found { - return &common.FieldNotFoundError{ - Field: "command", - Name: id, - } + return &common.FieldNotFoundError{ + Field: "command", + Name: id, } - - return nil } diff --git a/pkg/devfile/parser/data/v2/commands_test.go b/pkg/devfile/parser/data/v2/commands_test.go index b2d39502..29d6473d 100644 --- a/pkg/devfile/parser/data/v2/commands_test.go +++ b/pkg/devfile/parser/data/v2/commands_test.go @@ -373,9 +373,8 @@ func TestDeleteCommands(t *testing.T) { err := d.DeleteCommand(tt.commandToDelete) if (err != nil) != tt.wantErr { t.Errorf("DeleteCommand() error = %v, wantErr %v", err, tt.wantErr) - } - - if err == nil && !tt.wantErr { + return + } else if err == nil { assert.Equal(t, tt.wantCommands, d.Commands, "The two values should be the same.") } }) diff --git a/pkg/devfile/parser/data/v2/components.go b/pkg/devfile/parser/data/v2/components.go index 92a30aba..0bac4bfd 100644 --- a/pkg/devfile/parser/data/v2/components.go +++ b/pkg/devfile/parser/data/v2/components.go @@ -88,21 +88,15 @@ func (d *DevfileV2) UpdateComponent(component v1.Component) { // DeleteComponent removes the specified component func (d *DevfileV2) DeleteComponent(name string) error { - found := false for i := range d.Components { if d.Components[i].Name == name { - found = true d.Components = append(d.Components[:i], d.Components[i+1:]...) - break + return nil } } - if !found { - return &common.FieldNotFoundError{ - Field: "component", - Name: name, - } + return &common.FieldNotFoundError{ + Field: "component", + Name: name, } - - return nil } diff --git a/pkg/devfile/parser/data/v2/components_test.go b/pkg/devfile/parser/data/v2/components_test.go index 929c9f97..09db14fe 100644 --- a/pkg/devfile/parser/data/v2/components_test.go +++ b/pkg/devfile/parser/data/v2/components_test.go @@ -488,9 +488,7 @@ func TestDeleteComponents(t *testing.T) { err := d.DeleteComponent(tt.componentToDelete) if (err != nil) != tt.wantErr { t.Errorf("DeleteComponent() error = %v, wantErr %v", err, tt.wantErr) - } - - if err == nil && !tt.wantErr { + } else if err == nil { assert.Equal(t, tt.wantComponents, d.Components, "The two values should be the same.") } }) diff --git a/pkg/devfile/parser/data/v2/events_test.go b/pkg/devfile/parser/data/v2/events_test.go index 50b3018e..a66c7a7c 100644 --- a/pkg/devfile/parser/data/v2/events_test.go +++ b/pkg/devfile/parser/data/v2/events_test.go @@ -68,10 +68,8 @@ func TestDevfile200_AddEvents(t *testing.T) { err := d.AddEvents(tt.newEvents) - if !tt.wantErr && err != nil { - t.Errorf("TestDevfile200_AddEvents() unexpected error - %+v", err) - } else if tt.wantErr && err == nil { - t.Errorf("TestDevfile200_AddEvents() expected error but got nil") + if (err != nil) != tt.wantErr { + t.Errorf("DeleteCommand() error = %v, wantErr %v", err, tt.wantErr) } }) diff --git a/pkg/devfile/parser/data/v2/projects.go b/pkg/devfile/parser/data/v2/projects.go index 676f243d..62185bee 100644 --- a/pkg/devfile/parser/data/v2/projects.go +++ b/pkg/devfile/parser/data/v2/projects.go @@ -58,23 +58,17 @@ func (d *DevfileV2) UpdateProject(project v1.Project) { // DeleteProject removes the specified project func (d *DevfileV2) DeleteProject(name string) error { - found := false for i := range d.Projects { if d.Projects[i].Name == name { - found = true d.Projects = append(d.Projects[:i], d.Projects[i+1:]...) - break + return nil } } - if !found { - return &common.FieldNotFoundError{ - Field: "project", - Name: name, - } + return &common.FieldNotFoundError{ + Field: "project", + Name: name, } - - return nil } //GetStarterProjects returns the DevfileStarterProject parsed from devfile @@ -128,21 +122,15 @@ func (d *DevfileV2) UpdateStarterProject(project v1.StarterProject) { // DeleteStarterProject removes the specified starter project func (d *DevfileV2) DeleteStarterProject(name string) error { - found := false for i := range d.StarterProjects { if d.StarterProjects[i].Name == name { - found = true d.StarterProjects = append(d.StarterProjects[:i], d.StarterProjects[i+1:]...) - break + return nil } } - if !found { - return &common.FieldNotFoundError{ - Field: "starter project", - Name: name, - } + return &common.FieldNotFoundError{ + Field: "starter project", + Name: name, } - - return nil } diff --git a/pkg/devfile/parser/data/v2/projects_test.go b/pkg/devfile/parser/data/v2/projects_test.go index 554646a8..d3455f7e 100644 --- a/pkg/devfile/parser/data/v2/projects_test.go +++ b/pkg/devfile/parser/data/v2/projects_test.go @@ -238,9 +238,7 @@ func TestDevfile200_DeleteProject(t *testing.T) { err := d.DeleteProject(tt.projectToDelete) if (err != nil) != tt.wantErr { t.Errorf("DeleteProject() error = %v, wantErr %v", err, tt.wantErr) - } - - if err == nil && !tt.wantErr { + } else if err == nil { assert.Equal(t, tt.wantProjects, d.Projects, "The two values should be the same.") } }) @@ -480,9 +478,7 @@ func TestDevfile200_DeleteStarterProject(t *testing.T) { err := d.DeleteStarterProject(tt.starterProjectToDelete) if (err != nil) != tt.wantErr { t.Errorf("DeleteStarterProject() error = %v, wantErr %v", err, tt.wantErr) - } - - if err == nil && !tt.wantErr { + } else if err == nil { assert.Equal(t, tt.wantStarterProjects, d.StarterProjects, "The two values should be the same.") } }) diff --git a/pkg/devfile/parser/data/v2/volumes.go b/pkg/devfile/parser/data/v2/volumes.go index 4746ebcd..1f0abd48 100644 --- a/pkg/devfile/parser/data/v2/volumes.go +++ b/pkg/devfile/parser/data/v2/volumes.go @@ -47,6 +47,10 @@ func (d *DevfileV2) DeleteVolumeMount(name string) error { found := false for i := range d.Components { if d.Components[i].Container != nil && d.Components[i].Name != name { + // Volume Mounts can have multiple instances of a volume mounted at different paths + // As arrays are rearraged/shifted for deletion, we lose one element every time there is a match + // Looping backward is efficient, otherwise we would have to manually decrement counter + // if we looped forward for j := len(d.Components[i].Container.VolumeMounts) - 1; j >= 0; j-- { if d.Components[i].Container.VolumeMounts[j].Name == name { found = true @@ -68,17 +72,14 @@ func (d *DevfileV2) DeleteVolumeMount(name string) error { // GetVolumeMountPath gets the mount path of the specified volume mount from the specified container component func (d *DevfileV2) GetVolumeMountPath(mountName, componentName string) (string, error) { - mountFound := false componentFound := false - var path string for _, component := range d.Components { if component.Container != nil && component.Name == componentName { componentFound = true for _, volumeMount := range component.Container.VolumeMounts { if volumeMount.Name == mountName { - mountFound = true - path = volumeMount.Path + return volumeMount.Path, nil } } } @@ -89,9 +90,7 @@ func (d *DevfileV2) GetVolumeMountPath(mountName, componentName string) (string, Field: "container component", Name: componentName, } - } else if !mountFound { - return "", fmt.Errorf("volume %s not mounted to component %s", mountName, componentName) } - return path, nil + return "", fmt.Errorf("volume %s not mounted to component %s", mountName, componentName) } diff --git a/pkg/devfile/parser/data/v2/volumes_test.go b/pkg/devfile/parser/data/v2/volumes_test.go index 9df82118..0554ef0e 100644 --- a/pkg/devfile/parser/data/v2/volumes_test.go +++ b/pkg/devfile/parser/data/v2/volumes_test.go @@ -162,9 +162,7 @@ func TestDevfile200_AddVolumeMount(t *testing.T) { err := d.AddVolumeMounts(tt.args.componentName, tt.args.volumeMounts) if (err != nil) != tt.wantErr { t.Errorf("AddVolumeMounts() error = %v, wantErr %v", err, tt.wantErr) - } - - if err == nil && !tt.wantErr { + } else if err == nil { assert.Equal(t, tt.wantComponents, d.Components, "The two values should be the same.") } }) @@ -257,9 +255,7 @@ func TestDevfile200_DeleteVolumeMounts(t *testing.T) { err := d.DeleteVolumeMount(tt.volMountToDelete) if (err != nil) != tt.wantErr { t.Errorf("DeleteVolumeMount() error = %v, wantErr %v", err, tt.wantErr) - } - - if err == nil && !tt.wantErr { + } else if err == nil { assert.Equal(t, tt.wantComponents, d.Components, "The two values should be the same.") } }) @@ -355,9 +351,7 @@ func TestDevfile200_GetVolumeMountPath(t *testing.T) { gotPath, err := d.GetVolumeMountPath(tt.mountName, tt.componentName) if (err != nil) != tt.wantErr { t.Errorf("GetVolumeMountPath() error = %v, wantErr %v", err, tt.wantErr) - } - - if err == nil && !tt.wantErr { + } else if err == nil { assert.Equal(t, tt.wantPath, gotPath, "The two values should be the same.") } })