From 70b951d4811a77dcd067ddc06e8b6d750ac53d9a Mon Sep 17 00:00:00 2001
From: Nader Ziada <nziada@pivotal.io>
Date: Thu, 4 Oct 2018 18:01:11 -0400
Subject: [PATCH] address comments from review of pr/91

---
 docs/pipeline-resources.md                    |  4 ++
 pkg/apis/pipeline/v1alpha1/git_resource.go    | 14 +++--
 .../taskrun/resources/input_resource_test.go  | 52 +++++++++++++++++++
 .../taskrun/resources/input_resources.go      | 15 +++---
 4 files changed, 75 insertions(+), 10 deletions(-)

diff --git a/docs/pipeline-resources.md b/docs/pipeline-resources.md
index a695692c8b5..be4be4a11f0 100644
--- a/docs/pipeline-resources.md
+++ b/docs/pipeline-resources.md
@@ -19,6 +19,10 @@ spec:
   params:
   - name: url
     value: github.com/wizzbangcorp/wizzbang
+  - name: Revision
+    value: master
+  - name: ServiceAccount
+    value: pipeline-sa    
  ```
 
    Params that can be added are the following:
diff --git a/pkg/apis/pipeline/v1alpha1/git_resource.go b/pkg/apis/pipeline/v1alpha1/git_resource.go
index fdb07ea2c74..c0f8afcf5c1 100644
--- a/pkg/apis/pipeline/v1alpha1/git_resource.go
+++ b/pkg/apis/pipeline/v1alpha1/git_resource.go
@@ -17,6 +17,7 @@ limitations under the License.
 package v1alpha1
 
 import (
+	"fmt"
 	"strings"
 )
 
@@ -34,8 +35,11 @@ type GitResource struct {
 	ServiceAccount string `json:"serviceAccount,omitempty"`
 }
 
-// NewGitResource create a new git resource to pass to Knativve Build
-func NewGitResource(r *PipelineResource) *GitResource {
+// NewGitResource create a new git resource to pass to Knative Build
+func NewGitResource(r *PipelineResource) (*GitResource, error) {
+	if r.Spec.Type != PipelineResourceTypeGit {
+		return nil, fmt.Errorf("GitResource: Cannot create a Git resource from a %s Pipeline Resource", r.Spec.Type)
+	}
 	gitResource := GitResource{
 		Name: r.Name,
 		Type: r.Spec.Type,
@@ -50,7 +54,11 @@ func NewGitResource(r *PipelineResource) *GitResource {
 			gitResource.Revision = param.Value
 		}
 	}
-	return &gitResource
+	// default revision to master is nothing is provided
+	if gitResource.Revision == "" {
+		gitResource.Revision = "master"
+	}
+	return &gitResource, nil
 }
 
 // GetName returns the name of the resource
diff --git a/pkg/reconciler/v1alpha1/taskrun/resources/input_resource_test.go b/pkg/reconciler/v1alpha1/taskrun/resources/input_resource_test.go
index 3a82dcce38f..a88ebc4859d 100644
--- a/pkg/reconciler/v1alpha1/taskrun/resources/input_resource_test.go
+++ b/pkg/reconciler/v1alpha1/taskrun/resources/input_resource_test.go
@@ -176,6 +176,58 @@ func TestAddResourceToBuild(t *testing.T) {
 				},
 			},
 		},
+	}, {
+		desc: "simple with branch",
+		task: task,
+		taskRun: &v1alpha1.TaskRun{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "build-from-repo-run",
+				Namespace: "marshmallow",
+			},
+			Spec: v1alpha1.TaskRunSpec{
+				TaskRef: v1alpha1.TaskRef{
+					Name: "simpleTask",
+				},
+				Inputs: v1alpha1.TaskRunInputs{
+					Resources: []v1alpha1.PipelineResourceVersion{
+						v1alpha1.PipelineResourceVersion{
+							ResourceRef: v1alpha1.PipelineResourceRef{
+								Name: "workspace",
+							},
+							Version: "branch",
+						},
+					},
+				},
+			},
+		},
+		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{
+				Source: &buildv1alpha1.SourceSpec{
+					Git: &buildv1alpha1.GitSourceSpec{
+						Url:      "https://github.com/grafeas/kritis",
+						Revision: "branch",
+					},
+				},
+			},
+		},
 	}, {
 		desc: "set revision to default value",
 		task: task,
diff --git a/pkg/reconciler/v1alpha1/taskrun/resources/input_resources.go b/pkg/reconciler/v1alpha1/taskrun/resources/input_resources.go
index c3363f09d31..7f109de3556 100644
--- a/pkg/reconciler/v1alpha1/taskrun/resources/input_resources.go
+++ b/pkg/reconciler/v1alpha1/taskrun/resources/input_resources.go
@@ -37,25 +37,26 @@ func AddInputResource(
 	for _, input := range task.Spec.Inputs.Sources {
 		resource, err := pipelineResourceLister.PipelineResources(task.Namespace).Get(input.Name)
 		if err != nil {
-
 			logger.Errorf("Failed to reconcile TaskRun: %q failed to Get Pipeline Resource: %q", task.Name, input.Name)
 			return nil, err
 		}
 		if resource.Spec.Type == v1alpha1.PipelineResourceTypeGit {
-			gitResource = v1alpha1.NewGitResource(resource)
+			gitResource, err = v1alpha1.NewGitResource(resource)
+			if err != nil {
+				logger.Errorf("Failed to reconcile TaskRun: %q Invalid Pipeline Resource: %q", task.Name, input.Name)
+				return nil, err
+			}
 			for _, trInput := range taskRun.Spec.Inputs.Resources {
 				if trInput.ResourceRef.Name == input.Name {
-					gitResource.Revision = trInput.Version
+					if trInput.Version != "" {
+						gitResource.Revision = trInput.Version
+					}
 					break
 				}
 			}
 			break
 		}
 	}
-	// default revision to master is nothing is provided
-	if gitResource.Revision == "" {
-		gitResource.Revision = "master"
-	}
 
 	gitSourceSpec := &buildv1alpha1.GitSourceSpec{
 		Url:      gitResource.URL,