search

keystonejs / keystone-storage-adapter-s3

⚠️ Archived - Legacy S3 Storage Adapter for KeystoneJS
MIT License
17 stars 56 forks source link

Region handling clarity #10

Closed thekevinbrown closed 7 years ago

thekevinbrown commented 7 years ago

The docs don't currently specify that region is a potential option for the configuration of the bucket. Knox was correctly handing this option, but I think we should make it clear that it's possible to pass this value in, and we should make the default specific and within our control.

This PR does both these things, and I don't believe it'll be a breaking change for people unless they have process.env.S3_REGION set for some other reason, but they were depending on the default us-east-1 from Knox to work.

thekevinbrown commented 7 years ago

@JedWatson, sorry to be a pain, but could you let me know when someone'll have a chance to review this? Thanks!

JedWatson commented 7 years ago

:+1:

JedWatson commented 7 years ago

Merged, but just thinking further - wouldn't it make more sense to throw an error if the region isn't defined? and the other required options; we're not actually validating them. Thoughts @blargity?

thekevinbrown commented 7 years ago

In general I agree that validating these would be a good thing, however with region specifically all of the AWS tolling will default to us-east-1 if you don't specify a region. That's the cli tools, APIs, everything. So it feels like we should have same behaviour for this (actually Knox does this by default too).

The other params though, absolutely should be screaming if they're not supplied. I'll PR that when I get a chance.