Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support Aliyun provider to authenticated_url aliyun #2381

Merged
merged 2 commits into from Apr 30, 2019
Merged

Add support Aliyun provider to authenticated_url aliyun #2381

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4f62b55
Add Aliyun to support authenticated_url
Nitrino Mar 11, 2019
44dba53
Calculate expire_at from now
Nitrino Mar 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 5 additions & 2 deletions lib/carrierwave/storage/fog.rb
View file
Open in desktop
Expand Up @@ -177,7 +177,7 @@ def attributes

##
# Return a temporary authenticated url to a private file, if available
# Only supported for AWS, Rackspace, Google and AzureRM providers
# Only supported for AWS, Rackspace, Google, AzureRM and Aliyun providers
#
# === Returns
#
Expand All @@ -186,7 +186,7 @@ def attributes
# [NilClass] no authenticated url available
#
def authenticated_url(options = {})
if ['AWS', 'Google', 'Rackspace', 'OpenStack', 'AzureRM'].include?(@uploader.fog_credentials[:provider])
if ['AWS', 'Google', 'Rackspace', 'OpenStack', 'AzureRM', 'Aliyun'].include?(@uploader.fog_credentials[:provider])
# avoid a get by using local references
local_directory = connection.directories.new(:key => @uploader.fog_directory)
local_file = local_directory.files.new(:key => path)
Expand All @@ -204,6 +204,9 @@ def authenticated_url(options = {})
connection.get_object_https_url(@uploader.fog_directory, path, expire_at, options)
when 'OpenStack'
connection.get_object_https_url(@uploader.fog_directory, path, expire_at)
when 'Aliyun'
expire_at = expire_at - Time.now
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should calculate expire_at from now because inside aliyun-fog added Time.now
https://github.com/fog/fog-aliyun/blob/6f4d1716488bbcb09ba6538eb3d1cc07d2f26f4b/lib/fog/aliyun/requests/storage/get_object_http_url.rb#L25

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a test for this feature?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this logic require testing?
expire_at is defined in this function and passed to another library.
I see here only the opportunity to test that the url method with the correct parameter was called inside the authenticated_url method. But this test is not of great use.

local_file.url(expire_at)
else
local_file.url(expire_at)
end
Expand Down
2 changes: 1 addition & 1 deletion spec/storage/fog_helper.rb
View file
Open in desktop
Expand Up @@ -438,7 +438,7 @@ class FogSpec#{fog_credentials[:provider]}Uploader < CarrierWave::Uploader::Base
end

it "should have an authenticated_url" do
if ['AWS', 'Rackspace', 'Google', 'OpenStack', 'AzureRM'].include?(@provider)
if ['AWS', 'Rackspace', 'Google', 'OpenStack', 'AzureRM', 'Aliyun'].include?(@provider)
expect(@fog_file.authenticated_url).not_to be_nil
end
end
Expand Down