Add PullRequestResource functionality for updating labels and comments. #895

wlynch · 2019-05-22T15:54:38Z

Changes

Add PullRequestResource functionality for updating labels and comments.

Missing from this change:

Statuses, since this might involve more discussion on the set of statuses
that we want to support.
Updating from changes to the raw payloads. Unclear how we want to
handle these at the moment. Punting on this for now.

First step for supporting #778.

This takes over initial work done by @dlorenc in #779

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

Release Notes

Includes label and comment support for GitHub Pull Request CRD.

googlebot · 2019-05-22T15:54:40Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

wlynch · 2019-05-22T15:55:20Z

/assign @dlorenc

bobcatfish

This is looking great!! I am so excited to have this and be able to start using it for our own CI :D :D :D 🤞

I love the fake github that you use to test the github image! I'd also like to see some unit test coverage here too if possible (the tests you've currently got are a kind of "integration test" in my opinion since they test the functionality as a whole)
I think you need to update resource_types.go as well https://github.com/tektoncd/pipeline/blob/ffbb67959dec0abb0f07b1d456398a99531d89f8/pkg/apis/pipeline/v1alpha1/resource_types.go
I'm a bit confused about why we're including labels - I thought we were going to leave them out of scope for now based on the design doc (and cuz systems like bitbucket don't have them?) - np tho if we decided otherwise, I know folks like @vdemeester have really been wanting some automation around labels in our own workflows, so maybe thats important enough that we included it even if bitbucket doesn't support it?

Before we merge this can you also include:

Docs on how to use this new resource type https://github.com/tektoncd/pipeline/blob/master/docs/resources.md - one thing im not clear about is how the secrets work exactly, but some usage docs would help with that :D
An example in the examples dir that uses the PipelineResource
Maybe an end to end test too in the test dir
A bit more detail in the commit message about the context 8f80faf (https://github.com/tektoncd/community/blob/master/standards.md#commit-messages)

/meow space

cmd/pullrequest-init/README.md

cmd/pullrequest-init/fake_github.go

bobcatfish · 2019-05-31T22:15:08Z

cmd/pullrequest-init/fake_github.go

+}
+
+func (g *FakeGitHub) getPullRequest(w http.ResponseWriter, r *http.Request) {
+	id, err := strconv.ParseInt(mux.Vars(r)["number"], 10, 64)


what mux.Vars all about? I tried looking at the docs for the function but I still don't quite understand what's happening here :(

It's how you extract path variables with mux. See https://www.gorillatoolkit.org/pkg/mux

cmd/pullrequest-init/fake_github.go

bobcatfish · 2019-05-31T22:18:41Z

cmd/pullrequest-init/github.go

+// NewGitHubHandler initializes a new handler for interacting with GitHub
+// resources.
+func NewGitHubHandler(ctx context.Context, logger *zap.SugaredLogger, rawURL string) (*GitHubHandler, error) {
+	token := strings.TrimSpace(os.Getenv("GITHUBOAUTHTOKEN"))


hm where does GITHUBOAUTHTOKEN come from? maybe it could be mentioned in the README? (what will be setting this?)

It's passed in as an environment variable from the generated Container spec. Added a note in the README.

bobcatfish · 2019-05-31T22:25:02Z

pkg/apis/pipeline/v1alpha1/pull_request_resource.go

+	Name             string               `json:"name"`
+	Type             PipelineResourceType `json:"type"`
+	URL              string               `json:"url"`
+	GithubOauthToken string               `json:"githubOauthToken"`


is the plan to support more than just oauth token based auth? (disclaimer: i dont know what im talking about 😅 )

Yes. I would like to support GitHub Apps at some point in the future. TBD how we're going to do this (I need to read more about how k8s secret management and access works).

bobcatfish · 2019-05-31T22:25:44Z

pkg/apis/pipeline/v1alpha1/pull_request_resource.go

+}
+
+// GetParams returns the resource params
+func (s PullRequestResource) GetParams() []Param { return []Param{} }


im confused about why GetParams exists when every instance of this interface returns an empty list 🤔

Don't know the answer to this one. =\

Let's raise an issue to see if there was a reason for this?

yep, @wlynch did you create an issue already ?

pkg/apis/pipeline/v1alpha1/pull_request_resource.go

bobcatfish · 2019-05-31T22:28:54Z

pkg/apis/pipeline/v1alpha1/pull_request_resource.go

+	evs := []corev1.EnvVar{}
+	for _, sec := range s.Secrets {
+		switch {
+		case strings.EqualFold(sec.FieldName, "githubOauthToken"):


why are secrets only added if they have this particular name? (maybe im misunderstanding!)

The thought was to pull the value of the token from any (secret name, secret key). We use the FieldName to denote which secret should be used for this so that you can define your k8s with whatever name/key you want and set it in the ResourceSpec with the specified name.

No idea if this is the right way to go about doing this. If there is a better way somewhere else let me know!

Added this to the documentation.

pkg/apis/pipeline/v1alpha1/pull_request_resource.go

tekton-robot · 2019-05-31T22:33:15Z

@bobcatfish:

In response to this:

This is looking great!! I am so excited to have this and be able to start using it for our own CI :D :D :D 🤞

I love the fake github that you use to test the github image! I'd also like to see some unit test coverage here too if possible (the tests you've currently got are a kind of "integration test" in my opinion since they test the functionality as a whole)

I think you need to update resource_types.go as well https://github.com/tektoncd/pipeline/blob/ffbb67959dec0abb0f07b1d456398a99531d89f8/pkg/apis/pipeline/v1alpha1/resource_types.go

Before we merge this can you also include:

Docs on how to use this new resource type https://github.com/tektoncd/pipeline/blob/master/docs/resources.md - one thing im not clear about is how the secrets work exactly, but some usage docs would help with that :D

An example in the examples dir that uses the PipelineResource

Maybe an end to end test too in the test dir

/meow space

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.

bobcatfish · 2019-05-31T22:33:41Z

(p.s. sorry this took me 9 days to get to, should have faster turnaround now :D!)

bobcatfish · 2019-05-31T22:35:57Z

vdemeester

Looking good, just a few questions

vdemeester · 2019-06-04T13:59:46Z

cmd/pullrequest-init/github.go

+// NewGitHubHandler initializes a new handler for interacting with GitHub
+// resources.
+func NewGitHubHandler(ctx context.Context, logger *zap.SugaredLogger, rawURL string) (*GitHubHandler, error) {
+	token := strings.TrimSpace(os.Getenv("GITHUBTOKEN"))


Might be good for a follow-up/enhancement and not as part of this PR but, shouldn't we support using secrets for that instead 👼

Agreed. We're loosely using secrets here, but I'm sure there's better practices that we could be following. If you have any examples I can reference, please send them my way!

vdemeester · 2019-06-04T14:10:36Z

cmd/pullrequest-init/github_test.go

+		t.Errorf("Raw PR path: want [%s], got [%s]", rawCommentPath, gotPR.Comments[0].Raw)
+	}
+
+	// Upload


I originally wanted to do this, but I found that the easiest way to test Upload was to call Download, modify the result of download, then upload. Since a lot of the setup (i.e. populating the fake GitHub server) ends up being duplicative, it seemed simpler to test them together.

The setup for upload could be "hard-coded" file(s) instead of doing Download right ? (that said it might be as long of a setup as calling Download 😅 )

vdemeester · 2019-06-04T14:11:21Z

cmd/pullrequest-init/github_test.go

+		t.Fatal(err)
+	}
+	if diff := cmp.Diff(want, got); diff != "" {
+		t.Errorf("PR Diff: %s", diff)


nit: I think we are using -got +wanted: %s elsewhere

Looks like it's -want +got, but done.

vdemeester · 2019-06-04T14:12:53Z

cmd/pullrequest-init/main.go

+
+	switch *mode {
+	case "download":
+		logger.Info("RUNNING DOWNLOAD!")


vdemeester · 2019-06-04T14:13:03Z

cmd/pullrequest-init/main.go

+			logger.Fatal(err)
+		}
+	case "upload":
+		logger.Info("RUNNING UPLOAD!")


vdemeester · 2019-06-04T14:14:16Z

docs/resources.md

- A `Task`'s output can be your application container image which can be then
-  deployed in a cluster.
- A `Task`'s output can be a jar file to be uploaded to a storage bucket.
+-   A `Task`'s input could be a GitHub source which contains your application


nit: extra spaces 😅 (sorry 🙇‍♂️)

vdemeester · 2019-06-04T14:15:52Z

pkg/apis/pipeline/v1alpha1/pull_request_resource.go

+}
+
+// GetParams returns the resource params
+func (s PullRequestResource) GetParams() []Param { return []Param{} }


pkg/apis/pipeline/v1alpha1/pull_request_resource.go

bobcatfish

Looking great! Looks like from my perspective there are 2 main remaining issues:

Secrets - @vdemeester has some feedback here and I am just generally confused about when we want to use secrets vs. service accounts but you can probably just ignore me and listen to whatever he has to say
Unit test coverage for the binary itself. It looks like I cant reply to your original comment @wlynch cuz it was on code that is changed so here are my thoughts:

This brings up the common debate of individual unit vs integration style tests. The argument for the integration style with an in-memory fake is to write tests closer to the perspective of the user.

I think both styles of tests are super useful, but relying only on integration tests for coverage i think loses one of the biggest strengths of unit tests: they force us to design our code well.

when you have to write unit tests for individual functions, you have to think more about the signatures.

if the unit tests you end up writing are ugly and hard to deal with, there's a good chance that there is something that can be improved or cleaned up in the abstraction you've created through the functions themselves

These arguably have the same coverage as individual unit tests, and are less brittle to changes for how we structure Upload/Downloads (e.g. with fine-grained unit tests we will need to modify tests for any refactoring work vs with coarse integration style unit tests we do not so long as the Upload/Download interface does not change).

I actually think having to change the tests when you change the code is an advantage. when code changes but tests dont change its hard to know if the changes are doing what they were intended to: unit tests come as close as you can get to providing a "proof" that your changes are going in as expected.

like you said i think it's valuable to have tests a level or two up (integration and end to end) to give you that reassurance that the user facing functionality is working, so i wouldnt say dont write the integration tests, but i am saying still write the unit tests :D

I am now going to shame{less/ful}ly plug my own talk on this very topic: Testing CRDs <-- high recommend the sources slide, particularly "integrated tests are a scam" :D

/meow

bobcatfish · 2019-06-05T22:47:01Z

docs/resources.md

+
+Adding the Pull Request resource as an output of a Task will update the source
+control system with any changes made to the pull request resource during the
+pipeline.


aaaamazing 🎉

bobcatfish · 2019-06-05T22:51:32Z

docs/resources.md

+control system with any changes made to the pull request resource during the
+pipeline.
+
+See [types.go](cmd/pullrequest-init/types.go) for the full payload spec.


hm we usually don't link to source code like this for the spec 🤔 maybe we could link to generated go-docs? (i dont see any other cmd info at https://godoc.org/github.com/tektoncd/pipeline tho 🤔 )

in most of our other docs we duplicate this information which i personally think is worth it even if it can be out of date, cuz its way easier to read, e.g. https://github.com/tektoncd/pipeline/blob/master/docs/tasks.md#syntax

Replaced with sample payload.

tfw the person goes along with your request and you feel you have maybe gone too far XD

jk jk I think this is probably good to have :D

bobcatfish · 2019-06-05T22:52:28Z

docs/resources.md

+  secrets:
+    - fieldName: githubToken
+      secretName: github-secrets
+      secretKey: token


hm do we have any idea where we draw the line between when we use the service account attached to the task and when we want ppl to specify secrets @vdemeester @wlynch ?

This is something I want to look into more, particularly when we start looking at GitHub Apps. For now this is probably fine. But a good target for a follow up change.

agreeing with @wlynch we can do a follow-up 👼

bobcatfish · 2019-06-05T22:53:50Z

docs/resources.md

+        -   Manifest indicates that resource should be fetched using a source
+            manifest file.
+        -   If `artifactType` is set to `Manifest`, `location` should point to a
+            source manifest file.


hm was all this whitespace changing intentionally?

No. The standard Google mdformat did this. =\

ah kk 😅 I'm sure @mattmoor-sockpuppet will have something to say about it XD

bobcatfish · 2019-06-05T22:56:04Z

pkg/apis/pipeline/v1alpha1/pull_request_resource.go

+	URL              string               `json:"url"`
+	GithubOauthToken string               `json:"githubOauthToken"`
+	//Secrets holds a struct to indicate a field name and corresponding secret name to populate it
+	Secrets []SecretParam `json:"secrets"`


i have a feeling that Type is being used to inspect PipelineResources without having to use introspection to see specifically what type of resource it is (e.g. if you have a list of PipelineResources ) - but looking for usages might help clear that up (unless it isnt used?)

Name i think is the name of the resource, like wizzbang-git in this example

(or if im totally wrong then i have no idea what they are either)

bobcatfish · 2019-06-05T23:08:41Z

/meow

tekton-robot · 2019-06-05T23:08:42Z

@bobcatfish:

In response to this:

/meow

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.

bobcatfish · 2019-06-19T22:24:16Z

Amaaaazing thanks for trying this out @afrittoli and for the insightful feedback!!! (And the cool demo today!)

googlebot · 2019-06-19T22:27:13Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

Missing from this change: * Statuses, since this might involve more discussion on the set of statuses that we want to support. * Updating from changes to the raw payloads. Unclear how we want to handle these at the moment. Punting on this for now. First step for supporting tektoncd#778. Co-authored-by: Dan Lorenc <[email protected]> Co-authored-by: Billy Lynch <[email protected]>

bobcatfish

looking great! after the commits get squashed i am happy to approve + merge!

🎉🎉🎉

(and then very excited to start using it... haha :D)

bobcatfish · 2019-06-19T23:05:29Z

cmd/pullrequest-init/github_test.go

+	want := []*github.IssueComment{comment2, comment3}
+	if diff := cmp.Diff(want, got); diff != "" {
+		t.Errorf("-want +got: %v", diff)
+	}


nice unit tests!! 😎

googlebot · 2019-06-19T23:13:31Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

bobcatfish · 2019-06-19T23:14:04Z

Manually resolved CLA issue, the commit is co-authored by @dlorenc and @wlynch both from Google

bobcatfish · 2019-06-19T23:14:22Z

/approve
/lgtm
/meow space

tekton-robot · 2019-06-19T23:14:23Z

@bobcatfish:

In response to this:

/approve
/lgtm
/meow space

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.

tekton-robot · 2019-06-19T23:14:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobcatfish, 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~~ [bobcatfish]

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

bobcatfish · 2019-06-20T00:03:20Z

wooooot

🎉

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.

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.

tekton-robot requested review from dlorenc and imjasonh May 22, 2019 15:54

googlebot added the cla: no label May 22, 2019

tekton-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label May 22, 2019

tekton-robot assigned dlorenc May 22, 2019

wlynch force-pushed the pr branch 3 times, most recently from be0580a to 53df976 Compare May 23, 2019 13:08

vdemeester requested review from vdemeester and bobcatfish May 24, 2019 07:45

vdemeester added this to the Pipelines 0.5 🐱 : <cat name tbd @dlorenc> milestone May 28, 2019

wlynch force-pushed the pr branch 4 times, most recently from 0ed9abe to 8f80faf Compare May 30, 2019 16:21

vdemeester mentioned this pull request May 31, 2019

WIP: PullRequestResource #779

Closed

bobcatfish reviewed May 31, 2019

View reviewed changes

wlynch force-pushed the pr branch from b0c3441 to cbe1d60 Compare June 4, 2019 00:50

mnuttall mentioned this pull request Jun 4, 2019

Pull Request / Webhooks integration tektoncd/experimental#36

Closed

vdemeester reviewed Jun 4, 2019

View reviewed changes

mnuttall mentioned this pull request Jun 4, 2019

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

Closed

bobcatfish reviewed Jun 5, 2019

View reviewed changes

tektoncd deleted a comment from tekton-robot Jun 5, 2019

wlynch mentioned this pull request Jun 19, 2019

What is the intended usage of GetParams in CRDs? #991

Closed

googlebot added cla: no and removed cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit labels Jun 19, 2019

wlynch force-pushed the pr branch from c37563e to c172d58 Compare June 19, 2019 22:34

wlynch force-pushed the pr branch from b5a94b2 to b8b43ed Compare June 19, 2019 23:11

bobcatfish reviewed Jun 19, 2019

View reviewed changes

bobcatfish added cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit and removed cla: no labels Jun 19, 2019

tekton-robot assigned bobcatfish Jun 19, 2019

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

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

tekton-robot merged commit 98000b4 into tektoncd:master Jun 19, 2019

wlynch mentioned this pull request Jun 20, 2019

Add GitHub status updates to Pull Request CRD. #995

Merged

3 tasks

ccojocar mentioned this pull request Jun 26, 2019

Evaluate the Tekton pull request reporting jenkins-x/lighthouse#1

Closed

vdemeester mentioned this pull request Jul 9, 2019

Cmd readmes #1057

Merged

3 tasks

Add PullRequestResource functionality for updating labels and comments. #895

Add PullRequestResource functionality for updating labels and comments. #895

Conversation

wlynch commented May 22, 2019

Changes

Submitter Checklist

Release Notes

googlebot commented May 22, 2019

wlynch commented May 22, 2019

bobcatfish left a comment • edited Loading

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

Choose a reason for hiding this comment

tekton-robot commented May 31, 2019

bobcatfish commented May 31, 2019

bobcatfish commented May 31, 2019

vdemeester left a 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

bobcatfish left a 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

bobcatfish commented Jun 5, 2019

tekton-robot commented Jun 5, 2019

bobcatfish commented Jun 19, 2019

googlebot commented Jun 19, 2019

bobcatfish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googlebot commented Jun 19, 2019

bobcatfish commented Jun 19, 2019

bobcatfish commented Jun 19, 2019

tekton-robot commented Jun 19, 2019

tekton-robot commented Jun 19, 2019

bobcatfish commented Jun 20, 2019

bobcatfish left a comment •

edited

Loading