search

onyx-platform / onyx-amazon-s3

Amazon S3 plugin for Onyx
1 stars 9 forks source link

Add support for S3 endpoint-url (for S3-compatible storage solutions) #10

Closed kjothen closed 7 years ago

kjothen commented 7 years ago
MichaelDrogalis commented 7 years ago

Thanks! We'll get a look at this today. cc @lbradstreet

kjothen commented 7 years ago

@MichaelDrogalis @lbradstreet Thanks! I was initially thrown by the fact that the end-to-end test fails. I don't have access to the existing "s3-plugin-test-onyx" bucket, and the test attempts to create it each time before using it. Under my AWS account this fails at the .createBucket call with the exception "requested bucket name is not available". Under your account, it silently passes over the fact you've created the bucket already, and the tests work.

TL;DR - please consider creating a random bucket in the end-to-end test, mirroring what the output-test does (I should have committed this change!)

lbradstreet commented 7 years ago

Thanks @kjothen! Looks like a good PR!

Yeah, that's a bit of a tough one with the bucket! I tried to avoid creating a random bucket as these things have a way of not being cleaned up when tests fail, especially on CI. I'll think about it, otherwise maybe I'll make the warning better and the bucket more configurable.

lbradstreet commented 7 years ago

@kjothen could you comment further on the recommendation for creating S3 client objects? We tried to avoid forcing users to provide their S3 credentials via the task-map, as many users will not want to store their S3 credentials in ZooKeeper (via Onyx's job submission). Therefore, we want to at least support the default provider chain which will attempt to use the standard environment variables and standard AWS config paths.

lbradstreet commented 7 years ago

Apologies, I looked at the S3 builder docs and I see it defaults to that provider chain. Good stuff!

kjothen commented 7 years ago

@lbradstreet thanks for persevering with this, but to reassure you, I've tested the plugin both with supplying credentials and without (using env vars or AWS configuration only).

lbradstreet commented 7 years ago

Great! Thanks.

On 16 May 2017 at 12:42, Kieran Othen notifications@github.com wrote:

@lbradstreet https://github.com/lbradstreet thanks for persevering with this, but to reassure you, I've tested the plugin both with supplying credentials and without (using env cars or AWS configuration only)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/onyx-platform/onyx-amazon-s3/pull/10#issuecomment-301893481, or mute the thread https://github.com/notifications/unsubscribe-auth/AAPZHXwWRctZHi9D7Wc3-MvcEbiUHoU0ks5r6fwcgaJpZM4NcUCE .

lbradstreet commented 7 years ago

Done. Thanks for the great PR!

lbradstreet commented 7 years ago

Released in 0.10.0.0-beta17.