Using go-github on Google App Engine with recent context changes - any interest?

[gmlewis]

After importing the recent context changes in #529 into Google, I became aware that it is a bit trickier now to use this same code base either within an App Engine project or outside of one.

I had to pull a few tricks with // +build appengine and // +build !appengine in a couple new files and patched a couple other files to make it all work.

I'm not sure if anyone is really interested in these changes, but if you are, please let me know and I'll see if I can replicate the needed changes in a new PR and incorporate them into the library.

[dmitshur]

This was recently mentioned in #526 (comment) by @samuelkaufman, FYI.

[samuelkaufman]

@gmlewis yeah I'm definitely interested. I'd managed to avoid vendoring up until Friday, when I had to pin this repo to the context changes :) I'd love to be able to keep backwards compatibility in this library so I could stay up to date (until AppEngine upgrades the go version)

[gmlewis]

OK, I'll put together a PR then, @samuelkaufman... hopefully later today.

[bradleyfalzon]

You may have already noticed, but there was some announcements affected Go on App Engine, in particular Go 1.8 on App Engine flexible.

Docs: https://cloud.google.com/appengine/docs/go/
Announcement: https://cloudplatform.googleblog.com/2017/03/Google-Cloud-Platform-your-Next-home-in-the-cloud.html

Just an FYI in case this changes the plans for App Engine support, but Standard environment is still 1.6.

[samuelkaufman]

@bradleyfalzon Thanks for the heads up.
It doesn't change anything for me, I use the standard environment. The flex environment you can use whatever you want anyways, sounds like they just updated the Dockerfile to support 1.8.

[ianrose14]

@gmlewis Can you help me understand how #582 is supposed to work? It looks to me like withContext in with_appengine.go is (in most/all situations) being passed a newly created http.Request (i.e. the one that is going to be used to make a request to github). In which case appengine.WithContext will panic with "appengine: NewContext passed an unknown http.Request". What am I missing here?

[gmlewis]

@ianrose14 - I'm not sure I understand why you claim that a panic will occur.

Please take a look at api_classic.go lines 62-65
and note that a new App Engine context is being created with appengine.NewContext(r).

Also note that we are only talking about App Engine Classic here, as stated in the docs from #582.

If you still think this is an issue, can you please provide a code snippet showing what is causing the panic and I will investigate? Thanks.

[ianrose14]

@gmlewis In the code you linked above, isn't it true that req must be an incoming http.Request that came through app engine's http server (and passed to your handler)? IIUC if you pass some random http.Request (e.g. one from http.NewRequest) then it will panic. The godocs hint (but are not 100% clear) at this by their use of "an in-flight HTTP request" wording.

[gmlewis]

Yes, req comes through App Engine's http server and is passed to your handler.

Are you trying to pass some random http.Request to go-github while running on App Engine?
Yes, that would probably cause a panic and is not the intended use case here.

[ianrose14]

Hmm. Sorry, I must be being dense. As an example, here is a place where the code creates a brand new (non-appengine) http request, and then a few lines later passes it as req to s.client.Do() which immediately calls withContext with that req.

I'm having trouble seeing how that code path won't lead to appengine panicking since req is a freshly created http request, not an in-flight appengine request.

[gmlewis]

Edit: ~~~I think I see the confusion. ListCommits takes a context.Context that was already generated with appengine.NewContext(req)... so the second appengine.WithContext is a NO-OP.~~~ <= That was bogus.

Let me whip up a simple example for App Engine, make sure it works, and then copy it into this thread.

[gmlewis]

Update: It looks like I need to apologize. I thought I had fully tested this, but I now see exactly what you are talking about, @ianrose14. This is broken. I'm going to reopen this issue and get it working with an example.

[ianrose14]

👍 thanks!