Add GitHub status updates to Pull Request CRD. #995

wlynch · 2019-06-20T15:42:17Z

This adds support for the GitHub Status API
(https://developer.github.com/v3/repos/statuses/).

This accompanies #778 and #895 to complete initial Pull Request support
support for GitHub OAuth.

Changes

Adds GitHub status fetching and updates to Pull Request CRD.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide
for more details.

Release Notes

Adds GitHub status fetching and updates to Pull Request CRD.

wlynch · 2019-06-25T00:48:38Z

/retest

vdemeester

Looks good to me, question though : right now it's up to the user to write github status or not right (with this PR) ? Do we envision some "automation" around that ?

wlynch · 2019-06-25T14:40:57Z

Looks good to me, question though : right now it's up to the user to write github status or not right (with this PR) ? Do we envision some "automation" around that ?

Yup! Great question, I'd love to source more ideas around this. The straight-forward answer would be to provide a runner binary that would wrap test execution and transform status codes and stdout/stderr to the pull request object.

Is there any work being done around passing state between TaskRuns (e.g. for conditional execution or something similar)?

vdemeester · 2019-06-25T14:56:09Z

@wlynch yeah we need to think about that, as I think I remember talking about notification as a first-class citizen in Tekton maybe ? (and most likely write on a design docs).

Is there any work being done around passing state between TaskRuns (e.g. for conditional execution or something similar)?

@dibyom ^^ 👼

dibyom · 2019-06-25T16:50:26Z

@wlynch yes, the condition container will have access to some read only metadata. Right now, this is just the pipeline run including the status fields. We could expose this to task runs as well? Or do you need to write additional metadata that needs to be exposed?

wlynch · 2019-06-25T20:47:32Z

@dibyom Don't need to write more data, but it would be useful to expose it to TaskRuns to have a binary look for this information and automatically prep PR output as a response. Can you link me to that data and how TaskRuns can access it?

dlorenc · 2019-06-26T01:02:26Z

@wlynch yeah we need to think about that, as I think I remember talking about notification as a first-class citizen in Tekton maybe ? (and most likely write on a design docs).

Yeah, I think the automated use-cases for this would fit into something around either "notification" or "conditional execution".

With the conditional execution approach, a pipeline would have a Task that runs on either success or failure and writes the status to Github/Gitlab.

afrittoli · 2019-06-26T09:33:55Z

Yeah, I think the automated use-cases for this would fit into something around either "notification" or "conditional execution".

This could be implemented with the CloudEventPipelineResource as well.
The setup would be:

each task that needs to publish test results to the PR has an output resource of type cloudevent, which is sent to a dedicated tekton listener. The TaskRun, including status, is sent as body of the cloudevent.
an event binding is defined that accepts TaskRun completion events, and triggers a GitHub PR update Task(Run)
The GitHub PR update taskrun uses the PullRequest resource as input and output, and the metadata in the TaskRun (passed as parameter to the Task) to update GitHub

The advantages of using a separate Task/TaskRun are:

The status of the original Task is not affected by the outcome of the PR update
It doesn't require conditional execution
The Task that updates GitHub PRs can be as isolated as we want from the Task that runs the tests, which should make it easier to avoid leaking the GitHub token in logs.

afrittoli

Thank you for this, nice work!

I tried out the code and integrated it in the demo I did last week and it works nicely.

afrittoli · 2019-06-26T09:39:36Z

cmd/pullrequest-init/types.go

+	ActionRequired StatusCode = "action_required"
+)
+
+// TODO: Add getters to make types nil-safe.


Are you planning this for a follow-up PR?

Yup! Didn't want to include this in this change since it's going to be a lot of boilerplate 😭

cmd/pullrequest-init/github_test.go

afrittoli · 2019-06-26T14:06:11Z

/retest

dibyom · 2019-06-26T15:22:42Z

@dibyom Don't need to write more data, but it would be useful to expose it to TaskRuns to have a binary look for this information and automatically prep PR output as a response. Can you link me to that data and how TaskRuns can access it?

WIP but its a file (pr-metadata.json) in /workspace for condition containers (sample)

Its exposed to conditional pods at the moment and not regular taskruns. I opened #1016 to track that.

wlynch · 2019-06-26T20:28:38Z

/assign @imjasonh

dlorenc · 2019-06-26T23:36:10Z

/lgtm

dlorenc · 2019-06-26T23:36:18Z

/approve

wlynch · 2019-07-02T15:40:32Z

/retest

dibyom

There is a binary checked in cmd/pullrequest-init/pullrequest-init. Is that intentional?

