Update build-gcs resource type

- support both `ZipArchive` and `TarGzArchive` types now supported by `gcs-fetcher` - mark `Archive` type as deprecated ❌ (`ZipArchive` is the equivalent) - rename `gcs-resource-spec-taskrun.yaml` -> `build-gcs-{targz,zip}.yaml` to make their purposes clearer - make the taskrun name more descriptive - fetch a smaller file (`archive.{zip,tar.gz}`) containing a single file for faster tests ⏩ - remove `test/gcs_taskrun_test.go` which didn't usefully test anything that wasn't tested in the YAML test - update docs to reflect these changes 📜 - explicitly set `gcs-fetcher` image entrypoint And, while I'm at it: - remove support for `build-gcs` upload functionality - this doesn't appear to have worked and AFAIK nobody used it anyway - if we want to support it, we can, but probably not like that... it only supported `Manifest`-style uploads, for instance 🌵 - remove `gcs-uploader` image from flags, and from `vendor/` and from `tekton/` release pipeline YAMLs 🎉
tektoncd · Aug 16, 2019 · fd6f1e6 · fd6f1e6
1 parent f41f8dd
commit fd6f1e6
Show file tree

Hide file tree

Showing 18 changed files with 537 additions and 547 deletions.
diff --git a/Gopkg.lock b/Gopkg.lock
diff --git a/Gopkg.toml b/Gopkg.toml
@@ -3,7 +3,6 @@
 
 required = [
   "github.com/GoogleCloudPlatform/cloud-builders/gcs-fetcher/cmd/gcs-fetcher",
-  "github.com/GoogleCloudPlatform/cloud-builders/gcs-fetcher/cmd/gcs-uploader",
   "github.com/knative/test-infra/tools/dep-collector",
   "github.com/tektoncd/plumbing/scripts",
   "k8s.io/apimachinery/pkg/util/sets/types",

diff --git a/config/controller.yaml b/config/controller.yaml
@@ -42,7 +42,6 @@ spec:
           "-imagedigest-exporter-image", "github.com/tektoncd/pipeline/cmd/imagedigestexporter",
           "-pr-image", "github.com/tektoncd/pipeline/cmd/pullrequest-init",
           "-build-gcs-fetcher-image", "github.com/tektoncd/pipeline/vendor/github.com/GoogleCloudPlatform/cloud-builders/gcs-fetcher/cmd/gcs-fetcher",
-          "-build-gcs-uploader-image", "github.com/tektoncd/pipeline/vendor/github.com/GoogleCloudPlatform/cloud-builders/gcs-fetcher/cmd/gcs-uploader",
         ]
         volumeMounts:
         - name: config-logging

diff --git a/docs/resources.md b/docs/resources.md
@@ -590,18 +590,15 @@ Params that can be added are the following:
 1.  `artifactType`: represent the type of GCS resource. Right now, we support
     following types:
 
