From b4f3efd939a4f6606735a2de7c9b789a539d43a2 Mon Sep 17 00:00:00 2001 From: Dan Lorenc Date: Sat, 18 Jul 2020 19:56:12 -0500 Subject: [PATCH] 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. --- .../resource/v1alpha1/storage/artifact_pvc.go | 3 +- pkg/apis/resource/v1alpha1/storage/storage.go | 33 ++++++++----------- pkg/artifacts/artifacts_storage.go | 6 ++++ 3 files changed, 21 insertions(+), 21 deletions(-) diff --git a/pkg/apis/resource/v1alpha1/storage/artifact_pvc.go b/pkg/apis/resource/v1alpha1/storage/artifact_pvc.go index 02bdbaec932..edf32b1030e 100644 --- a/pkg/apis/resource/v1alpha1/storage/artifact_pvc.go +++ b/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. diff --git a/pkg/apis/resource/v1alpha1/storage/storage.go b/pkg/apis/resource/v1alpha1/storage/storage.go index e3c0f445ca6..54a339951c6 100644 --- a/pkg/apis/resource/v1alpha1/storage/storage.go +++ b/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))), diff --git a/pkg/artifacts/artifacts_storage.go b/pkg/artifacts/artifacts_storage.go index 1b1223ad80a..55f0017167a 100644 --- a/pkg/artifacts/artifacts_storage.go +++ b/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