-
Notifications
You must be signed in to change notification settings - Fork 647
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
api: ListBuckets() should have a default region for AWS S3. #720
Conversation
6d95565
to
fe6ec26
Compare
e372a73
to
462bcab
Compare
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.
LGTM
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.
Comment fix. otherwise lgtm.
api.go
Outdated
location = metadata.bucketLocation | ||
} | ||
if location == "" { | ||
// Default to location to 'us-east-1'. |
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.
s/Default to location to/Default location to/
api.go
Outdated
location = "us-gov-west-1" | ||
} else if c.region != "" { | ||
location = c.region | ||
} | ||
} |
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.
Perhaps a switch statement would be more natural here.
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 tried that.. but it got complex. So an if-condition was easier. In-fact will cleanup to put this under functions for each cases.
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.
this would look more readable?:
if location == "" {
switch {
case s3utils.IsAmazonChinaEndpoint(c.endpointURL):
location = "cn-north-1"
case s3utils.IsAmazonGovCloudEndpoint(c.endpointURL):
location = "us-gov-west-1"
case c.region != "":
location = c.region
default:
location = "us-east-1"
}
}
db684ac
to
dd0efb0
Compare
can you look again @krishnasrinivas @donatello ? |
api.go
Outdated
location := metadata.bucketLocation | ||
if location == "" { | ||
switch { | ||
case metadata.bucketName != "": |
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.
as this is a switch with single case you could rather use
if metadata.bucketName != ""
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.
oh yes done.
dd0efb0
to
452bcc9
Compare
utils.go
Outdated
// | ||
// If no other cases match then the location is set to `us-east-1` | ||
// as a last resort. | ||
func getDefaultLocation(u url.URL, regionOverride string) (location string) { |
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.
If the user tries to override AmazonChinaEndpoint region then better not auto correct but let it fail naturally as the server will return error. This will make this function simpler as well.
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 don't get it.. @krishnasrinivas
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.
This is how it is in getBucketLocation() let me do that as well.
utils.go
Outdated
default: | ||
location = "us-east-1" | ||
} | ||
return |
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.
actually this is more readable as it is not a if/else situation (and hence switch need not be used):
func getDefaultLocation(u url.URL, regionOverride string) (location string) {
if s3utils.IsAmazonChinaEndpoint(u) {
return "cn-north-1"
}
if s3utils.IsAmazonGovCloudEndpoint(u) {
return "us-gov-west-1"
}
if regionOverride != "" {
return regionOverride
}
return "us-east-1"
}
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.
Well this is how it was before @krishnasrinivas
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.
Doing it without switch.
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.
previously it was if/else @harshavardhana :
if s3utils.IsAmazonChinaEndpoint(u) {
location = "cn-north-1"
} else if s3utils.IsAmazonGovCloudEndpoint(u) {
location = "us-gov-west-1"
} else if regionOverride != "" {
location = regionOverride
} else {
location = "us-east-1"
}
452bcc9
to
35c5b4d
Compare
35c5b4d
to
11561cb
Compare
Fixes #717