Implement the github Resource type

[bobcatfish]

Expected Behavior

When a user creates a TaskRun, the TaskRun will contain the actual Resources that the Task is using, and if these Resources are inputs, they should be made available as mounted volumes in the Task's steps.

For example run-kritis-test.yaml contains a kritis github source (note: maybe this section should be called resource now?):

        sources:
            - name: 'kritis'
              type: 'github'
              url:  'github.com/grafeas/kritis'
              branch: 'featureX'
              commit: 'HEAD'

This would mean that the TaskRun controller would need to pull from that github repo and make the contents available in a mounted volume for the steps subsequently executed by the TaskRun's Task.

It would need to use a serviceAccount to do this which would have the correct GitHub credentials.

Requirements

• Implement the logic to fetch a github Resource type, so that it can be made available for the steps in a TaskRun's Task (ideally sharing knative/build's gitToContainer)
• Docs on how to use a github Resource
• Test coverage, at least unit test, possibly also integration test if reasonable (may be blocked by Setup PR testing for build-pipeline integration tests #16) (this test coverage could even be added to the knative/build repo if that is where most of the logic livs)

Bonus:

• Update the TaskRun controller to invoke this logic

Actual Behavior

We have none of this implemented, TaskRun's controller currently just (barely) validates the yaml.

Additional Info

• If it is possible to share any of this logic with knative/build, that is great! knative/build-pipeline is likely going to depend on knative/build anyway
• Note that Remove kubebuilder (for now) #57 (coming asap) will make some sweeping changes to the repo.
• TaskRun currently doesn't seem to have any notion of the serviceAccount to use (e.g. in this example), the serviceAccount is currently specified only in the PipelineParams

(Open to redesign: instead of repeating all of the resources and credentials in the TaskRun, TaskRun could have a references to the Resource and PipelineParam CRDs. One downside tho is that if you look at a TaskRun, you won't know for sure what values were used for these, since they could have been subseqently updated.)

[bobcatfish]

@pivotal-nader-ziada this is the issue I was mentioning re. the github resource!

[nader-ziada]

Sounds good

/assign @pivotal-nader-ziada

[nader-ziada]

If it is possible to share any of this logic with knative/build, that is great! knative/build-pipeline is likely going to depend on knative/build anyway

It is better to pull the common parts we need to the pkg repo and use it from there in both build and build-pipeline, or another common repo?

TaskRun currently doesn't seem to have any notion of the serviceAccount to use (e.g. in this example), the serviceAccount is currently specified only in the PipelineParams

The resource itself has the ServiceAccount and the TaskRun refers to the resource

(Open to redesign: instead of repeating all of the resources and credentials in the TaskRun, TaskRun could have a references to the Resource and PipelineParam CRDs. One downside tho is that if you look at a TaskRun, you won't know for sure what values were used for these, since they could have been subsequently updated.)

I think we have a piece of that already in TaskRun using ResourceRef to link back to the resource and also having resourceVersion to indicate which version this specific task ran with. Isn't the taskRun result going to be saved as part of the pipelineRun?

[bobcatfish]

It is better to pull the common parts we need to the pkg repo and use it from there in both build and build-pipeline, or another common repo?

If you prefer that approach we can do it, but I prefer keeping the build related logic in build, since I think that's a reasonable grouping of logic.

One of the downsides of the pkg repo is that it isn't well factored; it's a grab bag of random stuff that seems to be useful across repos. build at least is limited to build related stuff so I kind of like that.

The resource itself has the ServiceAccount and the TaskRun refers to the resource

Oh interesting, I totally missed that the resource has a ServiceAccount. We had been assuming/hoping we could get away with one top level ServiceAccount for all actions a pipeline does, but it sounds like you'd rather have the flexibility of using different ServiceAccounts for different Resources?

I think we have a piece of that already in TaskRun using ResourceRef to link back to the resource and also having resourceVersion to indicate which version this specific task ran with. Isn't the taskRun result going to be saved as part of the pipelineRun?

Yep you are totally right, that's another thing (the fact that TaskRuns now have references to Resources) I missed!

The TaskRun will be saved into the run results - if we want to keep this as a ref, we could save a snapshot of the Resource as well - but if that's the way we want to go, I think we should do the same thing with other values such as the results which actually come from the PipelineParams. Maybe this is a better way to go so folks creating TaskRuns manually don't have to repeat as much info?

[nader-ziada]

If you prefer that approach we can do it, but I prefer keeping the build related logic in build, since I think that's a reasonable grouping of logic.

I'm fine with keeping it in build and depending on it there, I think we will have this dependency anyways. If we later decide to make the dependency less coupled, we can pull out the common things even in another common repo.

Oh interesting, I totally missed that the resource has a ServiceAccount. We had been assuming/hoping we could get away with one top level ServiceAccount for all actions a pipeline does, but it sounds like you'd rather have the flexibility of using different ServiceAccounts for different Resources?

yeah in case you have different accounts to get from buckets vs from github, I have seen this use case before, what do you think?

The TaskRun will be saved into the run results - if we want to keep this as a ref, we could save a snapshot of the Resource as well - but if that's the way we want to go, I think we should do the same thing with other values such as the results which actually come from the PipelineParams. Maybe this is a better way to go so folks creating TaskRuns manually don't have to repeat as much info?

I like the approach of saving the snapshot with the ref so everything is in one place

[bobcatfish]

. If we later decide to make the dependency less coupled, we can pull out the common things even in another common repo.

Sounds good to me!

yeah in case you have different accounts to get from buckets vs from github, I have seen this use case before, what do you think?

I think you're right, especially if you've seen it before. It'll be more flexible anyway!

I like the approach of saving the snapshot with the ref so everything is in one place

kk then let's assume that! In that case the PipelineRun would have references to PipelineParams + Resources, and TaskRun would have references to Resources as well. That would certainly make it easier to create them by hand!

[nader-ziada]

my high level plan so far is the following:

• the git resource will return an array of init-containers,
  • the creds init similar to how the build project uses the service account
  • git resource init-container that will include the github repo fetched on a volume in a folder with the name specified as the name of the inputSourceBinding

I'm expecting the TaskRun Controller/Runner will take this array of init-containers and add them to the Pod that will actually execute the task

@bobcatfish @imjasonh @shashwathi @tejal29 makes sense?

[bobcatfish]

Sounds great to me @pivotal-nader-ziada ! I pinged @aaron-prindle in case he has any other comments, he's working on #59.