Google cloud storage #66
Conversation
| } | ||
| } | ||
|
|
||
| func merge(cs ...<-chan error) <-chan error { |
There was a problem hiding this comment.
This can be removed.
TripleDogDare
force-pushed
the
google-cloud-storage
branch
from
0f46f86
to
9cb3a80
Compare
| service := makeGsObjectService(token) | ||
| object := &storage.Object{Name: path} | ||
| // TODO: multipart or resumable upload | ||
| ohshit := make(chan error, 1) |
There was a problem hiding this comment.
A more common name for this might be errCh 🌴 🌴 🌴
|
This is awesome! I'm testing it now and everything looks good. Google Cloud Storage support is going to be a really nice integration to have. (I'm also really appreciating the GCS web UI quite a bit compared to how AWS has been treating me. I might switch the backend behind the repeatr bootstrap assets to GCS after this lands!) I will have to ask you to do one quick edit though -- the "PR" CI check is red because these commits do not have the DCO sign-off required by the contribution guide. As soon as that's taken care of, I'll be thrilled to merge this 💃 |
| func getAccessTokenFromEnv() *oauth2.Token { | ||
| accessToken := os.Getenv(EnvGsAccessToken) | ||
| if accessToken != "" { | ||
| return &oauth2.Token{AccessToken: accessToken} |
There was a problem hiding this comment.
Hm. I wish there was a way to sanity-check the values here to make sure it smells like an oauth token -- It'd be nice to give a "token invalid" error right away without contacting a network service if someone does export GS_ACCESS_TOKEN="asdflol" -- but I don't see any way to do that in the library either (the Valid() predicate is... uninspiring), so okay, I guess we'll do without. 🙉
There was a problem hiding this comment.
This would be nice, but RFC6749 Section 1.4: Access tokens says that access tokens can be opaque to the client and the format is based on the server's requirements. We could implement something based on assumptions about Google's OAuth2 implementation, but ultimately it boils down to "try and see" after eliminating zero values. Anything else will probably be fragile and unsatisfying.
There was a problem hiding this comment.
Ach. Consider me well and thoroughly hit with the book.
Signed-off-by: Calvin Behling <calvin.behling@gmail.com>
Signed-off-by: Calvin Behling <calvin.behling@gmail.com>
TripleDogDare
force-pushed
the
google-cloud-storage
branch
2 times, most recently
from
9882b99
to
b6aaa90
Compare
Signed-off-by: Calvin Behling <calvin.behling@gmail.com>
TripleDogDare
force-pushed
the
google-cloud-storage
branch
from
b6aaa90
to
dc9f9f8
Compare
| a Google Developers service account JSON key file | ||
| which is then used for two-legged OAuth to create an Access Token | ||
| */ | ||
| const EnvGsAccountFile = "GS_SERVICE_ACCOUNT_FILE" |
There was a problem hiding this comment.
We should probably add "GOOGLE_APPLICATION_CREDENTIALS" as an alias of this as well: https://github.com/golang/oauth2/blob/2baa8a1b9338cf13d9eeb27696d761155fa480be/google/default.go#L62 I like your names here better, TBH, and GS_ACCESS_TOKEN in particular is scoped to make sense, but if someone's expecting these defaults to work, might as well.
On the other hand, that DefaultTokenSource function also hits metadata.OnGCE and that in turn... That's... rather indiscreet and not the sort of default behavior I want in this project -- phoning home without configuration, potentially adding hundreds of ms of latency in a timeout that can't be configured or disabled, just for feature detection that's something some users will be able to guarantee they'll never use; all No Thanks. So, maybe we should copy some of that functionality, and leave the rest out, or bring in a patched version that wraps that in conditionals.
There was a problem hiding this comment.
woof 🐕
Which environment variables to use and in what contexts can become quite complex. If people download private keys instead of the JSON file then we could load the account name and private keys separately. We could load refresh tokens or we could implement some portion of 3-legged oauth that helps people create tokens using other methods. All of this is extra complexity that we don't need at the moment. I'd like to see some use cases before going full bore into features. And there's a few TODOs in there that could be a lot more interesting/useful to knock down such as composite objects.
I agree on avoiding poking extra externals. We aren't trying to support GCE and it should not be wrapped into default behavior.
|
Sans my newly-discovered nit about env var names, this LGTM, CI's green, and I wanna start using this so... There's always time to slather on more config options as needed; Gonna go ahead and merge! |
This should enable the use of Google cloud storage.