From c8831b35760e97abd9da725fe29f6aa48cbfe870 Mon Sep 17 00:00:00 2001 From: Dan Lorenc Date: Wed, 31 Jul 2019 09:30:56 -0500 Subject: [PATCH] Refactor input resource volume handling to remove a type switch statement. 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 https://github.com/tektoncd/pipeline/pull/1135 --- .../pipeline/v1alpha1/build_gcs_resource.go | 4 ++ .../pipeline/v1alpha1/cluster_resource.go | 4 ++ pkg/apis/pipeline/v1alpha1/gcs_resource.go | 4 ++ pkg/apis/pipeline/v1alpha1/git_resource.go | 4 ++ pkg/apis/pipeline/v1alpha1/image_resource.go | 4 ++ .../v1alpha1/pull_request_resource.go | 4 ++ pkg/apis/pipeline/v1alpha1/resource_types.go | 1 + .../taskrun/resources/input_resources.go | 59 +++---------------- 8 files changed, 32 insertions(+), 52 deletions(-) diff --git a/pkg/apis/pipeline/v1alpha1/build_gcs_resource.go b/pkg/apis/pipeline/v1alpha1/build_gcs_resource.go index 27996e4af5f..c8af61bec3d 100644 --- a/pkg/apis/pipeline/v1alpha1/build_gcs_resource.go +++ b/pkg/apis/pipeline/v1alpha1/build_gcs_resource.go @@ -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) +} diff --git a/pkg/apis/pipeline/v1alpha1/cluster_resource.go b/pkg/apis/pipeline/v1alpha1/cluster_resource.go index 931310296b9..12556cacd1f 100644 --- a/pkg/apis/pipeline/v1alpha1/cluster_resource.go +++ b/pkg/apis/pipeline/v1alpha1/cluster_resource.go @@ -177,3 +177,7 @@ func (s *ClusterResource) GetDownloadContainerSpec() ([]corev1.Container, error) func (s *ClusterResource) GetUploadVolumeSpec(spec *TaskSpec) ([]corev1.Volume, error) { return nil, nil } + +func (s *ClusterResource) GetDownloadVolumeSpec(spec *TaskSpec) ([]corev1.Volume, error) { + return nil, nil +} diff --git a/pkg/apis/pipeline/v1alpha1/gcs_resource.go b/pkg/apis/pipeline/v1alpha1/gcs_resource.go index 30cb2ee9ba9..e54eb3b7749 100644 --- a/pkg/apis/pipeline/v1alpha1/gcs_resource.go +++ b/pkg/apis/pipeline/v1alpha1/gcs_resource.go @@ -154,3 +154,7 @@ func (s *GCSResource) GetDownloadContainerSpec() ([]corev1.Container, error) { func (s *GCSResource) GetUploadVolumeSpec(spec *TaskSpec) ([]corev1.Volume, error) { return getStorageUploadVolumeSpec(s, spec) } + +func (s *GCSResource) GetDownloadVolumeSpec(spec *TaskSpec) ([]corev1.Volume, error) { + return getStorageUploadVolumeSpec(s, spec) +} diff --git a/pkg/apis/pipeline/v1alpha1/git_resource.go b/pkg/apis/pipeline/v1alpha1/git_resource.go index 046aa4839b2..287f75bc57c 100644 --- a/pkg/apis/pipeline/v1alpha1/git_resource.go +++ b/pkg/apis/pipeline/v1alpha1/git_resource.go @@ -124,3 +124,7 @@ func (s *GitResource) GetUploadContainerSpec() ([]corev1.Container, error) { func (s *GitResource) GetUploadVolumeSpec(spec *TaskSpec) ([]corev1.Volume, error) { return nil, nil } + +func (s *GitResource) GetDownloadVolumeSpec(spec *TaskSpec) ([]corev1.Volume, error) { + return nil, nil +} diff --git a/pkg/apis/pipeline/v1alpha1/image_resource.go b/pkg/apis/pipeline/v1alpha1/image_resource.go index e25e428dba7..d3df939ee24 100644 --- a/pkg/apis/pipeline/v1alpha1/image_resource.go +++ b/pkg/apis/pipeline/v1alpha1/image_resource.go @@ -105,3 +105,7 @@ func (s ImageResource) String() string { func (s *ImageResource) GetUploadVolumeSpec(spec *TaskSpec) ([]corev1.Volume, error) { return nil, nil } + +func (s *ImageResource) GetDownloadVolumeSpec(spec *TaskSpec) ([]corev1.Volume, error) { + return nil, nil +} diff --git a/pkg/apis/pipeline/v1alpha1/pull_request_resource.go b/pkg/apis/pipeline/v1alpha1/pull_request_resource.go index abd8b7ed8d1..b012d736561 100644 --- a/pkg/apis/pipeline/v1alpha1/pull_request_resource.go +++ b/pkg/apis/pipeline/v1alpha1/pull_request_resource.go @@ -142,3 +142,7 @@ func (s *PullRequestResource) SetDestinationDirectory(dir string) { func (s *PullRequestResource) GetUploadVolumeSpec(spec *TaskSpec) ([]corev1.Volume, error) { return nil, nil } + +func (s *PullRequestResource) GetDownloadVolumeSpec(spec *TaskSpec) ([]corev1.Volume, error) { + return nil, nil +} diff --git a/pkg/apis/pipeline/v1alpha1/resource_types.go b/pkg/apis/pipeline/v1alpha1/resource_types.go index 3e1e3def832..c47a37c8aa3 100644 --- a/pkg/apis/pipeline/v1alpha1/resource_types.go +++ b/pkg/apis/pipeline/v1alpha1/resource_types.go @@ -56,6 +56,7 @@ type PipelineResourceInterface interface { GetDownloadContainerSpec() ([]corev1.Container, error) GetUploadContainerSpec() ([]corev1.Container, error) GetUploadVolumeSpec(spec *TaskSpec) ([]corev1.Volume, error) + GetDownloadVolumeSpec(spec *TaskSpec) ([]corev1.Volume, error) SetDestinationDirectory(string) } diff --git a/pkg/reconciler/v1alpha1/taskrun/resources/input_resources.go b/pkg/reconciler/v1alpha1/taskrun/resources/input_resources.go index a40577dd791..495e225c9ab 100644 --- a/pkg/reconciler/v1alpha1/taskrun/resources/input_resources.go +++ b/pkg/reconciler/v1alpha1/taskrun/resources/input_resources.go @@ -17,7 +17,6 @@ limitations under the License. package resources import ( - "fmt" "path/filepath" "github.com/tektoncd/pipeline/pkg/apis/pipeline" @@ -113,25 +112,13 @@ func AddInputResource( taskSpec.Steps = append(copyStepsFromPrevTasks, taskSpec.Steps...) taskSpec.Volumes = append(taskSpec.Volumes, as.GetSecretsVolumes()...) } else { - switch resource.GetType() { - case v1alpha1.PipelineResourceTypeStorage: - { - storageResource, ok := resource.(v1alpha1.PipelineStorageResourceInterface) - if !ok { - return nil, xerrors.Errorf("task %q invalid gcs Pipeline Resource: %q", taskName, boundResource.ResourceRef.Name) - } - resourceContainers, resourceVolumes, err = addStorageFetchStep(taskSpec, storageResource) - if err != nil { - return nil, xerrors.Errorf("task %q invalid gcs Pipeline Resource download steps: %q: %w", taskName, boundResource.ResourceRef.Name, err) - } - } - default: - { - resourceContainers, err = resource.GetDownloadContainerSpec() - if err != nil { - return nil, xerrors.Errorf("task %q invalid resource download spec: %q; error %w", taskName, boundResource.ResourceRef.Name, err) - } - } + resourceContainers, err = resource.GetDownloadContainerSpec() + if err != nil { + return nil, xerrors.Errorf("task %q invalid resource download spec: %q; error %w", taskName, boundResource.ResourceRef.Name, err) + } + resourceVolumes, err = resource.GetDownloadVolumeSpec(taskSpec) + if err != nil { + return nil, xerrors.Errorf("task %q invalid resource download spec: %q; error %w", taskName, boundResource.ResourceRef.Name, err) } allResourceContainers = append(allResourceContainers, resourceContainers...) @@ -146,38 +133,6 @@ func AddInputResource( return taskSpec, nil } -func addStorageFetchStep(taskSpec *v1alpha1.TaskSpec, storageResource v1alpha1.PipelineStorageResourceInterface) ([]corev1.Container, []corev1.Volume, error) { - gcsContainers, err := storageResource.GetDownloadContainerSpec() - if err != nil { - return nil, nil, err - } - - var storageVol []corev1.Volume - mountedSecrets := map[string]string{} - for _, volume := range taskSpec.Volumes { - mountedSecrets[volume.Name] = "" - } - - for _, secretParam := range storageResource.GetSecretParams() { - volName := fmt.Sprintf("volume-%s-%s", storageResource.GetName(), secretParam.SecretName) - - gcsSecretVolume := corev1.Volume{ - Name: volName, - VolumeSource: corev1.VolumeSource{ - Secret: &corev1.SecretVolumeSource{ - SecretName: secretParam.SecretName, - }, - }, - } - - if _, ok := mountedSecrets[volName]; !ok { - storageVol = append(storageVol, gcsSecretVolume) - mountedSecrets[volName] = "" - } - } - return gcsContainers, storageVol, nil -} - func getResource(r *v1alpha1.TaskResourceBinding, getter GetResource) (*v1alpha1.PipelineResource, error) { // Check both resource ref or resource Spec are not present. Taskrun webhook should catch this in validation error. if r.ResourceRef.Name != "" && r.ResourceSpec != nil {