refactor and enhancements to artifact bucket code

 - addressed comments from pr tektoncd#444 review

Author: nader-ziada

Diff for: docs/using.md

@@ -196,11 +196,12 @@ following alternatives:
   that tasks cannot have any volume mounted under path `/pvc`.
 
 - [GCS storage bucket](https://cloud.google.com/storage/docs/json_api/v1/buckets)
-  A storage bucket can be configured using a ConfigMap with the name `config-artifact-bucket` with the following attributes:
-- location: the address of the bucket (for example gs://mybucket)
-- bucket.service.account.secret.name: the name of the secret that will contain the credentials for the service account
+  A storage bucket can be configured using a ConfigMap named [`config-artifact-bucket`](./../config/config-artifact-bucket.yaml). 
+  with the following attributes:
+- `location`: the address of the bucket (for example gs://mybucket)
+- `bucket.service.account.secret.name`: the name of the secret that will contain the credentials for the service account
   with access to the bucket
-- bucket.service.account.secret.key: the key in the secret with the required service account json
+- `bucket.service.account.secret.key`: the key in the secret with the required service account json
 The bucket is configured with a retention policy of 24 hours after which files will be deleted
 
 ### Outputs

Diff for: pkg/reconciler/v1alpha1/pipelinerun/config/store.go

@@ -45,7 +45,7 @@ type Store struct {
 }
 
 func NewStore(logger configmap.Logger) *Store {
-	store := &Store{
+	return &Store{
 		UntypedStore: configmap.NewUntypedStore(
 			"pipelinerun",
 			logger,
@@ -54,8 +54,6 @@ func NewStore(logger configmap.Logger) *Store {
 			},
 		),
 	}
-
-	return store
 }
 
 func (s *Store) ToContext(ctx context.Context) context.Context {

Diff for: pkg/reconciler/v1alpha1/taskrun/resources/input_resource_test.go

@@ -1126,7 +1126,6 @@ func Test_StorageInputResource(t *testing.T) {
 }
 
 func TestAddStepsToBuild_WithBucketFromConfigMap(t *testing.T) {
-	boolTrue := true
 	task := &v1alpha1.Task{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      "build-from-repo",
@@ -1162,8 +1161,7 @@ func TestAddStepsToBuild_WithBucketFromConfigMap(t *testing.T) {
 		task    *v1alpha1.Task
 		taskRun *v1alpha1.TaskRun
 		build   *buildv1alpha1.Build
-		wantErr bool
-		want    *buildv1alpha1.Build
+		want    buildv1alpha1.BuildSpec
 	}{{
 		desc: "git resource as input from previous task - copy to bucket",
 		task: task,
@@ -1188,35 +1186,18 @@ func TestAddStepsToBuild_WithBucketFromConfigMap(t *testing.T) {
 				},
 			},
 		},
-		build:   build(),
-		wantErr: false,
-		want: &buildv1alpha1.Build{
-			TypeMeta: metav1.TypeMeta{
-				Kind:       "Build",
-				APIVersion: "build.knative.dev/v1alpha1"},
-			ObjectMeta: metav1.ObjectMeta{
-				Name:      "build-from-repo",
-				Namespace: "marshmallow",
-				OwnerReferences: []metav1.OwnerReference{{
-					APIVersion:         "pipeline.knative.dev/v1alpha1",
-					Kind:               "TaskRun",
-					Name:               "build-from-repo-run",
-					Controller:         &boolTrue,
-					BlockOwnerDeletion: &boolTrue,
-				}},
-			},
-			Spec: buildv1alpha1.BuildSpec{
-				Steps: []corev1.Container{{
-					Name:  "artifact-dest-mkdir-gitspace-0",
-					Image: "override-with-bash-noop:latest",
-					Args:  []string{"-args", "mkdir -p /workspace/gitspace"},
-				},
-					{
-						Name:  "artifact-copy-from-gitspace-0",
-						Image: "override-with-gsutil-image:latest",
-						Args:  []string{"-args", "cp -r gs://fake-bucket/prev-task-path/** /workspace/gitspace"},
-					}},
+		build: build(),
+		want: buildv1alpha1.BuildSpec{
+			Steps: []corev1.Container{{
+				Name:  "artifact-dest-mkdir-gitspace-0",
+				Image: "override-with-bash-noop:latest",
+				Args:  []string{"-args", "mkdir -p /workspace/gitspace"},
 			},
+				{
+					Name:  "artifact-copy-from-gitspace-0",
+					Image: "override-with-gsutil-image:latest",
+					Args:  []string{"-args", "cp -r gs://fake-bucket/prev-task-path/** /workspace/gitspace"},
+				}},
 		},
 	}, {
 		desc: "storage resource as input from previous task - copy from bucket",
@@ -1242,35 +1223,18 @@ func TestAddStepsToBuild_WithBucketFromConfigMap(t *testing.T) {
 				},
 			},
 		},
-		build:   build(),
-		wantErr: false,
-		want: &buildv1alpha1.Build{
-			TypeMeta: metav1.TypeMeta{
-				Kind:       "Build",
-				APIVersion: "build.knative.dev/v1alpha1"},
-			ObjectMeta: metav1.ObjectMeta{
-				Name:      "build-from-repo",
-				Namespace: "marshmallow",
-				OwnerReferences: []metav1.OwnerReference{{
-					APIVersion:         "pipeline.knative.dev/v1alpha1",
-					Kind:               "TaskRun",
-					Name:               "build-from-repo-run",
-					Controller:         &boolTrue,
-					BlockOwnerDeletion: &boolTrue,
-				}},
-			},
-			Spec: buildv1alpha1.BuildSpec{
-				Steps: []corev1.Container{{
-					Name:  "artifact-dest-mkdir-workspace-0",
-					Image: "override-with-bash-noop:latest",
-					Args:  []string{"-args", "mkdir -p /workspace/gcs-dir"},
-				},
-					{
-						Name:  "artifact-copy-from-workspace-0",
-						Image: "override-with-gsutil-image:latest",
-						Args:  []string{"-args", "cp -r gs://fake-bucket/prev-task-path/** /workspace/gcs-dir"},
-					}},
+		build: build(),
+		want: buildv1alpha1.BuildSpec{
+			Steps: []corev1.Container{{
+				Name:  "artifact-dest-mkdir-workspace-0",
+				Image: "override-with-bash-noop:latest",
+				Args:  []string{"-args", "mkdir -p /workspace/gcs-dir"},
 			},
+				{
+					Name:  "artifact-copy-from-workspace-0",
+					Image: "override-with-gsutil-image:latest",
+					Args:  []string{"-args", "cp -r gs://fake-bucket/prev-task-path/** /workspace/gcs-dir"},
+				}},
 		},
 	}} {
 		t.Run(c.desc, func(t *testing.T) {
@@ -1287,10 +1251,10 @@ func TestAddStepsToBuild_WithBucketFromConfigMap(t *testing.T) {
 				},
 			)
 			got, err := AddInputResource(fakekubeclient, c.build, c.task.Name, &c.task.Spec, c.taskRun, pipelineResourceLister, logger)
-			if (err != nil) != c.wantErr {
-				t.Errorf("Test: %q; AddInputResource() error = %v, WantErr %v", c.desc, err, c.wantErr)
+			if err != nil {
+				t.Errorf("Test: %q; AddInputResource() error = %v", c.desc, err)
 			}
-			if d := cmp.Diff(got, c.want); d != "" {
+			if d := cmp.Diff(got.Spec, c.want); d != "" {
 				t.Errorf("Diff:\n%s", d)
 			}
 		})

Diff for: test/README.md

@@ -107,16 +107,17 @@ pipelineRunsInformer.Informer().GetIndexer().Add(obj)
 Besides the environment variable `KO_DOCKER_REPO`, you may also need the
 permissions inside the TaskRun to run the Kaniko e2e test and GCS taskrun test.
 
-- In Kaniko e2e test, setting `KANIKO_SECRET_CONFIG_FILE` as the path of the GCP
+- In Kaniko e2e test, setting `GCP_SERVICE_ACCOUNT_KEY_PATH` as the path of the GCP
   service account JSON key which has permissions to push to the registry
   specified in `KO_DOCKER_REPO` will enable Kaniko to use those credentials when
   pushing an image.
 - In GCS taskrun test, GCP service account JSON key file at path
-  `KANIKO_SECRET_CONFIG_FILE` is used to generate Kubernetes secret to access
+  `GCP_SERVICE_ACCOUNT_KEY_PATH` is used to generate Kubernetes secret to access
   GCS bucket. This e2e test requires valid service account configuration json
   but it does not require any role binding.
-- In Storage artifact bucket, GCP service account JSON key file at path
-  `KANIKO_SECRET_CONFIG_FILE` is used to create/delete a bucket.
+- In Storage artifact bucket, setting the `GCP_SERVICE_ACCOUNT_KEY_PATH` as the 
+  path of the GCP service account JSON key which has permissions to create/delete 
+  a bucket.
 
 To reduce e2e test setup developers can use the same environment variable for
 both Kaniko e2e test and GCS taskrun test. To create a service account usable in
@@ -138,7 +139,7 @@ gcloud projects add-iam-policy-binding $PROJECT_ID --member serviceAccount:$EMAI
 # create the JSON key
 gcloud iam service-accounts keys create config.json --iam-account $EMAIL
 
-export KANIKO_SECRET_CONFIG_FILE="$PWD/config.json"
+export GCP_SERVICE_ACCOUNT_KEY_PATH="$PWD/config.json"
 ```
 
 ### Running

Diff for: test/artifact_bucket_test.go

@@ -44,9 +44,9 @@ const (
 // TestStorageBucketPipelineRun is an integration test that will verify a pipeline
 // can use a bucket for temporary storage of artifacts shared between tasks
 func TestStorageBucketPipelineRun(t *testing.T) {
-	configFilePath := os.Getenv("KANIKO_SECRET_CONFIG_FILE")
+	configFilePath := os.Getenv("GCP_SERVICE_ACCOUNT_KEY_PATH")
 	if configFilePath == "" {
-		t.Skip("KANIKO_SECRET_CONFIG_FILE variable is not set.")
+		t.Skip("GCP_SERVICE_ACCOUNT_KEY_PATH variable is not set.")
 	}
 	logger := logging.GetContextLogger(t.Name())
 	c, namespace := setup(t, logger)
@@ -100,13 +100,20 @@ func TestStorageBucketPipelineRun(t *testing.T) {
 
 	defer runTaskToDeleteBucket(c, t, logger, namespace, bucketName, bucketSecretName, bucketSecretKey)
 
+	originalConfigMap, err := c.KubeClient.Kube.CoreV1().ConfigMaps(systemNamespace).Get(v1alpha1.BucketConfigName, metav1.GetOptions{})
+	if err != nil {
+		t.Fatalf("Failed to get ConfigMap `%s`: %s", v1alpha1.BucketConfigName, err)
+	}
+	originalConfigMapData := originalConfigMap.Data
+
 	logger.Infof("Creating ConfigMap %s", v1alpha1.BucketConfigName)
 	configMapData := map[string]string{
 		v1alpha1.BucketLocationKey:              fmt.Sprintf("gs://%s", bucketName),
 		v1alpha1.BucketServiceAccountSecretName: bucketSecretName,
 		v1alpha1.BucketServiceAccountSecretKey:  bucketSecretKey,
 	}
 	c.KubeClient.UpdateConfigMap(systemNamespace, v1alpha1.BucketConfigName, configMapData)
+	defer resetConfigMap(c, systemNamespace, v1alpha1.BucketConfigName, originalConfigMapData)
 
 	logger.Infof("Creating Git PipelineResource %s", helloworldResourceName)
 	helloworldResource := tb.PipelineResource(helloworldResourceName, namespace, tb.PipelineResourceSpec(
@@ -205,6 +212,10 @@ func deleteBucketSecret(c *clients, t *testing.T, logger *logging.BaseLogger, na
 	}
 }
 
+func resetConfigMap(c *clients, namespace, configName string, values map[string]string) error {
+	return c.KubeClient.UpdateConfigMap(namespace, configName, values)
+}
+
 func runTaskToDeleteBucket(c *clients, t *testing.T, logger *logging.BaseLogger, namespace, bucketName, bucketSecretName, bucketSecretKey string) {
 	deletelbuckettask := tb.Task("deletelbuckettask", namespace, tb.TaskSpec(
 		tb.TaskVolume("bucket-secret-volume", tb.VolumeSource(corev1.VolumeSource{

Diff for: test/gcs_taskrun_test.go

@@ -35,9 +35,9 @@ import (
 // - places files in expected place
 
 func TestStorageTaskRun(t *testing.T) {
-	configFile := os.Getenv("KANIKO_SECRET_CONFIG_FILE")
+	configFile := os.Getenv("GCP_SERVICE_ACCOUNT_KEY_PATH")
 	if configFile == "" {
-		t.Skip("KANIKO_SECRET_CONFIG_FILE variable is not set.")
+		t.Skip("GCP_SERVICE_ACCOUNT_KEY_PATH variable is not set.")
 	}
 	logger := logging.GetContextLogger(t.Name())
 	t.Parallel()

Diff for: test/kaniko_task_test.go

@@ -65,7 +65,7 @@ func getDockerRepo() (string, error) {
 
 func createSecret(c *knativetest.KubeClient, namespace string) (bool, error) {
 	// when running e2e in cluster, this will not be set so just hop out early
-	file := os.Getenv("KANIKO_SECRET_CONFIG_FILE")
+	file := os.Getenv("GCP_SERVICE_ACCOUNT_KEY_PATH")
 	if file == "" {
 		return false, nil
 	}