search

mattheworiordan / capybara-screenshot

Automatically save screen shots when a Capybara scenario fails
MIT License
1.02k stars 168 forks source link

Allow to set bucket location to avoid get_bucket_location request #250

Closed oliverguenther closed 5 years ago

oliverguenther commented 5 years ago

As noted in https://github.com/mattheworiordan/capybara-screenshot/pull/239#issuecomment-449782254, it is currently required to fetch the bucket location to form the S3 URL which now requires the bucket name to be present.

By optionally accepting a bucket_location parameter in the s3 configuration, we can save this request and also avoid having to provide the s3:GetBucketLocation permission on the bucket.

The README is extended to reflect this change.

We have stumbled upon this discussion because after upgrading to the latest version, our screenshot uploading was broken due to the missing s3:GetBucketLocation permission.

oliverguenther commented 5 years ago

This is a really nice addition. Thanks for the contribution. If you could remove the changelog stuff, I will merge and get this out into the next release/

Sure, I just removed it. :+1:

machty commented 5 years ago

Ping for an update on this? Seems very useful.

machty commented 5 years ago

Actually, I think there's another approach this PR should take, which is to provide the S3 hostname to use for accessing the file.

Reason being: not all buckets support the #{bucket_name}.s3-#{s3_region}.amazonaws.com; some (especially non-DNS compliant bucket names) only support #{bucket_name}.s3.amazonaws.com.

So, I can't actually use the new option introduced in this PR, but if there were something like s3_host, e.g. s3_host: "my-bucket.s3.amazonaws.com" or s3_host: "my-bucket.s3-us-east-1.amazonaws.com", I believe this would cover all conceivable use cases.

machty commented 5 years ago

I introduced #252 to address the issues I commented on.

mattheworiordan commented 5 years ago

@machty closing as #252 is now merged and will be released soon. Shout if this PR needs to remain open.