Add the ability to use minio with s3 getter #68
Conversation
get_s3.go
Outdated
| bucket = pathParts[1] | ||
| path = pathParts[2] | ||
| version = u.Query().Get("version") | ||
| region = "us-east-1" |
There was a problem hiding this comment.
This should probably be parsable as well in case the user has set a non-default region on minio
There was a problem hiding this comment.
I completely overlooked the ability to set the region. I will query the region as well
get_s3.go
Outdated
| err = fmt.Errorf("URL is not a valid S3 URL") | ||
| return | ||
| } | ||
| if strings.Contains(u.Host, "amazonaws.com") { |
There was a problem hiding this comment.
Can you add a comment explaining the different parsing for supporting blob stores mimicking the AWS S3 API
| @@ -204,29 +212,45 @@ func (g *S3Getter) getAWSConfig(region string, creds *credentials.Credentials) * | |||
| } | |||
|
|
|||
| func (g *S3Getter) parseUrl(u *url.URL) (region, bucket, path, version string, creds *credentials.Credentials, err error) { | |||
There was a problem hiding this comment.
Lets add some unit tests to test parsing both s3 and s3 complaint URLs. Lets us get some test coverage and avoid running the server in tests.
After that we can merge!
|
@dadgar let me know if those unit tests are ok! Couldn't think of another way to test it |
|
Please dont merge yet. I want to clean up the tests a little and i forgot about a failure case. Sorry about this |
|
Question @armon are we looking to get this merge and added for 0.6.0 Nomad? |
|
@dadgar fixed my code and the tests. Ready for another review 👍 Sorry for the delay! |
get_s3_test.go
Outdated
| if creds != nil { | ||
| t.Fatal("expected to not be nil") | ||
| func TestS3Getter_Url(t *testing.T) { | ||
| for i, pt := range s3tests { |
There was a problem hiding this comment.
I would just inline here:
var s3tests = []struct{
url string
...
} {
{
// test 1
},
...
}
for i, pt := range s3tests {
}
And then you can use the new table driven test using t.Run(..)
Better example here: https://blog.golang.org/subtests
|
@lfarnell Looking good! Small comment on the tests just to clean it up a bit but otherwise 🥇 |
|
@dadgar done! |
This is an attempt at adding S3 compliant server support for the S3 protocol. I only tested with minio and it seemed to work
I wasn't exactly sure how to add a unit test for it considering i would need to have a minio server running. Any feedback on the code would be great!
Closes #52