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