Skip to content

Commit 63411f6

Browse files
maysunfaisalmaysunfaisal
committed
Update with PR feedback
Signed-off-by: Maysun J Faisal <[email protected]>
1 parent 74c6802 commit 63411f6

File tree

4 files changed

+82
-107
lines changed

4 files changed

+82
-107
lines changed

‎pkg/devfile/generator/generators.go

+11
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
"k8s.io/apimachinery/pkg/api/resource"
1111
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1212

13+
v1 "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2"
1314
"github.com/devfile/library/pkg/devfile/parser"
1415
"github.com/devfile/library/pkg/devfile/parser/data/v2/common"
1516
)
@@ -326,3 +327,13 @@ func GetVolumesAndVolumeMounts(devfileObj parser.DevfileObj, volumeParams Volume
326327
}
327328
return pvcVols, nil
328329
}
330+
331+
// GetVolumeMountPath gets the volume mount's path.
332+
func GetVolumeMountPath(volumeMount v1.VolumeMount) string {
333+
// if there is no volume mount path, default to volume mount name as per devfile schema
334+
if volumeMount.Path == "" {
335+
volumeMount.Path = "/" + volumeMount.Name
336+
}
337+
338+
return volumeMount.Path
339+
}

‎pkg/devfile/generator/generators_test.go

+63-34
Original file line numberDiff line numberDiff line change
@@ -417,50 +417,79 @@ func TestGetVolumesAndVolumeMounts(t *testing.T) {
417417
}
418418

419419
pvcVols, err := GetVolumesAndVolumeMounts(devObj, volumeParams, options)
420-
if !tt.wantErr && err != nil {
421-
t.Errorf("TestGetVolumesAndVolumeMounts unexpected error: %v", err)
422-
return
423-
} else if tt.wantErr && err != nil {
424-
return
425-
} else if tt.wantErr && err == nil {
426-
t.Error("TestGetVolumesAndVolumeMounts expected error but got nil")
427-
return
428-
}
429-
430-
// check if the pvc volumes returned are correct
431-
for _, volInfo := range tt.volumeNameToVolInfo {
432-
matched := false
433-
for _, pvcVol := range pvcVols {
434-
if volInfo.VolumeName == pvcVol.Name && pvcVol.PersistentVolumeClaim != nil && volInfo.PVCName == pvcVol.PersistentVolumeClaim.ClaimName {
435-
matched = true
420+
if tt.wantErr == (err == nil) {
421+
t.Errorf("TestGetVolumesAndVolumeMounts() error = %v, wantErr %v", err, tt.wantErr)
422+
} else if err == nil {
423+
// check if the pvc volumes returned are correct
424+
for _, volInfo := range tt.volumeNameToVolInfo {
425+
matched := false
426+
for _, pvcVol := range pvcVols {
427+
if volInfo.VolumeName == pvcVol.Name && pvcVol.PersistentVolumeClaim != nil && volInfo.PVCName == pvcVol.PersistentVolumeClaim.ClaimName {
428+
matched = true
429+
}
436430
}
437-
}
438431

439-
if !matched {
440-
t.Errorf("TestGetVolumesAndVolumeMounts error - could not find volume details %s in the actual result", volInfo.VolumeName)
432+
if !matched {
433+
t.Errorf("TestGetVolumesAndVolumeMounts error - could not find volume details %s in the actual result", volInfo.VolumeName)
434+
}
441435
}
442-
}
443436

444-
// check the volume mounts of the containers
445-
for _, container := range containers {
446-
if volMounts, ok := tt.wantContainerToVol[container.Name]; !ok {
447-
t.Errorf("TestGetVolumesAndVolumeMounts error - did not find the expected container %s", container.Name)
448-
return
449-
} else {
450-
for _, expectedVolMount := range volMounts {
451-
matched := false
452-
for _, actualVolMount := range container.VolumeMounts {
453-
if expectedVolMount.volumeName == actualVolMount.Name && expectedVolMount.mountPath == actualVolMount.MountPath {
454-
matched = true
437+
// check the volume mounts of the containers
438+
for _, container := range containers {
439+
if volMounts, ok := tt.wantContainerToVol[container.Name]; !ok {
440+
t.Errorf("TestGetVolumesAndVolumeMounts error - did not find the expected container %s", container.Name)
441+
return
442+
} else {
443+
for _, expectedVolMount := range volMounts {
444+
matched := false
445+
for _, actualVolMount := range container.VolumeMounts {
446+
if expectedVolMount.volumeName == actualVolMount.Name && expectedVolMount.mountPath == actualVolMount.MountPath {
447+
matched = true
448+
}
455449
}
456-
}
457450

458-
if !matched {
459-
t.Errorf("TestGetVolumesAndVolumeMounts error - could not find volume mount details for path %s in the actual result for container %s", expectedVolMount.mountPath, container.Name)
451+
if !matched {
452+
t.Errorf("TestGetVolumesAndVolumeMounts error - could not find volume mount details for path %s in the actual result for container %s", expectedVolMount.mountPath, container.Name)
453+
}
460454
}
461455
}
462456
}
463457
}
464458
})
465459
}
466460
}
461+
462+
func TestGetVolumeMountPath(t *testing.T) {
463+
464+
tests := []struct {
465+
name string
466+
volumeMount v1.VolumeMount
467+
wantPath string
468+
}{
469+
{
470+
name: "Mount Path is present",
471+
volumeMount: v1.VolumeMount{
472+
Name: "name1",
473+
Path: "/path1",
474+
},
475+
wantPath: "/path1",
476+
},
477+
{
478+
name: "Mount Path is absent",
479+
volumeMount: v1.VolumeMount{
480+
Name: "name1",
481+
},
482+
wantPath: "/name1",
483+
},
484+
}
485+
for _, tt := range tests {
486+
t.Run(tt.name, func(t *testing.T) {
487+
path := GetVolumeMountPath(tt.volumeMount)
488+
489+
if path != tt.wantPath {
490+
t.Errorf("TestGetVolumeMountPath error: mount path mismatch, expected: %v got: %v", tt.wantPath, path)
491+
}
492+
})
493+
}
494+
495+
}

‎pkg/devfile/generator/utils.go

-11
Original file line numberDiff line numberDiff line change
@@ -420,16 +420,6 @@ func getBuildConfigSpec(buildConfigSpecParams BuildConfigSpecParams) *buildv1.Bu
420420
}
421421
}
422422

423-
// GetVolumeMountPath gets the volume mount's path.
424-
func GetVolumeMountPath(volumeMount v1.VolumeMount) string {
425-
// if there is no volume mount path, default to volume mount name as per devfile schema
426-
if volumeMount.Path == "" {
427-
volumeMount.Path = "/" + volumeMount.Name
428-
}
429-
430-
return volumeMount.Path
431-
}
432-
433423
// getPVC gets a pvc type volume with the given volume name and pvc name.
434424
func getPVC(volumeName, pvcName string) corev1.Volume {
435425

@@ -454,7 +444,6 @@ func addVolumeMountToContainers(containers []corev1.Container, volumeName string
454444
containers[i].VolumeMounts = append(containers[i].VolumeMounts, corev1.VolumeMount{
455445
Name: volumeName,
456446
MountPath: mountPath,
457-
SubPath: "",
458447
},
459448
)
460449
}

‎pkg/devfile/generator/utils_test.go

+8-62
Original file line numberDiff line numberDiff line change
@@ -1311,48 +1311,15 @@ func TestGetBuildConfigSpec(t *testing.T) {
13111311

13121312
}
13131313

1314-
func TestGetVolumeMountPath(t *testing.T) {
1315-
1316-
tests := []struct {
1317-
name string
1318-
volumeMount v1.VolumeMount
1319-
wantPath string
1320-
}{
1321-
{
1322-
name: "Mount Path is present",
1323-
volumeMount: v1.VolumeMount{
1324-
Name: "name1",
1325-
Path: "/path1",
1326-
},
1327-
wantPath: "/path1",
1328-
},
1329-
{
1330-
name: "Mount Path is absent",
1331-
volumeMount: v1.VolumeMount{
1332-
Name: "name1",
1333-
},
1334-
wantPath: "/name1",
1335-
},
1336-
}
1337-
for _, tt := range tests {
1338-
t.Run(tt.name, func(t *testing.T) {
1339-
path := GetVolumeMountPath(tt.volumeMount)
1340-
1341-
if path != tt.wantPath {
1342-
t.Errorf("TestGetVolumeMountPath error: mount path mismatch, expected: %v got: %v", tt.wantPath, path)
1343-
}
1344-
})
1345-
}
1346-
1347-
}
1348-
13491314
func TestGetPVC(t *testing.T) {
13501315

13511316
tests := []struct {
1317+
name string
13521318
pvc string
13531319
volumeName string
13541320
}{
13551321
{
1322+
name: "Get PVC vol for given pvc name and volume name",
13561323
pvc: "mypvc",
13571324
volumeName: "myvolume",
13581325
},
@@ -1376,22 +1343,14 @@ func TestGetPVC(t *testing.T) {
13761343
func TestAddVolumeMountToContainers(t *testing.T) {
13771344

13781345
tests := []struct {
1379-
podName string
1380-
namespace string
1381-
serviceAccount string
1382-
pvc string
1346+
name string
13831347
volumeName string
13841348
containerMountPathsMap map[string][]string
13851349
container corev1.Container
1386-
labels map[string]string
1387-
wantErr bool
13881350
}{
13891351
{
1390-
podName: "podSpecTest",
1391-
namespace: "default",
1392-
serviceAccount: "default",
1393-
pvc: "mypvc",
1394-
volumeName: "myvolume",
1352+
name: "Successfully mount volume to container",
1353+
volumeName: "myvolume",
13951354
containerMountPathsMap: map[string][]string{
13961355
"container1": {"/tmp/path1", "/tmp/path2"},
13971356
},
@@ -1404,32 +1363,19 @@ func TestAddVolumeMountToContainers(t *testing.T) {
14041363
Args: []string{"-f", "/dev/null"},
14051364
Env: []corev1.EnvVar{},
14061365
},
1407-
labels: map[string]string{
1408-
"app": "app",
1409-
"component": "frontend",
1410-
},
1411-
wantErr: false,
14121366
},
14131367
{
1414-
podName: "podSpecTest",
1415-
namespace: "default",
1416-
serviceAccount: "default",
1417-
pvc: "mypvc",
1418-
volumeName: "myvolume",
1368+
name: "No Container present to mount volume",
1369+
volumeName: "myvolume",
14191370
containerMountPathsMap: map[string][]string{
14201371
"container1": {"/tmp/path1", "/tmp/path2"},
14211372
},
14221373
container: corev1.Container{},
1423-
labels: map[string]string{
1424-
"app": "app",
1425-
"component": "frontend",
1426-
},
1427-
wantErr: true,
14281374
},
14291375
}
14301376

14311377
for _, tt := range tests {
1432-
t.Run(tt.podName, func(t *testing.T) {
1378+
t.Run(tt.name, func(t *testing.T) {
14331379
containers := []corev1.Container{tt.container}
14341380
addVolumeMountToContainers(containers, tt.volumeName, tt.containerMountPathsMap)
14351381

0 commit comments

Comments
 (0)