Add a Cloudinary service for active_storage #341
Conversation
Service tests are limited to Cloudinary use cases (i.e. storing images and not strings)
| { | ||
| "Content-Type" => content_type, | ||
| "Content-MD5" => checksum, | ||
| # "Access-Control-Allow-Origin" => "localhost" |
| end | ||
| else | ||
| instrument :download, key: key do | ||
| puts "download URL #{url}" |
There was a problem hiding this comment.
I wouldn't use puts as a library owner. Hard to opt-out as a developer.
There was a problem hiding this comment.
Debugging leftovers...
| url = Cloudinary::Utils.unsigned_download_url(public_id(key), resource_type: resource_type(nil, key)) | ||
| uri = URI(url) | ||
| instrument :download, key: key do | ||
| puts "download URL #{url}" |
There was a problem hiding this comment.
I wouldn't use puts as a library owner. Hard to opt-out as a developer.
|
|
||
| def api_uri(action, options) | ||
| base_url = Cloudinary::Utils.cloudinary_api_url(action, options) | ||
| cloudinary_params = Cloudinary::Uploader.build_upload_params(options) |
There was a problem hiding this comment.
maybe rename cloudinary_params to upload_params
| instrument :download, key: key do | ||
| puts "download URL #{url}" | ||
| req = Net::HTTP::Get.new(uri) | ||
| req['range'] = "bytes=#{range.begin}-#{range.exclude_end? ? range.end - 1 : range.end}" |
There was a problem hiding this comment.
would be nice and future proof to consider endless ranges (ruby 2.6)
https://rubyreferences.github.io/rubychanges/2.6.html
| def upload(key, io, filename: nil, checksum: nil, **options) | ||
| instrument :upload, key: key, checksum: checksum do | ||
| begin | ||
| extra_headers = checksum.nil? ? {} : {'Content-md5': checksum} |
There was a problem hiding this comment.
Content-md5 has different casing then in headers_for_direct_upload . Consider a constant maybe ?
| begin | ||
| extra_headers = checksum.nil? ? {} : {'Content-md5': checksum} | ||
| options = @options.merge(options) | ||
| @image_meta = Cloudinary::Uploader.upload( |
|
What about large files? is that supported at our sdk level ? |
| AS_TAG = "active_storage_" + SUFFIX | ||
|
|
||
| SERVICE = ActiveStorage::Service.configure(:cloudinary, SERVICE_CONFIGURATIONS) | ||
| CONFIGURATION_PATH = Pathname.new(File.expand_path("service/configurations.yml", __dir__)) |
There was a problem hiding this comment.
These constants will be available globally (in testing) because they're defined here. Maybe they should be namespaced inside the describe block.
| end | ||
|
|
||
| it "should support uploading to Cloudinary" do | ||
| begin |
| **options | ||
| ) | ||
| rescue CloudinaryException | ||
| raise ActiveStorage::IntegrityError |
There was a problem hiding this comment.
Might make sense to add a message or wrap the CloudinaryException or at least pass on the backtrace?
It was decided to accept a PUT request with the file in the body at the same endpoint as usual POST uploads.
Links, appearance, etc.
amcaplan
force-pushed
the
feature/support-active-storage
branch
from
0810d04
to
b947a96
Compare
Order of dependencies loading apparently makes a big difference, and sprockets isn't helping here. We know this used to work, and the relevant code wasn't changed, so hopefully this just works now.
Service tests are limited to Cloudinary use cases (i.e. storing images and not strings)