From 46394d40bf16fc0d4f9fd775b5ac02b8e1cb54bf Mon Sep 17 00:00:00 2001 From: Alice Rum Date: Fri, 8 Apr 2022 10:09:58 +0200 Subject: [PATCH] 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. --- .../crds/shipwright.io_buildstrategies.yaml | 3 -- .../shipwright.io_clusterbuildstrategies.yaml | 3 -- pkg/apis/build/v1alpha1/buildstrategy.go | 4 -- .../build/v1alpha1/zz_generated.deepcopy.go | 5 -- pkg/volumes/volumes.go | 47 +++++++++---------- pkg/volumes/volumes_test.go | 42 +++++++++++++++-- 6 files changed, 59 insertions(+), 45 deletions(-) diff --git a/deploy/crds/shipwright.io_buildstrategies.yaml b/deploy/crds/shipwright.io_buildstrategies.yaml index 3fe67b182d..f4c3e8aded 100644 --- a/deploy/crds/shipwright.io_buildstrategies.yaml +++ b/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 diff --git a/deploy/crds/shipwright.io_clusterbuildstrategies.yaml b/deploy/crds/shipwright.io_clusterbuildstrategies.yaml index 29cf8e4353..046d1fa405 100644 --- a/deploy/crds/shipwright.io_clusterbuildstrategies.yaml +++ b/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 diff --git a/pkg/apis/build/v1alpha1/buildstrategy.go b/pkg/apis/build/v1alpha1/buildstrategy.go index 82bee7cb6b..b48803b50e 100644 --- a/pkg/apis/build/v1alpha1/buildstrategy.go +++ b/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 diff --git a/pkg/apis/build/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/build/v1alpha1/zz_generated.deepcopy.go index 4b125c66ab..153c6fce3e 100644 --- a/pkg/apis/build/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/build/v1alpha1/zz_generated.deepcopy.go @@ -548,11 +548,6 @@ func (in *BuildStrategyVolume) DeepCopyInto(out *BuildStrategyVolume) { *out = new(string) **out = **in } - if in.Optional != nil { - in, out := &in.Optional, &out.Optional - *out = new(bool) - **out = **in - } if in.Overridable != nil { in, out := &in.Overridable, &out.Overridable *out = new(bool) diff --git a/pkg/volumes/volumes.go b/pkg/volumes/volumes.go index 6b57e06fcd..49814fb8a7 100644 --- a/pkg/volumes/volumes.go +++ b/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) diff --git a/pkg/volumes/volumes_test.go b/pkg/volumes/volumes_test.go index 6924f2fa2e..4feb56dd47 100644 --- a/pkg/volumes/volumes_test.go +++ b/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 +}