|
|
Hello golang-dev@googlegroups.com (cc: adg@golang.org), I'd like you to review this change to https://code.google.com/p/goauth2
https://codereview.appspot.com/9998043/diff/2001/oauth/appengine/serviceaccou... File oauth/appengine/serviceaccount.go (right): https://codereview.appspot.com/9998043/diff/2001/oauth/appengine/serviceaccou... oauth/appengine/serviceaccount.go:47: func GetAccessToken(c appengine.Context, scopes ...string) (string, error) { s/Get// Maybe this should be CachedAccessToken, as distinct from appengine.AccessToken. The token is per-app, right? So there's no issue here with sharing the cache key between multiple instances of the same app? https://codereview.appspot.com/9998043/diff/2001/oauth/appengine/serviceaccou... oauth/appengine/serviceaccount.go:67: func NewClient(c appengine.Context, scopes ...string) (*http.Client, error) { A recent change to the oauth package means you should be able to just construct and use a regular oauth.Transport for this: return &http.Client{Transport:&oauth.Transport{ Token:&oauth.Token{AccessToken: token}, Transport: &urlfetch.Transport{...}, }}
https://codereview.appspot.com/9998043/diff/2001/oauth/appengine/serviceaccou... File oauth/appengine/serviceaccount.go (right): https://codereview.appspot.com/9998043/diff/2001/oauth/appengine/serviceaccou... oauth/appengine/serviceaccount.go:17: // client, err := serviceaccount.NewClient(c, "https://www.googleapis.com/auth/compute", "https://www.googleapis.com/auth/bigquery") check the errors in the example, please https://codereview.appspot.com/9998043/diff/2001/oauth/appengine/serviceaccou... oauth/appengine/serviceaccount.go:24: // Transport: &accesstoken.Transport{ accesstoken.Transport? that doesn't exist. but you won't need it (see lower comments)
https://codereview.appspot.com/9998043/diff/2001/oauth/appengine/serviceaccou... File oauth/appengine/serviceaccount.go (right): https://codereview.appspot.com/9998043/diff/2001/oauth/appengine/serviceaccou... oauth/appengine/serviceaccount.go:17: // client, err := serviceaccount.NewClient(c, "https://www.googleapis.com/auth/compute", "https://www.googleapis.com/auth/bigquery") On 2013年06月05日 06:47:59, adg wrote: > check the errors in the example, please Done. https://codereview.appspot.com/9998043/diff/2001/oauth/appengine/serviceaccou... oauth/appengine/serviceaccount.go:24: // Transport: &accesstoken.Transport{ On 2013年06月05日 06:47:59, adg wrote: > accesstoken.Transport? that doesn't exist. but you won't need it (see lower > comments) type, renamed the package since then, Done. https://codereview.appspot.com/9998043/diff/2001/oauth/appengine/serviceaccou... oauth/appengine/serviceaccount.go:47: func GetAccessToken(c appengine.Context, scopes ...string) (string, error) { On 2013年06月05日 06:46:59, adg wrote: > s/Get// > > Maybe this should be CachedAccessToken, as distinct from appengine.AccessToken. > > The token is per-app, right? So there's no issue here with sharing the cache key > between multiple instances of the same app? I splitted the cache in a separate file, and implement oauth.Cache interface. The cache is indeed per app, but I made the scopes part of the key. https://codereview.appspot.com/9998043/diff/2001/oauth/appengine/serviceaccou... oauth/appengine/serviceaccount.go:67: func NewClient(c appengine.Context, scopes ...string) (*http.Client, error) { On 2013年06月05日 06:46:59, adg wrote: > A recent change to the oauth package means you should be able to just construct > and use a regular oauth.Transport for this: > > return &http.Client{Transport:&oauth.Transport{ > Token:&oauth.Token{AccessToken: token}, > Transport: &urlfetch.Transport{...}, > }} Cool, I just looked into this, but I can't really reuse oauth.Transport there because the Refresh logic is different: but I did mimic the interface of it (by moving the AccessToken call in a Refresh method): see the new implementation.
https://codereview.appspot.com/9998043/diff/13001/oauth/appengine/serviceacco... File oauth/appengine/serviceaccount/cache.go (right): https://codereview.appspot.com/9998043/diff/13001/oauth/appengine/serviceacco... oauth/appengine/serviceaccount/cache.go:25: const KeyPrefix = "goauth2_appengine_serviceaccount_cache_" If you require the user specify the key, I wouldn't worry about the prefix. https://codereview.appspot.com/9998043/diff/13001/oauth/appengine/serviceacco... File oauth/appengine/serviceaccount/serviceaccount.go (right): https://codereview.appspot.com/9998043/diff/13001/oauth/appengine/serviceacco... oauth/appengine/serviceaccount/serviceaccount.go:5: // +build !appengine this is wrong, surely? we only want to build this on app engine https://codereview.appspot.com/9998043/diff/13001/oauth/appengine/serviceacco... oauth/appengine/serviceaccount/serviceaccount.go:21: // } indent https://codereview.appspot.com/9998043/diff/13001/oauth/appengine/serviceacco... oauth/appengine/serviceaccount/serviceaccount.go:57: transport := &Transport{ s/transport/t/ https://codereview.appspot.com/9998043/diff/13001/oauth/appengine/serviceacco... oauth/appengine/serviceaccount/serviceaccount.go:81: type Transport struct { This is OK for now, but we need to improve the oauth.Transport to make it easy to do this. https://codereview.appspot.com/9998043/diff/13001/oauth/appengine/serviceacco... oauth/appengine/serviceaccount/serviceaccount.go:125: newReq.Header.Set("Authorization", "Bearer "+t.AccessToken) you need to clone the headers, too. just copy the cloneRequest function from goauth2's oauth.go
PTAL https://codereview.appspot.com/9998043/diff/13001/oauth/appengine/serviceacco... File oauth/appengine/serviceaccount/cache.go (right): https://codereview.appspot.com/9998043/diff/13001/oauth/appengine/serviceacco... oauth/appengine/serviceaccount/cache.go:25: const KeyPrefix = "goauth2_appengine_serviceaccount_cache_" On 2013年06月06日 00:35:54, adg wrote: > If you require the user specify the key, I wouldn't worry about the prefix. Done. https://codereview.appspot.com/9998043/diff/13001/oauth/appengine/serviceacco... File oauth/appengine/serviceaccount/serviceaccount.go (right): https://codereview.appspot.com/9998043/diff/13001/oauth/appengine/serviceacco... oauth/appengine/serviceaccount/serviceaccount.go:5: // +build !appengine On 2013年06月06日 00:35:54, adg wrote: > this is wrong, surely? we only want to build this on app engine Done. In flagrante delicto of cargo cult programming. I was also confused because the go tool contained in the SDK wouldn't set the appengine build tag by default. https://codereview.appspot.com/9998043/diff/13001/oauth/appengine/serviceacco... oauth/appengine/serviceaccount/serviceaccount.go:21: // } On 2013年06月06日 00:35:54, adg wrote: > indent Done. https://codereview.appspot.com/9998043/diff/13001/oauth/appengine/serviceacco... oauth/appengine/serviceaccount/serviceaccount.go:57: transport := &Transport{ On 2013年06月06日 00:35:54, adg wrote: > s/transport/t/ Done. https://codereview.appspot.com/9998043/diff/13001/oauth/appengine/serviceacco... oauth/appengine/serviceaccount/serviceaccount.go:81: type Transport struct { On 2013年06月06日 00:35:54, adg wrote: > This is OK for now, but we need to improve the oauth.Transport to make it easy > to do this. filed: https://code.google.com/p/goauth2/issues/detail?id=18 https://codereview.appspot.com/9998043/diff/13001/oauth/appengine/serviceacco... oauth/appengine/serviceaccount/serviceaccount.go:125: newReq.Header.Set("Authorization", "Bearer "+t.AccessToken) On 2013年06月06日 00:35:54, adg wrote: > you need to clone the headers, too. > > just copy the cloneRequest function from goauth2's oauth.go Made oauth.CloneRequest public.
https://codereview.appspot.com/9998043/diff/13001/oauth/appengine/serviceacco... File oauth/appengine/serviceaccount/serviceaccount.go (right): https://codereview.appspot.com/9998043/diff/13001/oauth/appengine/serviceacco... oauth/appengine/serviceaccount/serviceaccount.go:81: type Transport struct { On 2013年06月06日 14:58:10, proppy wrote: > On 2013年06月06日 00:35:54, adg wrote: > > This is OK for now, but we need to improve the oauth.Transport to make it easy > > to do this. > > filed: https://code.google.com/p/goauth2/issues/detail?id=18 Thanks https://codereview.appspot.com/9998043/diff/13001/oauth/appengine/serviceacco... oauth/appengine/serviceaccount/serviceaccount.go:125: newReq.Header.Set("Authorization", "Bearer "+t.AccessToken) On 2013年06月06日 14:58:10, proppy wrote: > On 2013年06月06日 00:35:54, adg wrote: > > you need to clone the headers, too. > > > > just copy the cloneRequest function from goauth2's oauth.go > > Made oauth.CloneRequest public. No, don't do that. There is no reason that should be part of the oauth package's exported interface. Plus it only does the right thing in this specific instance. There is nothing wrong with copying a bit of trivial code.
On 2013年06月06日 23:26:17, adg wrote: > https://codereview.appspot.com/9998043/diff/13001/oauth/appengine/serviceacco... > File oauth/appengine/serviceaccount/serviceaccount.go (right): > > https://codereview.appspot.com/9998043/diff/13001/oauth/appengine/serviceacco... > oauth/appengine/serviceaccount/serviceaccount.go:81: type Transport struct { > On 2013年06月06日 14:58:10, proppy wrote: > > On 2013年06月06日 00:35:54, adg wrote: > > > This is OK for now, but we need to improve the oauth.Transport to make it > easy > > > to do this. > > > > filed: https://code.google.com/p/goauth2/issues/detail?id=18 > > Thanks > > https://codereview.appspot.com/9998043/diff/13001/oauth/appengine/serviceacco... > oauth/appengine/serviceaccount/serviceaccount.go:125: > newReq.Header.Set("Authorization", "Bearer "+t.AccessToken) > On 2013年06月06日 14:58:10, proppy wrote: > > On 2013年06月06日 00:35:54, adg wrote: > > > you need to clone the headers, too. > > > > > > just copy the cloneRequest function from goauth2's oauth.go > > > > Made oauth.CloneRequest public. > > No, don't do that. There is no reason that should be part of the oauth package's > exported interface. Plus it only does the right thing in this specific instance. > > There is nothing wrong with copying a bit of trivial code. Very true, I might quote this in my OSCON talk :)
On 2013年06月07日 00:29:41, proppy wrote: > On 2013年06月06日 23:26:17, adg wrote: > > > https://codereview.appspot.com/9998043/diff/13001/oauth/appengine/serviceacco... > > File oauth/appengine/serviceaccount/serviceaccount.go (right): > > > > > https://codereview.appspot.com/9998043/diff/13001/oauth/appengine/serviceacco... > > oauth/appengine/serviceaccount/serviceaccount.go:81: type Transport struct { > > On 2013年06月06日 14:58:10, proppy wrote: > > > On 2013年06月06日 00:35:54, adg wrote: > > > > This is OK for now, but we need to improve the oauth.Transport to make it > > easy > > > > to do this. > > > > > > filed: https://code.google.com/p/goauth2/issues/detail?id=18 > > > > Thanks > > > > > https://codereview.appspot.com/9998043/diff/13001/oauth/appengine/serviceacco... > > oauth/appengine/serviceaccount/serviceaccount.go:125: > > newReq.Header.Set("Authorization", "Bearer "+t.AccessToken) > > On 2013年06月06日 14:58:10, proppy wrote: > > > On 2013年06月06日 00:35:54, adg wrote: > > > > you need to clone the headers, too. > > > > > > > > just copy the cloneRequest function from goauth2's oauth.go > > > > > > Made oauth.CloneRequest public. > > > > No, don't do that. There is no reason that should be part of the oauth > package's > > exported interface. Plus it only does the right thing in this specific > instance. > > > > There is nothing wrong with copying a bit of trivial code. > > Very true, I might quote this in my OSCON talk :) and I forgot to say PTAL.
Looking good, but can you put this in the root/appengine/serviceaccount directory (ie move it one level up?)
On 2013年06月18日 03:54:20, adg wrote: > Looking good, but can you put this in the root/appengine/serviceaccount > directory (ie move it one level up?) Given that the jwt/ stuff is also under oauth/ do you want to also move it 1 level up? (Maybe we could do that in a separate re-org cl, if that's the case).
On 19 June 2013 07:59, <proppy@google.com> wrote: > Given that the jwt/ stuff is also under oauth/ do you want to also move > it 1 level up? > > (Maybe we could do that in a separate re-org cl, if that's the case). > I do want to do that at some point, but not now. There are people that depend on it.
On 2013年06月18日 22:03:28, adg wrote: > On 19 June 2013 07:59, <mailto:proppy@google.com> wrote: > > > Given that the jwt/ stuff is also under oauth/ do you want to also move > > it 1 level up? > > > > (Maybe we could do that in a separate re-org cl, if that's the case). > > > > I do want to do that at some point, but not now. There are people that > depend on it. PTAL
https://codereview.appspot.com/9998043/diff/34001/appengine/serviceaccount/se... File appengine/serviceaccount/serviceaccount.go (right): https://codereview.appspot.com/9998043/diff/34001/appengine/serviceaccount/se... appengine/serviceaccount/serviceaccount.go:26: // transport := &serviceaccount.Transport{ I think it might be worth unexporting the Transport type; what do you think?
https://codereview.appspot.com/9998043/diff/34001/appengine/serviceaccount/se... File appengine/serviceaccount/serviceaccount.go (right): https://codereview.appspot.com/9998043/diff/34001/appengine/serviceaccount/se... appengine/serviceaccount/serviceaccount.go:26: // transport := &serviceaccount.Transport{ On 2013年06月19日 03:32:47, adg wrote: > I think it might be worth unexporting the Transport type; what do you think? Then how do stack this on a customTransport?
R=adg
On 2013年06月19日 18:13:12, proppy wrote: > https://codereview.appspot.com/9998043/diff/34001/appengine/serviceaccount/se... > File appengine/serviceaccount/serviceaccount.go (right): > > https://codereview.appspot.com/9998043/diff/34001/appengine/serviceaccount/se... > appengine/serviceaccount/serviceaccount.go:26: // transport := > &serviceaccount.Transport{ > On 2013年06月19日 03:32:47, adg wrote: > > I think it might be worth unexporting the Transport type; what do you think? > > Then how do stack this on a customTransport? s/do/to/
https://codereview.appspot.com/9998043/diff/34001/appengine/serviceaccount/se... File appengine/serviceaccount/serviceaccount.go (right): https://codereview.appspot.com/9998043/diff/34001/appengine/serviceaccount/se... appengine/serviceaccount/serviceaccount.go:26: // transport := &serviceaccount.Transport{ On 2013年06月19日 18:13:12, proppy wrote: > On 2013年06月19日 03:32:47, adg wrote: > > I think it might be worth unexporting the Transport type; what do you think? > > Then how do stack this on a customTransport? Why would you ever want to use anything other than urlfetch?
You might want to also add googleapi.transport.ApiKey but I agree you can always stack them the other way around and put serviceaccount on the bottom. It really depend if you want to enforce that or not. On Jun 19, 2013 4:46 PM, <proppy@google.com> wrote: > On 2013年06月19日 18:13:12, proppy wrote: > > https://codereview.appspot.**com/9998043/diff/34001/** > appengine/serviceaccount/**serviceaccount.go<https://codereview.appspot.com/9998043/diff/34001/appengine/serviceaccount/serviceaccount.go> > >> File appengine/serviceaccount/**serviceaccount.go (right): >> > > > https://codereview.appspot.**com/9998043/diff/34001/** > appengine/serviceaccount/**serviceaccount.go#newcode26<https://codereview.appspot.com/9998043/diff/34001/appengine/serviceaccount/serviceaccount.go#newcode26> > >> appengine/serviceaccount/**serviceaccount.go:26: // transport := >> &serviceaccount.Transport{ >> On 2013年06月19日 03:32:47, adg wrote: >> > I think it might be worth unexporting the Transport type; what do >> > you think? > > Then how do stack this on a customTransport? >> > > s/do/to/ > > https://codereview.appspot.**com/9998043/<https://codereview.appspot.com/9998... >
What I want is the simplest possible API that is useful. It doesn't need to be a swiss army tool, just the thing that is unambiguously the right thing to use. For service-account auth on appengine, NewClient seems the thing. On 24 June 2013 14:39, Johan Euphrosine <proppy@google.com> wrote: > You might want to also add googleapi.transport.ApiKey but I agree you can > always stack them the other way around and put serviceaccount on the > bottom. It really depend if you want to enforce that or not. > On Jun 19, 2013 4:46 PM, <proppy@google.com> wrote: > >> On 2013年06月19日 18:13:12, proppy wrote: >> >> https://codereview.appspot.**com/9998043/diff/34001/** >> appengine/serviceaccount/**serviceaccount.go<https://codereview.appspot.com/9998043/diff/34001/appengine/serviceaccount/serviceaccount.go> >> >>> File appengine/serviceaccount/**serviceaccount.go (right): >>> >> >> >> https://codereview.appspot.**com/9998043/diff/34001/** >> appengine/serviceaccount/**serviceaccount.go#newcode26<https://codereview.appspot.com/9998043/diff/34001/appengine/serviceaccount/serviceaccount.go#newcode26> >> >>> appengine/serviceaccount/**serviceaccount.go:26: // transport := >>> &serviceaccount.Transport{ >>> On 2013年06月19日 03:32:47, adg wrote: >>> > I think it might be worth unexporting the Transport type; what do >>> >> you think? >> >> Then how do stack this on a customTransport? >>> >> >> s/do/to/ >> >> https://codereview.appspot.**com/9998043/<https://codereview.appspot.com/9998... >> >
I agree, this is specific enough (oauth+appengine+serviceaccount) I think nobody needs the flexibility of overriding the underlying transport. It will also make the doc simpler :) On Jun 23, 2013 9:41 PM, "Andrew Gerrand" <adg@golang.org> wrote: > What I want is the simplest possible API that is useful. It doesn't need > to be a swiss army tool, just the thing that is unambiguously the right > thing to use. For service-account auth on appengine, NewClient seems the > thing. > > > > > On 24 June 2013 14:39, Johan Euphrosine <proppy@google.com> wrote: > >> You might want to also add googleapi.transport.ApiKey but I agree you can >> always stack them the other way around and put serviceaccount on the >> bottom. It really depend if you want to enforce that or not. >> On Jun 19, 2013 4:46 PM, <proppy@google.com> wrote: >> >>> On 2013年06月19日 18:13:12, proppy wrote: >>> >>> https://codereview.appspot.**com/9998043/diff/34001/** >>> appengine/serviceaccount/**serviceaccount.go<https://codereview.appspot.com/9998043/diff/34001/appengine/serviceaccount/serviceaccount.go> >>> >>>> File appengine/serviceaccount/**serviceaccount.go (right): >>>> >>> >>> >>> https://codereview.appspot.**com/9998043/diff/34001/** >>> appengine/serviceaccount/**serviceaccount.go#newcode26<https://codereview.appspot.com/9998043/diff/34001/appengine/serviceaccount/serviceaccount.go#newcode26> >>> >>>> appengine/serviceaccount/**serviceaccount.go:26: // transport := >>>> &serviceaccount.Transport{ >>>> On 2013年06月19日 03:32:47, adg wrote: >>>> > I think it might be worth unexporting the Transport type; what do >>>> >>> you think? >>> >>> Then how do stack this on a customTransport? >>>> >>> >>> s/do/to/ >>> >>> https://codereview.appspot.**com/9998043/<https://codereview.appspot.com/9998... >>> >> >
Do you also want to hide other field like, Context, Scopes, and TokenCache? Should the cache be optional? On 2013年06月24日 04:46:45, proppy wrote: > I agree, this is specific enough (oauth+appengine+serviceaccount) I think > nobody needs the flexibility of overriding the underlying transport. It > will also make the doc simpler :) > On Jun 23, 2013 9:41 PM, "Andrew Gerrand" <mailto:adg@golang.org> wrote: > > > What I want is the simplest possible API that is useful. It doesn't need > > to be a swiss army tool, just the thing that is unambiguously the right > > thing to use. For service-account auth on appengine, NewClient seems the > > thing. > > > > > > > > > > On 24 June 2013 14:39, Johan Euphrosine <mailto:proppy@google.com> wrote: > > > >> You might want to also add googleapi.transport.ApiKey but I agree you can > >> always stack them the other way around and put serviceaccount on the > >> bottom. It really depend if you want to enforce that or not. > >> On Jun 19, 2013 4:46 PM, <mailto:proppy@google.com> wrote: > >> > >>> On 2013年06月19日 18:13:12, proppy wrote: > >>> > >>> https://codereview.appspot.**com/9998043/diff/34001/** > >>> > appengine/serviceaccount/**serviceaccount.go<https://codereview.appspot.com/9998043/diff/34001/appengine/serviceaccount/serviceaccount.go> > >>> > >>>> File appengine/serviceaccount/**serviceaccount.go (right): > >>>> > >>> > >>> > >>> https://codereview.appspot.**com/9998043/diff/34001/** > >>> > appengine/serviceaccount/**serviceaccount.go#newcode26<https://codereview.appspot.com/9998043/diff/34001/appengine/serviceaccount/serviceaccount.go#newcode26> > >>> > >>>> appengine/serviceaccount/**serviceaccount.go:26: // transport := > >>>> &serviceaccount.Transport{ > >>>> On 2013年06月19日 03:32:47, adg wrote: > >>>> > I think it might be worth unexporting the Transport type; what do > >>>> > >>> you think? > >>> > >>> Then how do stack this on a customTransport? > >>>> > >>> > >>> s/do/to/ > >>> > >>> > https://codereview.appspot.**com/9998043/%3Chttps://codereview.appspot.com/99...> > >>> > >> > >
On 2013年06月25日 00:51:45, proppy wrote: > Do you also want to hide other field like, Context, Scopes, and TokenCache? > > Should the cache be optional? Let's start by only exporting NewClient. The fields on the Transport struct can stay exported, but Transport should be "transport" (unexported).
On 2013年06月25日 04:09:22, adg wrote: > On 2013年06月25日 00:51:45, proppy wrote: > > Do you also want to hide other field like, Context, Scopes, and TokenCache? > > > > Should the cache be optional? > > Let's start by only exporting NewClient. > The fields on the Transport struct can stay exported, but Transport should be > "transport" (unexported). Done. PTAL
On 2013年06月25日 17:24:19, proppy wrote: > On 2013年06月25日 04:09:22, adg wrote: > > On 2013年06月25日 00:51:45, proppy wrote: > > > Do you also want to hide other field like, Context, Scopes, and TokenCache? > > > > > > Should the cache be optional? > > > > Let's start by only exporting NewClient. > > The fields on the Transport struct can stay exported, but Transport should be > > "transport" (unexported). > > Done. > > PTAL FYI, I also updated the service account sample to make use of this: https://github.com/GoogleCloudPlatform/appengine-goshorten/pull/2
https://codereview.appspot.com/9998043/diff/44001/appengine/serviceaccount/ca... File appengine/serviceaccount/cache.go (right): https://codereview.appspot.com/9998043/diff/44001/appengine/serviceaccount/ca... appengine/serviceaccount/cache.go:20: type Cache struct { and this? https://codereview.appspot.com/9998043/diff/44001/appengine/serviceaccount/se... File appengine/serviceaccount/serviceaccount.go (right): https://codereview.appspot.com/9998043/diff/44001/appengine/serviceaccount/se... appengine/serviceaccount/serviceaccount.go:63: type Transport struct { I thought we said this would be unexported?
PTAL https://codereview.appspot.com/9998043/diff/44001/appengine/serviceaccount/ca... File appengine/serviceaccount/cache.go (right): https://codereview.appspot.com/9998043/diff/44001/appengine/serviceaccount/ca... appengine/serviceaccount/cache.go:20: type Cache struct { On 2013年06月26日 01:12:05, adg wrote: > and this? Done. https://codereview.appspot.com/9998043/diff/44001/appengine/serviceaccount/se... File appengine/serviceaccount/serviceaccount.go (right): https://codereview.appspot.com/9998043/diff/44001/appengine/serviceaccount/se... appengine/serviceaccount/serviceaccount.go:63: type Transport struct { On 2013年06月26日 01:12:05, adg wrote: > I thought we said this would be unexported? Oh, sorry for the missunderding I though you were refering to Transport field not service.Transport type. (hence my question about the other member). Done.
https://codereview.appspot.com/9998043/diff/51002/appengine/serviceaccount/ca... File appengine/serviceaccount/cache.go (right): https://codereview.appspot.com/9998043/diff/51002/appengine/serviceaccount/ca... appengine/serviceaccount/cache.go:18: // A TokenCache implementation that use memcache to store AccessToken // cache implements TokenCache using memcache... https://codereview.appspot.com/9998043/diff/51002/appengine/serviceaccount/se... File appengine/serviceaccount/serviceaccount.go (right): https://codereview.appspot.com/9998043/diff/51002/appengine/serviceaccount/se... appengine/serviceaccount/serviceaccount.go:15: // // simple usage. delete https://codereview.appspot.com/9998043/diff/51002/appengine/serviceaccount/se... appengine/serviceaccount/serviceaccount.go:37: // NewClient returns a new *http.Client authorized for the s/a new/an/ https://codereview.appspot.com/9998043/diff/51002/appengine/serviceaccount/se... appengine/serviceaccount/serviceaccount.go:38: // given scopes with the service account owned by the application. mention caching
PTAL https://codereview.appspot.com/9998043/diff/51002/appengine/serviceaccount/ca... File appengine/serviceaccount/cache.go (right): https://codereview.appspot.com/9998043/diff/51002/appengine/serviceaccount/ca... appengine/serviceaccount/cache.go:18: // A TokenCache implementation that use memcache to store AccessToken On 2013年06月26日 01:23:51, adg wrote: > // cache implements TokenCache using memcache... Done. https://codereview.appspot.com/9998043/diff/51002/appengine/serviceaccount/se... File appengine/serviceaccount/serviceaccount.go (right): https://codereview.appspot.com/9998043/diff/51002/appengine/serviceaccount/se... appengine/serviceaccount/serviceaccount.go:15: // // simple usage. On 2013年06月26日 01:23:51, adg wrote: > delete Done. https://codereview.appspot.com/9998043/diff/51002/appengine/serviceaccount/se... appengine/serviceaccount/serviceaccount.go:37: // NewClient returns a new *http.Client authorized for the On 2013年06月26日 01:23:51, adg wrote: > s/a new/an/ Done. https://codereview.appspot.com/9998043/diff/51002/appengine/serviceaccount/se... appengine/serviceaccount/serviceaccount.go:38: // given scopes with the service account owned by the application. On 2013年06月26日 01:23:51, adg wrote: > mention caching Done.
Upload? On 26 June 2013 11:33, <proppy@google.com> wrote: > PTAL > > > > https://codereview.appspot.**com/9998043/diff/51002/** > appengine/serviceaccount/**cache.go<https://codereview.appspot.com/9998043/diff/51002/appengine/serviceaccount/cache.go> > File appengine/serviceaccount/**cache.go (right): > > https://codereview.appspot.**com/9998043/diff/51002/** > appengine/serviceaccount/**cache.go#newcode18<https://codereview.appspot.com/9998043/diff/51002/appengine/serviceaccount/cache.go#newcode18> > appengine/serviceaccount/**cache.go:18: // A TokenCache implementation > that use memcache to store AccessToken > On 2013年06月26日 01:23:51, adg wrote: > >> // cache implements TokenCache using memcache... >> > > Done. > > > https://codereview.appspot.**com/9998043/diff/51002/** > appengine/serviceaccount/**serviceaccount.go<https://codereview.appspot.com/9998043/diff/51002/appengine/serviceaccount/serviceaccount.go> > File appengine/serviceaccount/**serviceaccount.go (right): > > https://codereview.appspot.**com/9998043/diff/51002/** > appengine/serviceaccount/**serviceaccount.go#newcode15<https://codereview.appspot.com/9998043/diff/51002/appengine/serviceaccount/serviceaccount.go#newcode15> > appengine/serviceaccount/**serviceaccount.go:15: // // simple usage. > On 2013年06月26日 01:23:51, adg wrote: > >> delete >> > > Done. > > > https://codereview.appspot.**com/9998043/diff/51002/** > appengine/serviceaccount/**serviceaccount.go#newcode37<https://codereview.appspot.com/9998043/diff/51002/appengine/serviceaccount/serviceaccount.go#newcode37> > appengine/serviceaccount/**serviceaccount.go:37: // NewClient returns a > new *http.Client authorized for the > On 2013年06月26日 01:23:51, adg wrote: > >> s/a new/an/ >> > > Done. > > > https://codereview.appspot.**com/9998043/diff/51002/** > appengine/serviceaccount/**serviceaccount.go#newcode38<https://codereview.appspot.com/9998043/diff/51002/appengine/serviceaccount/serviceaccount.go#newcode38> > appengine/serviceaccount/**serviceaccount.go:38: // given scopes with the > service account owned by the application. > On 2013年06月26日 01:23:51, adg wrote: > >> mention caching >> > > Done. > > https://codereview.appspot.**com/9998043/<https://codereview.appspot.com/9998... >
Done, sorry On Tue, Jun 25, 2013 at 6:37 PM, Andrew Gerrand <adg@golang.org> wrote: > Upload? > > > On 26 June 2013 11:33, <proppy@google.com> wrote: > >> PTAL >> >> >> >> https://codereview.appspot.**com/9998043/diff/51002/** >> appengine/serviceaccount/**cache.go<https://codereview.appspot.com/9998043/diff/51002/appengine/serviceaccount/cache.go> >> File appengine/serviceaccount/**cache.go (right): >> >> https://codereview.appspot.**com/9998043/diff/51002/** >> appengine/serviceaccount/**cache.go#newcode18<https://codereview.appspot.com/9998043/diff/51002/appengine/serviceaccount/cache.go#newcode18> >> appengine/serviceaccount/**cache.go:18: // A TokenCache implementation >> that use memcache to store AccessToken >> On 2013年06月26日 01:23:51, adg wrote: >> >>> // cache implements TokenCache using memcache... >>> >> >> Done. >> >> >> https://codereview.appspot.**com/9998043/diff/51002/** >> appengine/serviceaccount/**serviceaccount.go<https://codereview.appspot.com/9998043/diff/51002/appengine/serviceaccount/serviceaccount.go> >> File appengine/serviceaccount/**serviceaccount.go (right): >> >> https://codereview.appspot.**com/9998043/diff/51002/** >> appengine/serviceaccount/**serviceaccount.go#newcode15<https://codereview.appspot.com/9998043/diff/51002/appengine/serviceaccount/serviceaccount.go#newcode15> >> appengine/serviceaccount/**serviceaccount.go:15: // // simple >> usage. >> On 2013年06月26日 01:23:51, adg wrote: >> >>> delete >>> >> >> Done. >> >> >> https://codereview.appspot.**com/9998043/diff/51002/** >> appengine/serviceaccount/**serviceaccount.go#newcode37<https://codereview.appspot.com/9998043/diff/51002/appengine/serviceaccount/serviceaccount.go#newcode37> >> appengine/serviceaccount/**serviceaccount.go:37: // NewClient returns a >> new *http.Client authorized for the >> On 2013年06月26日 01:23:51, adg wrote: >> >>> s/a new/an/ >>> >> >> Done. >> >> >> https://codereview.appspot.**com/9998043/diff/51002/** >> appengine/serviceaccount/**serviceaccount.go#newcode38<https://codereview.appspot.com/9998043/diff/51002/appengine/serviceaccount/serviceaccount.go#newcode38> >> appengine/serviceaccount/**serviceaccount.go:38: // given scopes with the >> service account owned by the application. >> On 2013年06月26日 01:23:51, adg wrote: >> >>> mention caching >>> >> >> Done. >> >> https://codereview.appspot.**com/9998043/<https://codereview.appspot.com/9998... >> > > -- Johan Euphrosine (proppy) Developer Programs Engineer Google Developer Relations
LGTM Thanks!
*** Submitted as https://code.google.com/p/goauth2/source/detail?r=ecc4c1308422 *** appengine: add service account helpers R=golang-dev, adg CC=golang-dev https://codereview.appspot.com/9998043 Committer: Andrew Gerrand <adg@golang.org>