initial implementation for git resource for pipeline #91

nader-ziada · 2018-10-03T20:54:48Z

fixes #58

Proposed changes

add resource to Build as an input source
taskRun controller would have to call the AddInputResource func

still in progress, but wanted to get your initial thoughts
/cc @bobcatfish @tejal29 @aaron-prindle

knative-prow-robot · 2018-10-03T20:55:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pivotal-nader-ziada

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [pivotal-nader-ziada]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

nader-ziada · 2018-10-03T20:56:03Z

/hold

nader-ziada · 2018-10-04T16:33:36Z

/hold cancel
@bobcatfish @tejal29 @aaron-prindle can you take a look when you can. Thanks

docs/pipeline-resources.md

tejal29 · 2018-10-04T20:17:41Z

pkg/apis/pipeline/v1alpha1/git_resource.go

@@ -30,6 +34,25 @@ type GitResource struct {
 	ServiceAccount string `json:"serviceAccount,omitempty"`
 }

+// NewGitResource create a new git resource to pass to Knativve Build


nit: s/Knativve/Knative

pkg/apis/pipeline/v1alpha1/git_resource.go

tejal29 · 2018-10-04T20:25:09Z

pkg/reconciler/v1alpha1/taskrun/resources/input_resource_test.go

+			},
+		},
+	}
+	pipelineResourceInformer.Informer().GetIndexer().Add(res)


nice! i will try to add the mock objects here instead of using mocks

tejal29 · 2018-10-04T20:33:04Z

pkg/reconciler/v1alpha1/taskrun/resources/input_resource_test.go

