Removed Optional field, faster volume merging

Optional field has been declared unnecessary. It has been removed
in this commit.
Previous volume merging algorithm has been implemented with
quadratic complexity, while linear is possible. Been reworked
to linear.

deploy/crds/shipwright.io_buildstrategies.yaml

@@ -1302,9 +1302,6 @@ spec:
                     name:
                       description: Name of the Build Volume
                       type: string
-                    optional:
-                      description: Whether this Volume is optional or not
-                      type: boolean
                     overridable:
                       description: Indicates that this Volume can be overrided in
                         a Build or BuildRun

deploy/crds/shipwright.io_clusterbuildstrategies.yaml

@@ -1302,9 +1302,6 @@ spec:
                     name:
                       description: Name of the Build Volume
                       type: string
-                    optional:
-                      description: Whether this Volume is optional or not
-                      type: boolean
                     overridable:
                       description: Indicates that this Volume can be overrided in
                         a Build or BuildRun

pkg/apis/build/v1alpha1/buildstrategy.go

@@ -79,10 +79,6 @@ type BuildStrategyVolume struct {
 	// +optional
 	Description *string `json:"description,omitempty"`
 
-	// Whether this Volume is optional or not
-	// +optional
-	Optional *bool `json:"optional,omitempty"`
-
 	// Indicates that this Volume can be overrided
 	// in a Build or BuildRun
 	// +optional

pkg/volumes/volumes.go

@@ -51,11 +51,8 @@ func TaskSpecVolumes(
 	}
 
 	for _, v := range volumes {
-		volumeMountExists := false
 		for _, volumeMount := range existingVolumeMounts {
 			if volumeMount.Name == v.Name {
-				volumeMountExists = true
-
 				// In case volume mount is not read only and
 				// volume type is either secret or config map,
 				// build should not be run, because this situation may lead
@@ -69,10 +66,6 @@ func TaskSpecVolumes(
 			}
 		}
 
-		if !volumeMountExists && v.Optional != nil && !*v.Optional {
-			return nil, fmt.Errorf("Volume %q is not optional and does not have corresponding Volume Mount", v.Name)
-		}
-
 		taskRunVolume := corev1.Volume{
 			Name:         v.Name,
 			VolumeSource: v.VolumeSource,
@@ -95,33 +88,37 @@ func MergeBuildVolumes(into []buildv1alpha1.BuildStrategyVolume, new []buildv1al
 		return into, nil
 	}
 
-	var result []buildv1alpha1.BuildStrategyVolume
+	mergeMap := make(map[string]buildv1alpha1.BuildStrategyVolume)
 	var errors []error
 
 	for _, vol := range into {
-		result = append(result, *vol.DeepCopy())
+		if _, ok := mergeMap[vol.Name]; ok {
+			return nil, fmt.Errorf("Build Strategy %q is listed more than once", vol.Name)
+		}
+		mergeMap[vol.Name] = *vol.DeepCopy()
 	}
 
 	for _, merger := range new {
-		found := false
-		for i, original := range into {
-			if original.Name == merger.Name {
-				// in case overridable is nil OR false (default is considered false)
-				// then return error, otherwise means it is not nil AND true
-				if original.Overridable == nil || !*original.Overridable {
-					errors = append(errors, fmt.Errorf("Cannot override BuildVolume %q", original.Name))
-					continue
-				}
-
-				found = true
-				result[i].VolumeSource = merger.VolumeSource
-				break
-			}
+		original, ok := mergeMap[merger.Name]
+		if !ok {
+			errors = append(errors, fmt.Errorf("Build Volume %q is not found in the BuildStrategy", merger.Name))
+			continue
 		}
 
-		if !found {
-			errors = append(errors, fmt.Errorf("BuildVolume %q is not found in the BuildStrategy", merger.Name))
+		// in case overridable is nil OR false (default is considered false)
+		// then return error, otherwise means it is not nil AND true
+		if original.Overridable == nil || !*original.Overridable {
+			errors = append(errors, fmt.Errorf("Cannot override BuildVolume %q", original.Name))
+			continue
 		}
+
+		original.VolumeSource = merger.VolumeSource
+		mergeMap[merger.Name] = original
+	}
+
+	result := make([]buildv1alpha1.BuildStrategyVolume, 0, len(mergeMap))
+	for _, v := range mergeMap {
+		result = append(result, v)
 	}
 
 	return result, kerrors.NewAggregate(errors)

pkg/volumes/volumes_test.go

@@ -270,12 +270,44 @@ func TestMergeVolumes(t *testing.T) {
 				t.Errorf("%s: expected error %v, got %v", td.name, td.expectErr, err)
 			}
 
-			// next check should be done only if we don't expect error in the previous check
-			if !td.expectErr && !reflect.DeepEqual(td.expected, res) {
-				resJson, _ := json.Marshal(res)
-				expJson, _ := json.Marshal(td.expected)
-				t.Errorf("%s: result merger is not equal to expected, res: %v, expected: %v", td.name, string(resJson), string(expJson))
+			// if we have been expecting err and if it happened, next checks should not be
+			// checked
+			if td.expectErr {
+				return
+			}
+
+			// volumes can be out of order, so we should convert to map, check length and then
+			// check that every expected volume exists in the actual merge result
+			volMap := toVolMap(res)
+
+			if len(volMap) != len(td.expected) {
+				t.Errorf("Length is not correct for merge result: %d, expected %d", len(volMap), len(td.expected))
+			}
+
+			for _, expectedVol := range td.expected {
+				actualVol, ok := volMap[expectedVol.Name]
+				if !ok {
+					resJson, _ := json.Marshal(res)
+					t.Errorf("Expected Volume %q not found in merge result %v", expectedVol.Name, string(resJson))
+				}
+
+				if !reflect.DeepEqual(expectedVol, actualVol) {
+					expJson, _ := json.Marshal(expectedVol)
+					actualJson, _ := json.Marshal(actualVol)
+					t.Errorf("Expected volume is not equal to actual vol, actual: %v, expected: %v",
+						string(actualJson), string(expJson))
+				}
 			}
 		})
 	}
 }
+
+func toVolMap(expected []buildv1alpha1.BuildStrategyVolume) map[string]buildv1alpha1.BuildStrategyVolume {
+	res := make(map[string]buildv1alpha1.BuildStrategyVolume)
+
+	for _, v := range expected {
+		res[v.Name] = v
+	}
+
+	return res
+}