Add link to GitHub Apps authentication package in docs. by bradleyfalzon · Pull Request #649 · google/go-github

bradleyfalzon · 2017-05-26T04:01:01Z

As discussed in #627 I have a package which provides the GitHub Apps Authentication as an Installation http.RoundTripper that I use with this GitHub library successfully. I'm happy to keep maintaining it in an external repository, but a link from this library may assist others in finding the package.

Relates to #455.

dmitshur · 2017-05-26T06:55:24Z

I feel it'd be nice to replace the period with a colon and add a simple basic usage example here, in this README.

You said:

I've sent through a minor one-liner, as I did begin to write a larger section which mostly just duplicated the docs already in https://github.com/bradleyfalzon/ghinstallation, so just pared it back to a minor note. Let me know in the PR if you'd like me to expand upon the docs though, happy to.

The way I see it, in this README, just include a basic usage example for people to get started with. But leave advanced usage, details, explanations in the ghinstallation repo README.

Just like for oauth2, this repo README contains a basic snippet for getting started, but oauth2 package documentation contain more detailed information.

bradleyfalzon · 2017-05-26T07:21:11Z

Sure, I can add a simple example.

Is this OK:

// Shared transport to reuse TCP connections.
tr := &http.Transport{}

// Wrap the shared transport for use with the integration ID 1 authenticating with installation ID 99.
itr, err := ghinstallation.NewKeyFromFile(tr, 1, 99, "2016-10-19.private-key.pem")

// Use installation transport with new client.
client := github.NewClient(http.Client{Transport: itr})

I can make it inline the *http.Transport if you like? The oauth2 example is a complete working program, would you prefer that too?

dmitshur · 2017-05-26T09:15:09Z

I think it'd be helpful to include the import statement. It makes it easier to see the whole thing at a glance. Otherwise it's extra work and uncertainty for people to figure out the import path (it could be a vanity import path, so it requires extra steps to find out).

You can stop after creating a GitHub client, no need to call methods on it. Maybe just a // Use client... comment.

Also, why do you suggest creating a separate http.RoundTripper for it, instead of just using http.DefaultTransport? The zero value of http.Transport{} doesn't seem to have good defaults.

bradleyfalzon · 2017-05-27T01:49:29Z

Sure, updated.

Also, why do you suggest creating a separate http.RoundTripper for it, instead of just using http.DefaultTransport? The zero value of http.Transport{} doesn't seem to have good defaults.

