Issue 104700043: oauth2: avoid using http.DefaultTransport
Descriptionhttps://github.com/golang/oauth2/issues/10 Patch Set 1 # Total comments: 2 Patch Set 2 : # Total comments: 8 Patch Set 3 : # Total comments: 8 Patch Set 4 : # Total comments: 2
MessagesTotal messages: 15
https://codereview.appspot.com/104700043/diff/1/google/appenginevm.go File google/appenginevm.go (right): https://codereview.appspot.com/104700043/diff/1/google/appenginevm.go#newcode29 google/appenginevm.go:29: return oauth2.NewAuthorizedTransport(http.DefaultTransport, c, nil) Should I be depending on net/http on Managed VMs? Sign in to reply to this message.
proppy, Can I have a LGTM? https://codereview.appspot.com/104700043/diff/1/google/appenginevm.go File google/appenginevm.go (right): https://codereview.appspot.com/104700043/diff/1/google/appenginevm.go#newcode29 google/appenginevm.go:29: return oauth2.NewAuthorizedTransport(http.DefaultTransport, c, nil) On 2014/07/11 18:02:35, jbd wrote: > Should I be depending on net/http on Managed VMs? I guess so. Therefore, passing a transport that depends on urlfetcher. Sign in to reply to this message.
can you cross-link the github issue in the description? https://codereview.appspot.com/104700043/diff/20001/google/appenginevm.go File google/appenginevm.go (right): https://codereview.appspot.com/104700043/diff/20001/google/appenginevm.go#new... google/appenginevm.go:11: "google.golang.org/appengine/urlfetch" we don't really need urlfetch on managed vm. https://codereview.appspot.com/104700043/diff/20001/google/google.go File google/google.go (right): https://codereview.appspot.com/104700043/diff/20001/google/google.go#newcode79 google/google.go:79: resp, err := (&http.Client{}).Do(req) http.DefaultClient? https://codereview.appspot.com/104700043/diff/20001/jwt.go File jwt.go (right): https://codereview.appspot.com/104700043/diff/20001/jwt.go#newcode103 jwt.go:103: client := http.Client{} http.DefaultClient? https://codereview.appspot.com/104700043/diff/20001/oauth2.go File oauth2.go (right): https://codereview.appspot.com/104700043/diff/20001/oauth2.go#newcode209 oauth2.go:209: r, err := (&http.Client{}).PostForm(c.tokenURL, v) http.DefaultClient? Sign in to reply to this message.
Reverted back the urlfetcher changes for appenginevm. https://codereview.appspot.com/104700043/diff/20001/google/appenginevm.go File google/appenginevm.go (right): https://codereview.appspot.com/104700043/diff/20001/google/appenginevm.go#new... google/appenginevm.go:11: "google.golang.org/appengine/urlfetch" On 2014/07/14 17:19:59, proppy wrote: > we don't really need urlfetch on managed vm. Reverted the previous change. https://codereview.appspot.com/104700043/diff/20001/google/google.go File google/google.go (right): https://codereview.appspot.com/104700043/diff/20001/google/google.go#newcode79 google/google.go:79: resp, err := (&http.Client{}).Do(req) On 2014/07/14 17:19:59, proppy wrote: > http.DefaultClient? Done. https://codereview.appspot.com/104700043/diff/20001/jwt.go File jwt.go (right): https://codereview.appspot.com/104700043/diff/20001/jwt.go#newcode103 jwt.go:103: client := http.Client{} On 2014/07/14 17:19:59, proppy wrote: > http.DefaultClient? Done. https://codereview.appspot.com/104700043/diff/20001/oauth2.go File oauth2.go (right): https://codereview.appspot.com/104700043/diff/20001/oauth2.go#newcode209 oauth2.go:209: r, err := (&http.Client{}).PostForm(c.tokenURL, v) On 2014/07/14 17:19:59, proppy wrote: > http.DefaultClient? Done. Sign in to reply to this message.
https://codereview.appspot.com/104700043/diff/40001/google/appengine.go File google/appengine.go (right): https://codereview.appspot.com/104700043/diff/40001/google/appengine.go#newco... google/appengine.go:29: func (c *AppEngineConfig) NewTransport() oauth2.Transport { you might give want to give the option to user to override the urlfetch transport default (or supply their own). It can be useful for them to be able to control the deadline for example. https://codereview.appspot.com/104700043/diff/40001/oauth2_test.go File oauth2_test.go (left): https://codereview.appspot.com/104700043/diff/40001/oauth2_test.go#oldcode52 oauth2_test.go:52: DefaultTransport = &mockTransport{ Isn't this global removed by this patch? Does this CL pass travis? https://codereview.appspot.com/104700043/diff/40001/oauth2_test.go#oldcode70 oauth2_test.go:70: DefaultTransport = &mockTransport{ Same comment here. Sign in to reply to this message.
https://codereview.appspot.com/104700043/diff/40001/google/appengine.go File google/appengine.go (right): https://codereview.appspot.com/104700043/diff/40001/google/appengine.go#newco... google/appengine.go:29: func (c *AppEngineConfig) NewTransport() oauth2.Transport { On 2014/07/14 17:35:19, proppy wrote: > you might give want to give the option to user to override the urlfetch > transport default (or supply their own). > > It can be useful for them to be able to control the deadline for example. Rather than asking for a context, maybe we should ask for a transport. NewAppEngineConfig(transport *urlfetch.Transport, scopes []string) https://codereview.appspot.com/104700043/diff/40001/oauth2_test.go File oauth2_test.go (left): https://codereview.appspot.com/104700043/diff/40001/oauth2_test.go#oldcode52 oauth2_test.go:52: DefaultTransport = &mockTransport{ On 2014/07/14 17:35:19, proppy wrote: > Isn't this global removed by this patch? Does this CL pass travis? This is http.DefaultTransport, not oauth2.DefaultTransport. https://codereview.appspot.com/104700043/diff/40001/oauth2_test.go#oldcode70 oauth2_test.go:70: DefaultTransport = &mockTransport{ On 2014/07/14 17:35:19, proppy wrote: > Same comment here. Same as above. Sign in to reply to this message.
https://codereview.appspot.com/104700043/diff/40001/google/appengine.go File google/appengine.go (right): https://codereview.appspot.com/104700043/diff/40001/google/appengine.go#newco... google/appengine.go:29: func (c *AppEngineConfig) NewTransport() oauth2.Transport { On 2014/07/14 17:44:58, jbd wrote: > On 2014/07/14 17:35:19, proppy wrote: > > you might give want to give the option to user to override the urlfetch > > transport default (or supply their own). > > > > It can be useful for them to be able to control the deadline for example. > > Rather than asking for a context, maybe we should ask for a transport. > > NewAppEngineConfig(transport *urlfetch.Transport, scopes []string) NewAppEngineConfigWithTransport? https://codereview.appspot.com/104700043/diff/40001/oauth2_test.go File oauth2_test.go (left): https://codereview.appspot.com/104700043/diff/40001/oauth2_test.go#oldcode52 oauth2_test.go:52: DefaultTransport = &mockTransport{ On 2014/07/14 17:44:58, jbd wrote: > On 2014/07/14 17:35:19, proppy wrote: > > Isn't this global removed by this patch? Does this CL pass travis? > > This is http.DefaultTransport, not oauth2.DefaultTransport. maybe my patch display is buggy, but I don't see oauth2.DefaultTransport being defined. Nor I see how you can override http.DefaultTransport without the 'http' package prefix. Sign in to reply to this message.
PTAL, I exported a transport: AppEngineConfig.Transport Sign in to reply to this message.
On 2014/07/14 17:48:42, proppy wrote: > https://codereview.appspot.com/104700043/diff/40001/google/appengine.go > File google/appengine.go (right): > > https://codereview.appspot.com/104700043/diff/40001/google/appengine.go#newco... > google/appengine.go:29: func (c *AppEngineConfig) NewTransport() > oauth2.Transport { > On 2014/07/14 17:44:58, jbd wrote: > > On 2014/07/14 17:35:19, proppy wrote: > > > you might give want to give the option to user to override the urlfetch > > > transport default (or supply their own). > > > > > > It can be useful for them to be able to control the deadline for example. > > > > Rather than asking for a context, maybe we should ask for a transport. > > > > NewAppEngineConfig(transport *urlfetch.Transport, scopes []string) > > NewAppEngineConfigWithTransport? > > https://codereview.appspot.com/104700043/diff/40001/oauth2_test.go > File oauth2_test.go (left): > > https://codereview.appspot.com/104700043/diff/40001/oauth2_test.go#oldcode52 > oauth2_test.go:52: DefaultTransport = &mockTransport{ > On 2014/07/14 17:44:58, jbd wrote: > > On 2014/07/14 17:35:19, proppy wrote: > > > Isn't this global removed by this patch? Does this CL pass travis? > > > > This is http.DefaultTransport, not oauth2.DefaultTransport. > > maybe my patch display is buggy, but I don't see oauth2.DefaultTransport being > defined. Nor I see how you can override http.DefaultTransport without the 'http' > package prefix. Yes, filed: https://code.google.com/p/rietveld/issues/detail?id=497 Sign in to reply to this message.
https://codereview.appspot.com/104700043/diff/60001/google/appengine.go File google/appengine.go (right): https://codereview.appspot.com/104700043/diff/60001/google/appengine.go#newco... google/appengine.go:46: token, expiry, err := appengine.AccessToken(c.context, strings.Join(c.scopes, " ")) maybe you can test if context is nil, and use it from the Transport if that's the case. Sign in to reply to this message.
https://codereview.appspot.com/104700043/diff/60001/google/appengine.go File google/appengine.go (right): https://codereview.appspot.com/104700043/diff/60001/google/appengine.go#newco... google/appengine.go:46: token, expiry, err := appengine.AccessToken(c.context, strings.Join(c.scopes, " ")) On 2014/07/14 18:14:34, proppy wrote: > maybe you can test if context is nil, and use it from the Transport if that's > the case. You should never initiate this config with a nil context. Sign in to reply to this message.
I'm merging this, additional improvements can wait another CL. Sign in to reply to this message.
Submitted. Sign in to reply to this message. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
