Address review feedback 2

Signed-off-by: Maysun J Faisal <[email protected]>

Author: maysunfaisal

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
 }

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.")
 			}
 		})

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
 }

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.")
 			}
 		})

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)
 			}
 
 		})

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
 }

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.")
 			}
 		})

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)
 }

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.")
 			}
 		})