Ah must just be an oversight, GopherCI uses DefaultTransport (https://github.com/bradleyfalzon/gopherci/blob/03badd2364a4a3e85c0e4389c88ca86b7e2f7a70/internal/github/github.go#L38), so I'll modify that example.

dmitshur · 2017-05-27T02:19:57Z

+
+func main() {
+  // Shared transport to reuse TCP connections.
+  tr := http.DefaultTransport


I think you can delete this line and its comment, and pass http.DefaultTransport directly to the ghinstallation.NewKeyFromFile call below.

It's pretty common knowledge that transports should be reused, unless there are specific reasons not to. I don't think you need to elaborate on that here. This is meant to be a minimal simple example to get started quickly.

Agreed, done.

dmitshur · 2017-05-27T02:21:18Z

+  // Wrap the shared transport for use with the integration ID 1 authenticating with installation ID 99.
+  itr, err := ghinstallation.NewKeyFromFile(tr, 1, 99, "2016-10-19.private-key.pem")
+  if err != nil {
+    log.Fatal(err)


Replace with // handle error to leave error handling up to user. We don't want to suggest bad practices, and doing better error handling can be verbose. It's common for usage examples to not elaborate on error handling, just that it should be done.

Sure, done.

dmitshur · 2017-05-27T02:24:53Z

+
+  // Use installation transport with client
+  client := github.NewClient(http.Client{Transport: itr})
+}


Replace } with // use client.... This is an incomplete program anyway (it's missing a package clause), so leaving out the closing } is okay. I don't see why you'd want to include } and make it seem like client isn't used at all.

Yeah, but I was half copying the style of oauth2, I'll make this change, but let me know if you want to make further edits there for consistency.

dmitshur

Hopefully last nitpicky comments.

After this, I think this looks good to me. But I want @gmlewis and/or @willnorris to review the change at a high level - are we okay with adding endorsement for the ghinstallation package in a separate repository?

I am okay with that, because:

As I understand, it adds relevant missing functionality (to help #627).
I briefly looked at the code and it looks clean to me (and it's short).
There are no issues on https://goreportcard.com/report/github.com/bradleyfalzon/ghinstallation.
It only imports 1 external dependency (github.com/dgrijalva/jwt-go), which imports only standard library and looks fairly clean itself too (but I don't have experience with it).
I know @bradleyfalzon and trust his work. I'm sure that he'll maintain the package, or let us know if it should ever be removed from the README because it's no longer supported, etc.
It also helps to know that it's used in production.

dmitshur · 2017-05-27T04:10:53Z

+func main() {
+  // Wrap the shared transport for use with the integration ID 1 authenticating with installation ID 99.
+  itr, err := ghinstallation.NewKeyFromFile(http.DefaultTransport, 1, 99, "2016-10-19.private-key.pem")
+  // Handle error.


I meant like this:

itr, err := ghinstallation.NewKeyFromFile(http.DefaultTransport, 1, 99, "2016-10-19.private-key.pem") if err != nil { // Handle error. }

I think it's a lot more readable at a glance this way, compared to just a plain text comment without the if err != nil { line.

Of course, I wasn't thinking. Done.

dmitshur · 2017-05-27T04:13:22Z

+		client := github.NewClient(http.Client{Transport: itr})
+
+		// Use client...
+


Okay, I see how this looks kinda bad/unbalanced without the closing } for func main() { now. So let's add that back in. Keep the // Use client... comment of course.

Yeah sounds good. Done.

dmitshur · 2017-05-27T04:14:39Z

 For API methods that require HTTP Basic Authentication, use the
 [`BasicAuthTransport`](https://godoc.org/github.com/google/go-github/github#BasicAuthTransport).

+GitHub Apps authentication can be provided by [ghinstallation](https://github.com/bradleyfalzon/ghinstallation).


Add the word "package" after ghinstallation maybe?

GitHub Apps authentication can be provided by the ghinstallation package.

Notice how above it says:

The easiest and recommended way to do this is using the oauth2 library, ...

Agreed, done.

bradleyfalzon · 2017-05-27T11:14:34Z

are we okay with adding endorsement for the ghinstallation package in a separate repository?

The other option is to roll ghinstallation into go-github, I'd have no problem with that. Potentially with the idea to split the single Transport package into IntegrationTransport and InstallationTransport.

dmitshur · 2017-05-28T06:22:34Z

+  }
+
+  // Use installation transport with client.
+  client := github.NewClient(http.Client{Transport: itr})


Minor. Shouldn't this be &http.Client?

Yes it should, fixed, and thanks.

dmitshur · 2017-06-02T19:49:50Z

Friendly ping @gmlewis and @willnorris. I want to make sure you saw my comment from 7 days ago, in case it got buried by the minor typo discussion that followed.

gmlewis · 2017-06-02T19:57:56Z

I'm fine with the endorsement, but before proceeding, I would like confirmation from @willnorris and I defer to his judgement. I'll ask him to review this.

dmitshur · 2017-06-04T02:52:46Z

Great! Merging.

If there are any important updates or changes to the linked package, please let us know @bradleyfalzon. We're here to help. Thanks for creating it!

This is a package that provides an http.RoundTripper for GitHub Apps Authentication as an Installation. It can be used with go-github. Helps google#455. Helps google#627.

dmitshur requested review from gmlewis and willnorris May 26, 2017 06:55

bradleyfalzon force-pushed the ghinstallation branch from 26095af to e97b685 Compare May 27, 2017 01:49

dmitshur reviewed May 27, 2017

View reviewed changes

bradleyfalzon force-pushed the ghinstallation branch 2 times, most recently from 135f498 to 399fdcd Compare May 27, 2017 03:05

dmitshur reviewed May 27, 2017

View reviewed changes

bradleyfalzon force-pushed the ghinstallation branch from 399fdcd to 7c732c8 Compare May 27, 2017 11:05

dmitshur reviewed May 28, 2017

View reviewed changes

Add link to GitHub Apps authentication package in docs.

509b36e

bradleyfalzon force-pushed the ghinstallation branch from 7c732c8 to 509b36e Compare May 28, 2017 22:37

willnorris approved these changes Jun 2, 2017

View reviewed changes

dmitshur merged commit 7a51fb9 into google:master Jun 4, 2017

		client := github.NewClient(http.Client{Transport: itr})

		// Use client...

Conversation

bradleyfalzon commented May 26, 2017

Uh oh!

dmitshur commented May 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bradleyfalzon commented May 26, 2017

Uh oh!

dmitshur commented May 26, 2017

Uh oh!

bradleyfalzon commented May 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitshur left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bradleyfalzon commented May 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitshur commented Jun 2, 2017

Uh oh!

gmlewis commented Jun 2, 2017

Uh oh!

dmitshur commented Jun 4, 2017

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dmitshur commented May 26, 2017 •

edited

Loading

dmitshur left a comment •

edited

Loading