From 96d5e67944fc79994ee23877eab97d0f0623f1a3 Mon Sep 17 00:00:00 2001 From: Christie Wilson Date: Fri, 25 Oct 2019 14:04:49 -0400 Subject: [PATCH] =?UTF-8?q?Add=20and=20fix=20various=20docstrings=20and=20?= =?UTF-8?q?comments=20=E2=9C=8D=EF=B8=8F?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit While working on #1417, which we decided in the end not to merge, I made a bunch of updates to docstrings and comments which are now in this commit: - Adding more detail to some docstrings - Adding missing docstrings - Updating docstrings that were sometimes wrong (e.g. a function name had changed) - Removing some comments in the Conditions logic that were no longer accurate - Added comments in some sections of code that took me a while to understand to hopefully make it faster next time :) --- pkg/apis/pipeline/v1alpha1/artifact_pvc.go | 16 ++++++++-------- pkg/apis/pipeline/v1alpha1/build_gcs_resource.go | 3 +-- pkg/apis/pipeline/v1alpha1/resource_types.go | 12 +++++++++++- pkg/apis/pipeline/v1alpha1/storage_resource.go | 13 ++++++++++--- .../pipelinerun/resources/conditionresolution.go | 9 +++------ .../pipelinerun/resources/input_output_steps.go | 4 +++- .../taskrun/resources/output_resource.go | 1 + 7 files changed, 37 insertions(+), 21 deletions(-) diff --git a/pkg/apis/pipeline/v1alpha1/artifact_pvc.go b/pkg/apis/pipeline/v1alpha1/artifact_pvc.go index 2a4ec48b80f..0fd7a0b9c20 100644 --- a/pkg/apis/pipeline/v1alpha1/artifact_pvc.go +++ b/pkg/apis/pipeline/v1alpha1/artifact_pvc.go @@ -28,8 +28,7 @@ var ( pvcDir = "/pvc" ) -// ArtifactPVC represents the pvc created by the pipelinerun -// for artifacts temporary storage +// ArtifactPVC represents the pvc created by the pipelinerun for artifacts temporary storage. type ArtifactPVC struct { Name string PersistentVolumeClaim *corev1.PersistentVolumeClaim @@ -37,17 +36,17 @@ type ArtifactPVC struct { BashNoopImage string } -// GetType returns the type of the artifact storage +// GetType returns the type of the artifact storage. func (p *ArtifactPVC) GetType() string { return ArtifactStoragePVCType } -// StorageBasePath returns the path to be used to store artifacts in a pipelinerun temporary storage +// StorageBasePath returns the path to be used to store artifacts in a pipelinerun temporary storage. func (p *ArtifactPVC) StorageBasePath(pr *PipelineRun) string { return pvcDir } -// GetCopyFromStorageToSteps returns a container used to download artifacts from temporary storage +// GetCopyFromStorageToSteps returns a container used to download artifacts from temporary storage. func (p *ArtifactPVC) GetCopyFromStorageToSteps(name, sourcePath, destinationPath string) []Step { return []Step{{Container: corev1.Container{ Name: names.SimpleNameGenerator.RestrictLengthWithRandomSuffix(fmt.Sprintf("source-copy-%s", name)), @@ -57,7 +56,7 @@ func (p *ArtifactPVC) GetCopyFromStorageToSteps(name, sourcePath, destinationPat }}} } -// GetCopyToStorageFromSteps returns a container used to upload artifacts for temporary storage +// GetCopyToStorageFromSteps returns a container used to upload artifacts for temporary storage. func (p *ArtifactPVC) GetCopyToStorageFromSteps(name, sourcePath, destinationPath string) []Step { return []Step{{Container: corev1.Container{ Name: names.SimpleNameGenerator.RestrictLengthWithRandomSuffix(fmt.Sprintf("source-mkdir-%s", name)), @@ -78,7 +77,7 @@ func (p *ArtifactPVC) GetCopyToStorageFromSteps(name, sourcePath, destinationPat }}} } -// GetPvcMount returns a mounting of the volume with the mount path /pvc +// GetPvcMount returns a mounting of the volume with the mount path /pvc. func GetPvcMount(name string) corev1.VolumeMount { return corev1.VolumeMount{ Name: name, // taskrun pvc name @@ -86,7 +85,8 @@ func GetPvcMount(name string) corev1.VolumeMount { } } -// CreateDirStep returns a container step to create a dir +// CreateDirStep returns a container step to create a dir at destinationPath. The name +// of the step will include name. func CreateDirStep(bashNoopImage string, name, destinationPath string) Step { return Step{Container: corev1.Container{ Name: names.SimpleNameGenerator.RestrictLengthWithRandomSuffix(fmt.Sprintf("create-dir-%s", strings.ToLower(name))), diff --git a/pkg/apis/pipeline/v1alpha1/build_gcs_resource.go b/pkg/apis/pipeline/v1alpha1/build_gcs_resource.go index 721a9bacff1..23b3d6b75d2 100644 --- a/pkg/apis/pipeline/v1alpha1/build_gcs_resource.go +++ b/pkg/apis/pipeline/v1alpha1/build_gcs_resource.go @@ -59,7 +59,6 @@ var validArtifactTypes = []GCSArtifactType{ // BuildGCSResource describes a resource in the form of an archive, // or a source manifest describing files to fetch. // BuildGCSResource does incremental uploads for files in directory. - type BuildGCSResource struct { Name string Type PipelineResourceType @@ -70,7 +69,7 @@ type BuildGCSResource struct { BuildGCSFetcherImage string `json:"-"` } -// creates a new BuildGCS resource to pass to a Task +// NewBuildGCSResource creates a new BuildGCS resource to pass to a Task. func NewBuildGCSResource(images pipeline.Images, r *PipelineResource) (*BuildGCSResource, error) { if r.Spec.Type != PipelineResourceTypeStorage { return nil, xerrors.Errorf("BuildGCSResource: Cannot create a BuildGCS resource from a %s Pipeline Resource", r.Spec.Type) diff --git a/pkg/apis/pipeline/v1alpha1/resource_types.go b/pkg/apis/pipeline/v1alpha1/resource_types.go index d798eb680de..69e5d363e87 100644 --- a/pkg/apis/pipeline/v1alpha1/resource_types.go +++ b/pkg/apis/pipeline/v1alpha1/resource_types.go @@ -54,10 +54,18 @@ var AllResourceTypes = []PipelineResourceType{PipelineResourceTypeGit, PipelineR // PipelineResourceInterface interface to be implemented by different PipelineResource types type PipelineResourceInterface interface { + // GetName returns the name of this PipelineResource instance. GetName() string + // GetType returns the type of this PipelineResource (often a super type, e.g. in the case of storage). GetType() PipelineResourceType + // Replacements returns all the attributes that this PipelineResource has that + // can be used for variable replacement. Replacements() map[string]string + // GetOutputTaskModifier returns the TaskModifier instance that should be used on a Task + // in order to add this kind of resource when it is being used as an output. GetOutputTaskModifier(ts *TaskSpec, path string) (TaskModifier, error) + // GetInputTaskModifier returns the TaskModifier instance that should be used on a Task + // in order to add this kind of resource when it is being used as an input. GetInputTaskModifier(ts *TaskSpec, path string) (TaskModifier, error) } @@ -201,7 +209,9 @@ type ResourceDeclaration struct { TargetPath string `json:"targetPath,omitempty"` } -// ResourceFromType returns a PipelineResourceInterface from a PipelineResource's type. +// ResourceFromType returns an instance of the correct PipelineResource object type which can be +// used to add input and ouput containers as well as volumes to a TaskRun's pod in order to realize +// a PipelineResource in a pod. func ResourceFromType(r *PipelineResource, images pipeline.Images) (PipelineResourceInterface, error) { switch r.Spec.Type { case PipelineResourceTypeGit: diff --git a/pkg/apis/pipeline/v1alpha1/storage_resource.go b/pkg/apis/pipeline/v1alpha1/storage_resource.go index 19e0d43012a..7b22f1f31e3 100644 --- a/pkg/apis/pipeline/v1alpha1/storage_resource.go +++ b/pkg/apis/pipeline/v1alpha1/storage_resource.go @@ -28,17 +28,24 @@ import ( type PipelineResourceStorageType string const ( - // PipelineResourceTypeGCS indicates that resource source is a GCS blob/directory. - PipelineResourceTypeGCS PipelineResourceType = "gcs" + // PipelineResourceTypeGCS is the subtype for the GCSResources, which is backed by a GCS blob/directory. + PipelineResourceTypeGCS PipelineResourceType = "gcs" + + // PipelineResourceTypeBuildGCS is the subtype for the BuildGCSResources, which is simialr to the GCSResource but + // with additional funcitonality that was added to be compatible with knative build. PipelineResourceTypeBuildGCS PipelineResourceType = "build-gcs" ) -// PipelineResourceInterface interface to be implemented by different PipelineResource types +// PipelineStorageResourceInterface is the interface for subtypes of the storage type. +// It adds a function to the PipelineResourceInterface for retrieving secrets that are usually +// needed for storage PipelineResources. type PipelineStorageResourceInterface interface { PipelineResourceInterface GetSecretParams() []SecretParam } +// NewStorageResource returns an instance of the requested storage subtype, which can be used +// to add input and output steps and volumes to an executing pod. func NewStorageResource(images pipeline.Images, r *PipelineResource) (PipelineStorageResourceInterface, error) { if r.Spec.Type != PipelineResourceTypeStorage { return nil, xerrors.Errorf("StoreResource: Cannot create a storage resource from a %s Pipeline Resource", r.Spec.Type) diff --git a/pkg/reconciler/pipelinerun/resources/conditionresolution.go b/pkg/reconciler/pipelinerun/resources/conditionresolution.go index 19e977d76b4..ed4b447f928 100644 --- a/pkg/reconciler/pipelinerun/resources/conditionresolution.go +++ b/pkg/reconciler/pipelinerun/resources/conditionresolution.go @@ -110,9 +110,7 @@ func (rcc *ResolvedConditionCheck) ConditionToTaskSpec() (*v1alpha1.TaskSpec, er }) } - // convert param strings of type $(params.x) to $(inputs.params.x) convertParamTemplates(&t.Steps[0], rcc.Condition.Spec.Params) - // convert resource strings of type $(resources.name.key) to $(inputs.resources.name.key) err := ApplyResourceSubstitution(&t.Steps[0], rcc.ResolvedResources, rcc.Condition.Spec.Resources, rcc.images) if err != nil { @@ -122,7 +120,7 @@ func (rcc *ResolvedConditionCheck) ConditionToTaskSpec() (*v1alpha1.TaskSpec, er return t, nil } -// Replaces all instances of $(params.x) in the container to $(inputs.params.x) for each param name +// convertParamTemplates replaces all instances of $(params.x) in the container to $(inputs.params.x) for each param name. func convertParamTemplates(step *v1alpha1.Step, params []v1alpha1.ParamSpec) { replacements := make(map[string]string) for _, p := range params { @@ -133,8 +131,7 @@ func convertParamTemplates(step *v1alpha1.Step, params []v1alpha1.ParamSpec) { v1alpha1.ApplyStepReplacements(step, replacements, map[string][]string{}) } -// ApplyResources applies the substitution from values in resources which are referenced -// in spec as subitems of the replacementStr. +// ApplyResourceSubstitution applies resource attribute variable substitution. func ApplyResourceSubstitution(step *v1alpha1.Step, resolvedResources map[string]*v1alpha1.PipelineResource, conditionResources []v1alpha1.ResourceDeclaration, images pipeline.Images) error { replacements := make(map[string]string) for _, cr := range conditionResources { @@ -153,7 +150,7 @@ func ApplyResourceSubstitution(step *v1alpha1.Step, resolvedResources map[string return nil } -// NewConditionCheck status creates a ConditionCheckStatus from a ConditionCheck +// NewConditionCheckStatus creates a ConditionCheckStatus from a ConditionCheck func (rcc *ResolvedConditionCheck) NewConditionCheckStatus() *v1alpha1.ConditionCheckStatus { var checkStep corev1.ContainerState trs := rcc.ConditionCheck.Status diff --git a/pkg/reconciler/pipelinerun/resources/input_output_steps.go b/pkg/reconciler/pipelinerun/resources/input_output_steps.go index 4c7a78856db..abf2ddd8cf0 100644 --- a/pkg/reconciler/pipelinerun/resources/input_output_steps.go +++ b/pkg/reconciler/pipelinerun/resources/input_output_steps.go @@ -22,7 +22,7 @@ import ( "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" ) -// GetOutputSteps will add the correct `path` to the input resources for pt +// GetOutputSteps will add the correct `path` to the output resources for pt func GetOutputSteps(outputs map[string]*v1alpha1.PipelineResource, taskName, storageBasePath string) []v1alpha1.TaskResourceBinding { var taskOutputResources []v1alpha1.TaskResourceBinding @@ -78,6 +78,8 @@ func GetInputSteps(inputs map[string]*v1alpha1.PipelineResource, pt *v1alpha1.Pi } } + // Determine if the value is meant to come `from` a previous Task - if so, add the path to the pvc + // that contains the data as the `path` the resulting TaskRun should get the data from. var stepSourceNames []string if pt.Resources != nil { for _, pipelineTaskInput := range pt.Resources.Inputs { diff --git a/pkg/reconciler/taskrun/resources/output_resource.go b/pkg/reconciler/taskrun/resources/output_resource.go index 7dcaa9dcecc..413b7e167e0 100644 --- a/pkg/reconciler/taskrun/resources/output_resource.go +++ b/pkg/reconciler/taskrun/resources/output_resource.go @@ -110,6 +110,7 @@ func AddOutputResources( } v1alpha1.ApplyTaskModifier(taskSpec, modifier) + // Attach the PVC that will be used for `from` copying. if as.GetType() == v1alpha1.ArtifactStoragePVCType { if pvcName == "" { return taskSpec, nil