+				Source: &buildv1alpha1.SourceSpec{
+					Git: &buildv1alpha1.GitSourceSpec{
+						Url:      "https://github.com/grafeas/kritis",
+						Revision: "master",


can you add a task run with Revision other than master specified for completeness.

tejal29 · 2018-10-04T20:35:15Z

pkg/reconciler/v1alpha1/taskrun/resources/input_resources.go

+	for _, input := range task.Spec.Inputs.Sources {
+		resource, err := pipelineResourceLister.PipelineResources(task.Namespace).Get(input.Name)
+		if err != nil {
+


remove empty line

tejal29 · 2018-10-04T20:37:41Z

pkg/reconciler/v1alpha1/taskrun/resources/input_resource_test.go

+						ResourceRef: v1alpha1.PipelineResourceRef{
+							Name: "workspace",
+						},
+						Version: "master",


can you specify something other than master as default is master

there is a test set revision to default value that doesn't specify any revision and checks it gets back master, is that what you mean?

no, there is not test where the Version of Taskrun is set correctly in GitResource Revision.
Right now, we can't tell if Version is set in loop L46 or L57

ok, I get what you are saying, will add another test

tejal29 · 2018-10-04T20:41:27Z

pkg/reconciler/v1alpha1/taskrun/resources/input_resources.go

+		}
+	}
+	// default revision to master is nothing is provided
+	if gitResource.Revision == "" {


hmm, i feel the defaults should be set in the GitResource Constructor. The GitResource object decalres the defaults for its fields, so the onus should be on the Constructor rather than on this method.

the revision comes from the taskRun which happens after the constructor is called

which is fine. We default the revision to master if resource does not revision defined in NewGitResource().
Then in the loop, we override it trInput.Version.

ok, makes sense

but please note that the TaskRun validation should ensure it has a version, this is a required field

- add resource to Build as an input source

nader-ziada · 2018-10-04T22:06:59Z

@tejal29 ready for another look, thanks

tejal29 · 2018-10-04T22:30:04Z

/lgtm

bobcatfish · 2018-10-05T16:27:26Z

docs/pipeline-resources.md

@@ -0,0 +1,83 @@
+### Pipeline Resources
+
+## Git Resource


so cool, thanks for adding docs!

it would be good at add a link from https://github.com/knative/build-pipeline#resources to here as well, since folks won't know this doc exists

bobcatfish · 2018-10-09T17:44:37Z

docs/pipeline-resources.md

+
+   1. URL: represents the location of the git repository 
+   1. Revision: Git [revision](https://git-scm.com/docs/gitrevisions#_specifying_revisions ) (branch, tag, commit SHA or ref) to clone. If no revision is specified, the resource will default to `latest` from `master`
+   1. Service Account: specifies the `name` of a `ServiceAccount` resource object. Add this paramater to run your task with the privileges of the specified service account. If no serviceAccountName field is specified, your task runs using the default service account that is in the namespace of the Pipeline resource object.  


hmmm i wonder if this is a good default - in PipelineParams we currently have a serviceAccount field as well, is there some way we can default to that one?

and/or maybe have an error if there is no serviceaccount?

bobcatfish · 2018-10-09T17:45:00Z

docs/pipeline-resources.md

+        artifacts:
+          - name: builtImage
+            type: image
+``` 


great docs!!

bobcatfish · 2018-10-09T17:47:12Z

pkg/reconciler/v1alpha1/taskrun/resources/input_resource_test.go

+			got, err := AddInputResource(c.build, c.task, c.taskRun, pipelineResourceLister, logger)
+			if (err != nil) != c.wantErr {
+				t.Errorf("Test: %q; NewControllerConfigFromConfigMap() error = %v, WantErr %v", c.desc, err, c.wantErr)
+			}


im kind of making this comment everywhere, but what are your thoughts about having separate test functions for expected failures and successes? I feel like mixing them and having to handle both here clutters up the test a bit

I'm okay with splitting them if you prefer that, big table tests are hard to read for sure

bobcatfish · 2018-10-09T17:49:03Z

pkg/reconciler/v1alpha1/taskrun/resources/input_resources.go

+)
+
+// AddInputResource will update the input build with the input resource from the task
+func AddInputResource(


ah nice, so I guess this function is waiting to be called by someone implementing #62 or #63 ?

yes, I'm just working on it today as part of #62 :)

feat: adapt for tekton 0.56

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 3, 2018

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 3, 2018

nader-ziada force-pushed the git-resource branch from ba1aaa4 to 8221983 Compare October 4, 2018 16:23

nader-ziada changed the title ~~[WIP] initial implementation for git resource for pipeline~~ initial implementation for git resource for pipeline Oct 4, 2018

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 4, 2018

tejal29 suggested changes Oct 4, 2018

View reviewed changes

nader-ziada added 2 commits October 4, 2018 18:02

git resource for pipeline tektoncd#58

f46c479

- add resource to Build as an input source

address comments from review of pr/91

e2ce7ba

nader-ziada force-pushed the git-resource branch from 8221983 to e2ce7ba Compare October 4, 2018 22:02

knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 4, 2018

knative-prow-robot assigned tejal29 Oct 4, 2018

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 4, 2018

tejal29 approved these changes Oct 4, 2018

View reviewed changes

knative-prow-robot merged commit 70b951d into tektoncd:master Oct 4, 2018

nader-ziada mentioned this pull request Oct 5, 2018

add test coverage automation to the repo #99

Closed

bobcatfish reviewed Oct 5, 2018

View reviewed changes

bobcatfish reviewed Oct 9, 2018

View reviewed changes

docs/pipeline-resources.md

artifacts:

- name: builtImage

type: image

```

Copy link

Collaborator

bobcatfish Oct 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great docs!!

bobcatfish reviewed Oct 9, 2018

View reviewed changes

nader-ziada deleted the git-resource branch October 11, 2018 20:26

mchmarny unassigned tejal29 Mar 7, 2019

l-qing added a commit to l-qing/pipeline that referenced this pull request Feb 10, 2024

Merge pull request tektoncd#91 from katanomi/feat/adapt-for-tekton-0.56

28a5904

feat: adapt for tekton 0.56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

initial implementation for git resource for pipeline #91

initial implementation for git resource for pipeline #91

nader-ziada commented Oct 3, 2018

knative-prow-robot commented Oct 3, 2018

nader-ziada commented Oct 3, 2018

nader-ziada commented Oct 4, 2018

tejal29 Oct 4, 2018

tejal29 Oct 4, 2018

tejal29 Oct 4, 2018

tejal29 Oct 4, 2018

tejal29 Oct 4, 2018

nader-ziada Oct 4, 2018

tejal29 Oct 4, 2018

nader-ziada Oct 4, 2018

tejal29 Oct 4, 2018

nader-ziada Oct 4, 2018

tejal29 Oct 4, 2018

nader-ziada Oct 4, 2018

nader-ziada Oct 4, 2018

nader-ziada commented Oct 4, 2018

tejal29 commented Oct 4, 2018

bobcatfish Oct 5, 2018

bobcatfish Oct 9, 2018

bobcatfish Oct 9, 2018

bobcatfish Oct 9, 2018

nader-ziada Oct 9, 2018

bobcatfish Oct 9, 2018

nader-ziada Oct 9, 2018

initial implementation for git resource for pipeline #91

initial implementation for git resource for pipeline #91

Conversation

nader-ziada commented Oct 3, 2018

Proposed changes

knative-prow-robot commented Oct 3, 2018

nader-ziada commented Oct 3, 2018

nader-ziada commented Oct 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nader-ziada commented Oct 4, 2018

tejal29 commented Oct 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment