-
Notifications
You must be signed in to change notification settings - Fork 808
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
blob: use new URLMux pattern for OpenBucket #1282
Conversation
Design discussion in google#1209. Updates google#1174
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.
Looks pretty good! A few comments, mostly the same 2 over and over.
|
||
if region := q["region"]; len(region) > 0 { |
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.
Can you put these back into the default implementation?
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.
Can you help me understand why they're here? They seem to only be for constructing the session and aren't specific to the S3 bucket. I removed them because it seemed like based on our new design principles that someone who wants to override these would create a new s3blob.URLOpener
.
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.
I failed to follow up on this comment. I think it's important to include these, will send a PR.
Can you sync to HEAD as well? Looks like there are some conflicts, hopefully not too hairy. |
Address some documentation concerns in the process
PTAL |
Design discussion in #1209.
A follow-up PR will shorten the tutorial to use
blob.OpenBucket
.Updates #1174