wlynch · 2019-07-02T17:03:47Z

There is a binary checked in cmd/pullrequest-init/pullrequest-init. Is that intentional?

Good catch! Removed.

This adds support for the GitHub Status API (https://developer.github.com/v3/repos/statuses/). This accompanies tektoncd#778 and tektoncd#895 to complete initial Pull Request support support for GitHub OAuth.

vdemeester

/lgtm

tekton-robot · 2019-07-03T09:01:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afrittoli, dlorenc, vdemeester, wlynch

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~~ [dlorenc,vdemeester]

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

googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Jun 20, 2019

tekton-robot requested review from bobcatfish and imjasonh June 20, 2019 15:42

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 20, 2019

wlynch force-pushed the pr branch from 0a2b0b4 to 04a2cfc Compare June 20, 2019 15:48

vdemeester reviewed Jun 25, 2019

View reviewed changes

afrittoli reviewed Jun 26, 2019

View reviewed changes

afrittoli approved these changes Jun 26, 2019

View reviewed changes

dibyom mentioned this pull request Jun 26, 2019

Expose pipelinerun status to taskrun #1016

Closed

tekton-robot assigned imjasonh Jun 26, 2019

dlorenc approved these changes Jun 26, 2019

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 26, 2019

tekton-robot assigned dlorenc Jun 26, 2019

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 26, 2019

dibyom reviewed Jul 2, 2019

View reviewed changes

wlynch force-pushed the pr branch from 04a2cfc to 726368e Compare July 2, 2019 17:03

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 2, 2019

wlynch closed this Jul 2, 2019

wlynch reopened this Jul 2, 2019

Add GitHub status updates to Pull Request CRD.

2a8ec17

This adds support for the GitHub Status API (https://developer.github.com/v3/repos/statuses/). This accompanies tektoncd#778 and tektoncd#895 to complete initial Pull Request support support for GitHub OAuth.

wlynch force-pushed the pr branch from 726368e to 2a8ec17 Compare July 2, 2019 17:04

vdemeester approved these changes Jul 3, 2019

View reviewed changes

tekton-robot assigned vdemeester Jul 3, 2019

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 3, 2019

tekton-robot merged commit 260c227 into tektoncd:master Jul 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add GitHub status updates to Pull Request CRD. #995

Add GitHub status updates to Pull Request CRD. #995

wlynch commented Jun 20, 2019 •

edited

Loading

wlynch commented Jun 25, 2019

vdemeester left a comment

wlynch commented Jun 25, 2019

vdemeester commented Jun 25, 2019

dibyom commented Jun 25, 2019

wlynch commented Jun 25, 2019

dlorenc commented Jun 26, 2019

afrittoli commented Jun 26, 2019

afrittoli left a comment

afrittoli Jun 26, 2019

wlynch Jun 26, 2019

wlynch Jun 26, 2019

afrittoli commented Jun 26, 2019

dibyom commented Jun 26, 2019

wlynch commented Jun 26, 2019

dlorenc commented Jun 26, 2019

dlorenc commented Jun 26, 2019

wlynch commented Jul 2, 2019

dibyom left a comment

wlynch commented Jul 2, 2019

vdemeester left a comment

tekton-robot commented Jul 3, 2019

Add GitHub status updates to Pull Request CRD. #995

Add GitHub status updates to Pull Request CRD. #995

Conversation

wlynch commented Jun 20, 2019 • edited Loading

Changes

Submitter Checklist

Release Notes

wlynch commented Jun 25, 2019

vdemeester left a comment

Choose a reason for hiding this comment

wlynch commented Jun 25, 2019

vdemeester commented Jun 25, 2019

dibyom commented Jun 25, 2019

wlynch commented Jun 25, 2019

dlorenc commented Jun 26, 2019

afrittoli commented Jun 26, 2019

afrittoli left a comment

Choose a reason for hiding this comment

afrittoli Jun 26, 2019

Choose a reason for hiding this comment

wlynch Jun 26, 2019

Choose a reason for hiding this comment

wlynch Jun 26, 2019

Choose a reason for hiding this comment

afrittoli commented Jun 26, 2019

dibyom commented Jun 26, 2019

wlynch commented Jun 26, 2019

dlorenc commented Jun 26, 2019

dlorenc commented Jun 26, 2019

wlynch commented Jul 2, 2019

dibyom left a comment

Choose a reason for hiding this comment

wlynch commented Jul 2, 2019

vdemeester left a comment

Choose a reason for hiding this comment

tekton-robot commented Jul 3, 2019

wlynch commented Jun 20, 2019 •

edited

Loading