Skip to content
This repository has been archived by the owner on Dec 5, 2024. It is now read-only.

Prototype Pull Request support in a webhooks-driven pipeline #45

Closed
mnuttall opened this issue Jun 4, 2019 · 20 comments
Closed

Prototype Pull Request support in a webhooks-driven pipeline #45

mnuttall opened this issue Jun 4, 2019 · 20 comments
Assignees
Labels

Comments

@mnuttall
Copy link
Contributor

mnuttall commented Jun 4, 2019

tektoncd/pipeline#895 is getting close to being merged. It introduces a first level of support for a PullRequest PipelineResource. Use this issue to build a working prototype in which a pipeline run created by a webhook from an incoming Pull Request from a github fork results in the associated pull request being updated. We want to know:

  • Can we access the 'status' of the pull request to reflect whether the build was successful or not?
  • If not what can we currently do? Add comments? What else?

Our current sink code will need to be updated so as to create a PullRequest pipeline resource. Obviously the code is not merged yet so things may be a bit flaky!

Update Epic #36 with more tasks once we better understand where we're starting.

the design doc states,

An author of a Tekton Task should be able to write a Task which takes an SCM PipelineResource as an:

Input: This should make data about an existing Pull Request available to a Task, including:

  • Description
  • Corresponding version control info (HEAD commit/ref/repo, base commit/ref/repo)
  • Comments
  • Check status(es)

Output: After Task execution, changes made to the PipelineResource should be reflected in the
Pull Request including:

  • Comments
  • Check status(es)
  • Merging the pull request/closing issues.
  • Label manipulation
  • Description edits

Which of these have been implemented so far under tektoncd/pipeline#895? Which if any are under development?

@mnuttall mnuttall changed the title Prototype PR support in a webhooks-driven pipeline Prototype Pull Request support in a webhooks-driven pipeline Jun 7, 2019
@akihikokuroda
Copy link
Member

I take this.

@akihikokuroda
Copy link
Member

/assign

@akihikokuroda
Copy link
Member

So far, I see the tektoncd/pipeline#895 working for retrieving and push back the pull request payload. It implemented the update of the comments and labels not. I'll try some updates. For the prototype, I'll update the webhook sink code to extract the pullrequest URL form the payload so that can create the PullRequest pipeline resource. I can not use the pipelinerun status to update the pullrequest in the task running in the pipeline so that I'll make a task that picks up the status for the specific task in the pipeline and use it to update the pullrequest.

@akihikokuroda
Copy link
Member

@afrittoli demonstrated "PR -> Tekton Pipeline -> Image in Registry -> Comment on the PR with URL of the image@digest" in the CloudEvent PipelineResource E2E demo (https://docs.google.com/document/d/1rPR7m1Oj0ip3bpd_bcS1sjZyPgGi_g9asF5YrExeESc/edit#)

@akihikokuroda
Copy link
Member

akihikokuroda commented Jun 20, 2019

@akihikokuroda
Copy link
Member

akihikokuroda commented Jun 24, 2019

I created the task that checks the status of tasks in the same pipeline except itself. It determines if everything is OK or any error happens. It updates the pull request with the result. The task difinition is here https://github.com/akihikokuroda/pullrequest. I'll update the webhook to create the pullrequest pipeline resource.

@akihikokuroda
Copy link
Member

There some design issue in the current webhook for the pullrequest support. I hope that the new event triggering will cover these.

With the current Tekton, the same pipeline can not be used for the webhook for the commit and the pull request. The pull request pipeline needs the pullrequest pipeline resource and the pipelinerun must provide it. The pipelinerun needs to provide the resource when it is required by the pipeline otherwise it is invalid. The webhook resource has one pipeline in it and the gitrepositoryurl is the key for the instance so it can not have more than one pipeline can not be associated to the repository.

@akihikokuroda
Copy link
Member

For this prototype, I'll create a new pipeline with the pullrequest pipeline resource. Its name will be "pipelinename" + "-pull". The webhook will use the pipeline (pipelinename-pull) instead of the specified pipeline (pipelinename) when it get the pull request notification.

@akihikokuroda
Copy link
Member

akihikokuroda commented Jun 28, 2019

I could make the end-to-end scenario work with the docker for desktop and the github enterprise.
I had to add the github enterprise support in the Pullrequest pipeline resource. I probably create a PR for it in the pipeline.

So far, I created:
task definition: check the pipelinerun status and update the pull request
pipeline definition: updated simple-pipeline with the pull request task

I updated:
pipeline/cmd/pullrequest-init/github.go: add github enterprise support
experimental/webhooks-extension/pkg/endpoints/sink.go: handle pull request notification

@akihikokuroda
Copy link
Member

I created a PR for the GHE support for the Pull Reqeust pipeline resource: tektoncd/pipeline#1033

How should I warp up this issue? I have one task, one example pipeline that uses the task and sink.go in the webhook extension.

Will we implement this in the current webhook extension or wait for the new triggering code?

@akihikokuroda
Copy link
Member

There is one thing that must be decided if the code is in the current webhook. So far, there are 2 pipelines necessary. One is for the manual trigger without updating the pull request and the other is for the pull request trigger that update the pull request at the end. In the prototype, I just named the latter as the first pipeline name plus "-pull" so that the webhook handing code can switch between these pipelines by the type of the github notification. Another way is to add another pipeline name for the pull request notification in the webhook. This works for the new triggering design that allow association between the event type and the pipeline.

@akihikokuroda
Copy link
Member

The 2 pipeline issue can be resolved by the conditional task run. The webhook always creates the pipelinerun with the pull request resource. The rul in the resource has a fake value when the notification is not the pull request. The webhook also passes the type of notification as the parameter. The pull request update task should be conditional and executed only when the notification type is the pull request.

@akihikokuroda
Copy link
Member

I updated the sink to create a taskrun in addition of the pipelinerun. The taskrun monitors the status of the pipelinerun and update the pullrequest based on the result of the pipelinerun. This way seems better than adding an additional task at the end of the pipelinerun. It doesn't need 2 pipeline definition and the status check code is much simpler. We can provide the default taskrun and we can update the webhook to have the user defined taskrun.

@akihikokuroda
Copy link
Member

PR for the webhook: #138
PR for the Task: pipeline-hotel/example-pipelines#3

@a-roberts
Copy link
Member

The task is now merged, next we need to take a look at the extension PR

@akihikokuroda
Copy link
Member

I refreshed the PR for the webhook: #154

@akihikokuroda
Copy link
Member

Adding the error message in the pipelinerun to the pull request when the pipelinerun fails.
PR: pipeline-hotel/example-pipelines#5

@akihikokuroda
Copy link
Member

PR: #189 merged.

@akihikokuroda
Copy link
Member

/close

@tekton-robot
Copy link

@akihikokuroda: Closing this issue.

In response to this:

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

4 participants