Cleanup some code in artifact storage.

- We had some constants in two locations - this removes/dedupes these.
- We were doing some unusual string pointer manipulation- this cleans it up
a bit and adds some comments explaining why.
- Minor cleanups around set manipulation.

pkg/apis/resource/v1alpha1/storage/artifact_pvc.go

@@ -19,6 +19,7 @@ package storage
 import (
 	"fmt"
 
+	"github.com/tektoncd/pipeline/pkg/apis/pipeline"
 	"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
 	"github.com/tektoncd/pipeline/pkg/names"
 	corev1 "k8s.io/api/core/v1"
@@ -39,7 +40,7 @@ type ArtifactPVC struct {
 
 // GetType returns the type of the artifact storage.
 func (p *ArtifactPVC) GetType() string {
-	return ArtifactStoragePVCType
+	return pipeline.ArtifactStoragePVCType
 }
 
 // StorageBasePath returns the path to be used to store artifacts in a pipelinerun temporary storage.

pkg/apis/resource/v1alpha1/storage/storage.go

@@ -27,16 +27,8 @@ import (
 	corev1 "k8s.io/api/core/v1"
 )
 
-const (
-	// ArtifactStorageBucketType holds the name of the PipelineResource type for a bucket
-	ArtifactStorageBucketType = "bucket"
-
-	// ArtifactStoragePVCType holds the name of the PipelineResource type for a pvc
-	ArtifactStoragePVCType = "pvc"
-)
-
-// It adds a function to the PipelineResourceInterface for retrieving secrets that are usually
-// needed for storage PipelineResources.
+// PipelineStorageResourceInterface adds a function to the PipelineResourceInterface for retrieving
+// secrets that are usually needed for storage PipelineResources.
 type PipelineStorageResourceInterface interface {
 	v1beta1.PipelineResourceInterface
 	GetSecretParams() []resource.SecretParam
@@ -52,9 +44,9 @@ func NewResource(name string, images pipeline.Images, r *resource.PipelineResour
 	for _, param := range r.Spec.Params {
 		if strings.EqualFold(param.Name, "type") {
 			switch {
-			case strings.EqualFold(param.Value, string(resource.PipelineResourceTypeGCS)):
+			case strings.EqualFold(param.Value, resource.PipelineResourceTypeGCS):
 				return NewGCSResource(name, images, r)
-			case strings.EqualFold(param.Value, string(resource.PipelineResourceTypeBuildGCS)):
+			case strings.EqualFold(param.Value, resource.PipelineResourceTypeBuildGCS):
 				return NewBuildGCSResource(name, images, r)
 			default:
 				return nil, fmt.Errorf("%s is an invalid or unimplemented PipelineStorageResource", param.Value)
@@ -66,15 +58,19 @@ func NewResource(name string, images pipeline.Images, r *resource.PipelineResour
 
 func getStorageVolumeSpec(s PipelineStorageResourceInterface, spec v1beta1.TaskSpec) []corev1.Volume {
 	var storageVol []corev1.Volume
-	mountedSecrets := map[string]string{}
+	mountedSecrets := map[string]struct{}{}
 
 	for _, volume := range spec.Volumes {
-		mountedSecrets[volume.Name] = ""
+		mountedSecrets[volume.Name] = struct{}{}
 	}
 
 	// Map holds list of secrets that are mounted as volumes
 	for _, secretParam := range s.GetSecretParams() {
 		volName := fmt.Sprintf("volume-%s-%s", s.GetName(), secretParam.SecretName)
+		if _, ok := mountedSecrets[volName]; ok {
+			// There is already a volume mounted with this name
+			continue
+		}
 
 		gcsSecretVolume := corev1.Volume{
 			Name: volName,
@@ -84,16 +80,13 @@ func getStorageVolumeSpec(s PipelineStorageResourceInterface, spec v1beta1.TaskS
 				},
 			},
 		}
-
-		if _, ok := mountedSecrets[volName]; !ok {
-			storageVol = append(storageVol, gcsSecretVolume)
-			mountedSecrets[volName] = ""
-		}
+		storageVol = append(storageVol, gcsSecretVolume)
+		mountedSecrets[volName] = struct{}{}
 	}
 	return storageVol
 }
 
-// of the step will include name.
+// CreateDirStep returns a Step that creates a given directory with a given name.
 func CreateDirStep(shellImage string, name, destinationPath string) v1beta1.Step {
 	return v1beta1.Step{Container: corev1.Container{
 		Name:    names.SimpleNameGenerator.RestrictLengthWithRandomSuffix(fmt.Sprintf("create-dir-%s", strings.ToLower(name))),

pkg/artifacts/artifacts_storage.go

@@ -178,6 +178,12 @@ func createPVC(ctx context.Context, pr *v1beta1.PipelineRun, c kubernetes.Interf
 			if err != nil {
 				return nil, err
 			}
+
+			// The storage class name on pod spec has three states. Tekton doesn't support the empty-string case.
+			// - nil if we don't care
+			// - "" if we explicitly want to have no class names
+			// - "$name" if we want a specific name
+			// https://kubernetes.io/docs/concepts/storage/persistent-volumes/#class-1
 			var pvcStorageClassName *string
 			if pvcConfig.StorageClassName == "" {
 				pvcStorageClassName = nil