-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Support Google App Engine with recent context changes #582
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
// Copyright 2017 The go-github AUTHORS. All rights reserved. | ||
// | ||
// Use of this source code is governed by a BSD-style | ||
// license that can be found in the LICENSE file. | ||
|
||
// +build appengine | ||
|
||
// This file provides glue for making github work on App Engine. | ||
// In order to get the entire github package to compile with | ||
// Go 1.6, you will need to rewrite all the import "context" lines. | ||
// Fortunately, this is easy with "gofmt": | ||
// | ||
// gofmt -w -r '"context" -> "golang.org/x/net/context"' *.go | ||
|
||
package github | ||
|
||
import ( | ||
"context" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't use App Engine, so I'm not familiar. If #526 (comment) is accurate, and App Engine uses/forces you to use Go 1.6, how does this work? Package There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's an excellent point, and I believe this was an oversight on my part. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Although now that I'm thinking about this a little more, I'm wondering how the other I think I'm going to have to do some more experimentation with this PR before we call it ready for review. Sorry, @shurcooL. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If that's the case, that's going to be an issue. Just about every single file in package
If we need the appengine version to import That would be a pretty significant disturbance, and it was one of the main reasons we decided to wait for Go 1.8 to come out, so that we could drop support for 1.6 and older before adding context support to this package. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. I seem to remember hearing Dave Symonds say that the two implementations are 100% compatible... so maybe I don't need to change things after all... I'm just not sure if the Go 1.6 compiler will actually find the bare |
||
"net/http" | ||
|
||
"google.golang.org/appengine" | ||
) | ||
|
||
func withContext(ctx context.Context, req *http.Request) (context.Context, *http.Request) { | ||
return appengine.WithContext(ctx, req), req | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
// Copyright 2017 The go-github AUTHORS. All rights reserved. | ||
// | ||
// Use of this source code is governed by a BSD-style | ||
// license that can be found in the LICENSE file. | ||
|
||
// +build !appengine | ||
|
||
// This file provides glue for making github work without App Engine. | ||
|
||
package github | ||
|
||
import ( | ||
"context" | ||
"net/http" | ||
) | ||
|
||
func withContext(ctx context.Context, req *http.Request) (context.Context, *http.Request) { | ||
return ctx, req.WithContext(ctx) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Asking users to modify the source code of a package in order to get it to work is pretty unusual, and I feel this comment would be hard to spot.
I think a nice thing we can do for users is mention this in a new "App Engine" section of README (and package comment, to keep them in sync). It can simply state the current situation, that the package is expected not to build by default and needs to be modified, and point to this file for further instructions. Similar to what you wrote in #582 (comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM.
Done.