Refactor input resource volume handling to remove a type switch state… #1139
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dlorenc The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
The following is the coverage report on pkg/.
|
…ment. This change refactors the way input resources attach volumes to their steps. Previously, there was a special-cased switch statement for Storage type resources. This made the logic for resources hard to follow, and spread across the codebase. This change refactors that by moving it up into the general Resource interface. This is another follow-on to tektoncd#1135
|
The following is the coverage report on pkg/.
|
| @@ -177,3 +177,7 @@ func getArtifactType(val string) (GCSArtifactType, error) { | |||
| func (s *BuildGCSResource) GetUploadVolumeSpec(spec *TaskSpec) ([]corev1.Volume, error) { | |||
| return getStorageUploadVolumeSpec(s, spec) | |||
| } | |||
|
|
|||
| func (s *BuildGCSResource) GetDownloadVolumeSpec(spec *TaskSpec) ([]corev1.Volume, error) { | |||
| return getStorageUploadVolumeSpec(s, spec) | |||
There was a problem hiding this comment.
v minor but would renaming to getStorageTransferVolumeSpec or maybe even just getStorageVolumeSpec or similar make sense given this is now also being used for the download volume?
| return nil, xerrors.Errorf("task %q invalid resource download spec: %q; error %w", taskName, boundResource.ResourceRef.Name, err) | ||
| } | ||
| } | ||
| resourceContainers, err = resource.GetDownloadContainerSpec() |
There was a problem hiding this comment.
Nice, I'm not sure why this was part of the addStorageFetchStep to begin with.
| @@ -113,25 +112,13 @@ func AddInputResource( | |||
| taskSpec.Steps = append(copyStepsFromPrevTasks, taskSpec.Steps...) | |||
| taskSpec.Volumes = append(taskSpec.Volumes, as.GetSecretsVolumes()...) | |||
| } else { | |||
| switch resource.GetType() { | |||
There was a problem hiding this comment.
So happy to see this switch go away :)
|
/lgtm |
…ment.
Changes
This change refactors the way input resources attach volumes to their steps.
Previously, there was a special-cased switch statement for Storage type resources.
This made the logic for resources hard to follow, and spread across the codebase. This
change refactors that by moving it up into the general Resource interface.
This is another follow-on to #1135
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.