Fix Google App Engine support and provide working example #697
Conversation
Fixes google#578. Change-Id: Id7f3cc079d12b9d6d27baa2308c78a04ceaeb181
github/with_appengine.go
Outdated
| ) | ||
|
|
||
| func withContext(ctx context.Context, req *http.Request) (context.Context, *http.Request) { | ||
| return appengine.WithContext(ctx, req), req | ||
| return ctx, req |
There was a problem hiding this comment.
Given this change, I think you can simplify withContext signature to return only *http.Request then. Can you?
There was a problem hiding this comment.
Also, now that this func is a no-op for appengine case, I think you should add a comment explaining that. E.g.:
// No-op implementation for appengine because it doesn't support adding context to a request.
return reqChange-Id: I8aa5b61c4c65e7dc70ff6ad17d9bbc52f2c52b28
| &oauth2.Token{AccessToken: os.Getenv("GITHUB_AUTH_TOKEN")}, | ||
| ) | ||
| tc := oauth2.NewClient(ctx, ts) | ||
| client := github.NewClient(tc) |
There was a problem hiding this comment.
So a new client must be made for every incoming request? That seems pretty unfortunate.
There was a problem hiding this comment.
This was expedient for this demo, but I believe it is possible to make a single client as a package global during initialization.
One caveat is that you can't call os.Getenv on App Engine within an init function due to the environment not being fully passed-in on app startup. The details are quite hairy.
So in cases like that, you could have a sync.Once block get the environment variable value and create the client before its first use... but I wanted to have a simple example here and not complicate things too much.
example/appengine/app.go
Outdated
| commits, _, err := client.Repositories.ListCommits(ctx, "google", "go-github", nil) | ||
| if err != nil { | ||
| log.Errorf(ctx, "ListCommits: %v", err) | ||
| return |
There was a problem hiding this comment.
You haven't written anything to w yet, so you could also do http.Error(w, err.Error(), http.Status...) here.
| @@ -399,7 +399,7 @@ func parseRate(r *http.Response) Rate { | |||
| // The provided ctx must be non-nil. If it is canceled or times out, | |||
| // ctx.Err() will be returned. | |||
| func (c *Client) Do(ctx context.Context, req *http.Request, v interface{}) (*Response, error) { | |||
| ctx, req = withContext(ctx, req) | |||
| req = withContext(ctx, req) | |||
There was a problem hiding this comment.
This is one of those rare times where I feel it might be worth breaking the "context is always the first parameter" pattern.
Here, withContext is basically a method-as-a-function that adds ctx to req. I think seeing it like this:
req = withContext(req, ctx)Might read better...
Well, after writing it up and looking at it for a few minutes, I'm not so sure it looks better. It seems about about the same. 😑
There was a problem hiding this comment.
Agreed. I'll leave it alone.
Change-Id: I2a5ef31709f7333004fc8e4f29761e46e6236900
|
Thank you, @shurcooL! Merging. |
Fixes #578.