-    -   `Archive`:
-        -   Archive indicates that resource fetched is an archive file.
-            Currently, Build GCS resource only supports `.zip` archive.
-        -   It unzips the archive and places all the files in the directory
-            which is set at runtime.
-        -   If `artifactType` is set to `Archive`, `location` should point to a
-            `.zip` file.
-    -   [`Manifest`](https://github.com/GoogleCloudPlatform/cloud-builders/tree/master/gcs-fetcher#source-manifests):
-        -   Manifest indicates that resource should be fetched using a source
-            manifest file.
-        -   If `artifactType` is set to `Manifest`, `location` should point to a
-            source manifest file.
+* `ZipArchive`:
+  * ZipArchive indicates that resource fetched is an archive file in the zip format.
+  * It unzips the archive and places all the files in the directory which is set at runtime.
+  * `Archive` is also supported and is equivalent to `ZipArchive`, but is deprecated.
+* `TarGzArchive`:
+  * TarGzArchive indicates that resource fetched is a gzipped archive file in the tar format.
+  * It unzips the archive and places all the files in the directory which is set at runtime.
+* [`Manifest`](https://github.com/GoogleCloudPlatform/cloud-builders/tree/master/gcs-fetcher#source-manifests):
+  * Manifest indicates that resource should be fetched using a source manifest file.
 
 Private buckets other than ones accessible by
 [TaskRun Service Account](./taskruns.md#service-account) can not be configured

diff --git a/...s/taskruns/gcs-resource-spec-taskrun.yaml → examples/taskruns/build-gcs-targz.yaml b/...s/taskruns/gcs-resource-spec-taskrun.yaml → examples/taskruns/build-gcs-targz.yaml
@@ -1,27 +1,25 @@
 apiVersion: tekton.dev/v1alpha1
 kind: TaskRun
 metadata:
-  name: list-file
+  name: build-gcs-targz
 spec:
   taskSpec:
     inputs:
       resources:
-      - name: rules
+      - name: source
         type: storage
     steps:
-    - name: list
-      image: ubuntu
-      command: ["/bin/bash"]
-      args: ['-c', 'ls -al /workspace/rules/rules_docker-master'] # tests build-gcs resource
+    - image: ubuntu
+      command: ['cat', 'source/file.txt']  # tests build-gcs resource
   inputs:
     resources:
-    - name: rules
+    - name: source
       resourceSpec:
         type: storage
         params:
           - name: location
-            value: gs://build-crd-tests/rules_docker-master.zip
+            value: gs://build-crd-tests/archive.tar.gz
           - name: artifactType
-            value: Archive
+            value: TarGzArchive
           - name: type
             value: build-gcs
diff --git a/examples/taskruns/build-gcs-zip.yaml b/examples/taskruns/build-gcs-zip.yaml
@@ -0,0 +1,25 @@
+apiVersion: tekton.dev/v1alpha1
+kind: TaskRun
+metadata:
+  name: build-gcs-zip
+spec:
+  taskSpec:
+    inputs:
+      resources:
+      - name: source
+        type: storage
+    steps:
+    - image: ubuntu
+      command: ['cat', 'source/file.txt']  # tests build-gcs resource
+  inputs:
+    resources:
+    - name: source
+      resourceSpec:
+        type: storage
+        params:
+          - name: location
+            value: gs://build-crd-tests/archive.zip
+          - name: artifactType
+            value: ZipArchive
+          - name: type
+            value: build-gcs
diff --git a/pkg/apis/pipeline/v1alpha1/build_gcs_resource.go b/pkg/apis/pipeline/v1alpha1/build_gcs_resource.go
@@ -29,25 +29,38 @@ import (
 var (
 	buildGCSFetcherImage = flag.String("build-gcs-fetcher-image", "gcr.io/cloud-builders/gcs-fetcher:latest",
 		"The container image containing our GCS fetcher binary.")
-	buildGCSUploaderImage = flag.String("build-gcs-uploader-image", "gcr.io/cloud-builders/gcs-uploader:latest",
-		"The container image containing our GCS uploader binary.")
 )
 
 // GCSArtifactType defines a type of GCS resource.
 type GCSArtifactType string
 
 const (
-	// GCSArchive indicates that resource should be fetched from a typical archive file.
+	// GCSZipArchive indicates that the resource should be fetched and
+	// extracted as a .zip file.
+	//
+	// Deprecated: Use GCSZipArchive instead.
 	GCSArchive GCSArtifactType = "Archive"
 
+	// GCSZipArchive indicates that the resource should be fetched and
+	// extracted as a .zip file.
+	GCSZipArchive GCSArtifactType = "ZipArchive"
+
+	// GCSTarGzArchive indicates that the resource should be fetched and
+	// extracted as a .tar.gz file.
+	GCSTarGzArchive GCSArtifactType = "TarGzArchive"
+
 	// GCSManifest indicates that resource should be fetched using a
 	// manifest-based protocol which enables incremental source upload.
 	GCSManifest GCSArtifactType = "Manifest"
-
-	// EmptyArtifactType indicates, no artifact type is specified.
-	EmptyArtifactType = ""
 )
 
+var validArtifactTypes = []GCSArtifactType{
+	GCSArchive,
+	GCSManifest,
+	GCSZipArchive,
+	GCSTarGzArchive,
+}
+
 // 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.
@@ -69,7 +82,6 @@ func NewBuildGCSResource(r *PipelineResource) (*BuildGCSResource, error) {
 	}
 	var location string
 	var aType GCSArtifactType
-
 	for _, param := range r.Spec.Params {
 		switch {
 		case strings.EqualFold(param.Name, "Location"):
@@ -85,8 +97,8 @@ func NewBuildGCSResource(r *PipelineResource) (*BuildGCSResource, error) {
 	if location == "" {
 		return nil, xerrors.Errorf("BuildGCSResource: Need Location to be specified in order to create BuildGCS resource %s", r.Name)
 	}
-	if aType == EmptyArtifactType {
-		return nil, xerrors.Errorf("BuildGCSResource: Need ArtifactType to be specified in order to fetch BuildGCS resource %s", r.Name)
+	if aType == GCSArtifactType("") {
+		return nil, xerrors.Errorf("BuildGCSResource: Need ArtifactType to be specified to create BuildGCS resource %s", r.Name)
 	}
 	return &BuildGCSResource{
 		Name:         r.Name,
@@ -96,18 +108,11 @@ func NewBuildGCSResource(r *PipelineResource) (*BuildGCSResource, error) {
 	}, nil
 }
 
-// GetName returns the name of the resource
-func (s BuildGCSResource) GetName() string {
-	return s.Name
-}
-
-// GetType returns the type of the resource, in this case "storage"
-func (s BuildGCSResource) GetType() PipelineResourceType {
-	return PipelineResourceTypeStorage
-}
-
-// GetSecretParams returns the resource secret params
-func (s *BuildGCSResource) GetSecretParams() []SecretParam { return nil }
+func (s BuildGCSResource) GetName() string                                         { return s.Name }
+func (s BuildGCSResource) GetType() PipelineResourceType                           { return PipelineResourceTypeStorage }
+func (s *BuildGCSResource) GetSecretParams() []SecretParam                         { return nil }
+func (s *BuildGCSResource) GetUploadSteps(string) ([]Step, error)                  { return nil, nil }
+func (s *BuildGCSResource) GetUploadVolumeSpec(*TaskSpec) ([]corev1.Volume, error) { return nil, nil }
 
 // Replacements is used for template replacement on an GCSResource inside of a Taskrun.
 func (s *BuildGCSResource) Replacements() map[string]string {
@@ -129,48 +134,23 @@ func (s *BuildGCSResource) GetDownloadSteps(sourcePath string) ([]Step, error) {
 	return []Step{
 		CreateDirStep(s.Name, sourcePath),
 		{Container: corev1.Container{
-			Name:  names.SimpleNameGenerator.RestrictLengthWithRandomSuffix(fmt.Sprintf("storage-fetch-%s", s.Name)),
-			Image: *buildGCSFetcherImage,
-			Args:  args,
+			Name:    names.SimpleNameGenerator.RestrictLengthWithRandomSuffix(fmt.Sprintf("storage-fetch-%s", s.Name)),
+			Command: []string{"/ko-app/gcs-fetcher"},
+			Image:   *buildGCSFetcherImage,
+			Args:    args,
 		}}}, nil
 }
 
-// GetUploadSteps gets container spec for gcs resource to be uploaded like
-// set environment variable from secret params and set volume mounts for those secrets
-func (s *BuildGCSResource) GetUploadSteps(sourcePath string) ([]Step, error) {
-	if s.ArtifactType != GCSManifest {
-		return nil, xerrors.Errorf("BuildGCSResource: Can only upload Artifacts of type Manifest: %s", s.Name)
-	}
-	if sourcePath == "" {
-		return nil, xerrors.Errorf("BuildGCSResource: Expect Destination Directory param to be set %s", s.Name)
-	}
-	args := []string{"--location", s.Location, "--dir", sourcePath}
-
-	return []Step{{Container: corev1.Container{
-		Name:  names.SimpleNameGenerator.RestrictLengthWithRandomSuffix(fmt.Sprintf("storage-upload-%s", s.Name)),
-		Image: *buildGCSUploaderImage,
-		Args:  args,
-	}}}, nil
-}
-
 func getArtifactType(val string) (GCSArtifactType, error) {
-	aType := GCSArtifactType(val)
-	valid := []string{string(GCSArchive), string(GCSManifest)}
-	switch aType {
-	case GCSArchive:
-		return aType, nil
-	case GCSManifest:
-		return aType, nil
-	case EmptyArtifactType:
-		return "", xerrors.Errorf("ArtifactType is empty. Should be one of %s", strings.Join(valid, ","))
+	a := GCSArtifactType(val)
+	for _, v := range validArtifactTypes {
+		if a == v {
+			return a, nil
+		}
 	}
-	return "", xerrors.Errorf("Invalid ArtifactType %s. Should be one of %s", aType, strings.Join(valid, ","))
-}
-
-func (s *BuildGCSResource) GetUploadVolumeSpec(spec *TaskSpec) ([]corev1.Volume, error) {
-	return getStorageUploadVolumeSpec(s, spec)
+	return "", xerrors.Errorf("Invalid ArtifactType %s. Should be one of %s", val, validArtifactTypes)
 }
 
 func (s *BuildGCSResource) GetDownloadVolumeSpec(spec *TaskSpec) ([]corev1.Volume, error) {
-	return getStorageUploadVolumeSpec(s, spec)
+	return getStorageVolumeSpec(s, spec)
 }
diff --git a/pkg/apis/pipeline/v1alpha1/build_gcs_resource_test.go b/pkg/apis/pipeline/v1alpha1/build_gcs_resource_test.go
@@ -126,71 +126,49 @@ func Test_BuildGCSGetReplacements(t *testing.T) {
 }
 
 func Test_BuildGCSGetDownloadSteps(t *testing.T) {
-	resource := &v1alpha1.BuildGCSResource{
-		Name:         "gcs-valid",
-		Location:     "gs://some-bucket",
-		ArtifactType: "Archive",
-	}
-	wantSteps := []v1alpha1.Step{{Container: corev1.Container{
-		Name:    "create-dir-gcs-valid-9l9zj",
-		Image:   "override-with-bash-noop:latest",
-		Command: []string{"/ko-app/bash"},
-		Args:    []string{"-args", "mkdir -p /workspace"},
-	}}, {Container: corev1.Container{
-		Name:  "storage-fetch-gcs-valid-mz4c7",
-		Image: "gcr.io/cloud-builders/gcs-fetcher:latest",
-		Args: []string{"--type", "Archive", "--location", "gs://some-bucket",
-			"--dest_dir", "/workspace"},
-	}}}
-	names.TestingSeed()
-	got, err := resource.GetDownloadSteps("/workspace")
-	if err != nil {
-		t.Fatalf("GetDownloadSteps: %v", err)
-	}
-	if d := cmp.Diff(got, wantSteps); d != "" {
-		t.Errorf("Error mismatch between download steps: %s", d)
-	}
-}
-
-func Test_BuildGCSGetUploadSteps(t *testing.T) {
-	for _, tc := range []struct {
-		name      string
-		resource  *v1alpha1.BuildGCSResource
-		wantSteps []v1alpha1.Step
-		wantErr   bool
-	}{{
-		name: "valid upload to protected buckets with directory paths",
-		resource: &v1alpha1.BuildGCSResource{
-			Name:         "gcs-valid",
-			Location:     "gs://some-bucket/manifest.json",
-			ArtifactType: "Manifest",
-		},
-		wantSteps: []v1alpha1.Step{{Container: corev1.Container{
-			Name:  "storage-upload-gcs-valid-mssqb",
-			Image: "gcr.io/cloud-builders/gcs-uploader:latest",
-			Args:  []string{"--location", "gs://some-bucket/manifest.json", "--dir", "/workspace"},
-		}}},
-	}, {
-		name: "invalid upload to protected buckets with single file",
-		resource: &v1alpha1.BuildGCSResource{
-			Name:         "gcs-valid",
-			ArtifactType: "Archive",
-			Location:     "gs://some-bucket",
-		},
-		wantErr: true,
-	}} {
-		t.Run(tc.name, func(t *testing.T) {
-			got, err := tc.resource.GetUploadSteps("/workspace")
-			if tc.wantErr && err == nil {
-				t.Fatalf("Expected error to be %t but got %v:", tc.wantErr, err)
+	for _, at := range []v1alpha1.GCSArtifactType{
+		v1alpha1.GCSArchive,
+		v1alpha1.GCSZipArchive,
+		v1alpha1.GCSTarGzArchive,
+		v1alpha1.GCSManifest,
+	} {
+		t.Run(string(at), func(t *testing.T) {
+			resource := &v1alpha1.BuildGCSResource{
+				Name:         "gcs-valid",
+				Location:     "gs://some-bucket",
+				ArtifactType: at,
 			}
-			if !tc.wantErr && err != nil {
-				t.Fatalf("GetUploadSteps: %v", err)
+			wantSteps := []v1alpha1.Step{{Container: corev1.Container{
+				Name:    "create-dir-gcs-valid-9l9zj",
+				Image:   "override-with-bash-noop:latest",
+				Command: []string{"/ko-app/bash"},
+				Args:    []string{"-args", "mkdir -p /workspace"},
+			}}, {Container: corev1.Container{
+				Name:    "storage-fetch-gcs-valid-mz4c7",
+				Image:   "gcr.io/cloud-builders/gcs-fetcher:latest",
+				Args:    []string{"--type", string(at), "--location", "gs://some-bucket", "--dest_dir", "/workspace"},
+				Command: []string{"/ko-app/gcs-fetcher"},
+			}}}
+			names.TestingSeed()
+			got, err := resource.GetDownloadSteps("/workspace")
+			if err != nil {
+				t.Fatalf("GetDownloadSteps: %v", err)
 			}
-
-			if d := cmp.Diff(got, tc.wantSteps); d != "" {
-				t.Errorf("Error mismatch between upload steps: %s", d)
+			if d := cmp.Diff(got, wantSteps); d != "" {
+				t.Errorf("Error mismatch between download steps: %s", d)
 			}
 		})
 	}
 }
+
+func TestBuildGCS_InvalidArtifactType(t *testing.T) {
+	pr := tb.PipelineResource("build-gcs-resource", "default", tb.PipelineResourceSpec(
+		v1alpha1.PipelineResourceTypeStorage,
+		tb.PipelineResourceSpecParam("Location", "gs://fake-bucket"),
+		tb.PipelineResourceSpecParam("type", "build-gcs"),
+		tb.PipelineResourceSpecParam("ArtifactType", "InVaLiD"),
+	))
+	if _, err := v1alpha1.NewBuildGCSResource(pr); err == nil {
+		t.Error("NewBuildGCSResource: expected error")
+	}
+}