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.

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

Add Storage Qiniu #737

Merged
merged 3 commits into from May 15, 2016

Conversation

Projects
None yet
4 participants
@bastengao
Contributor

bastengao commented Feb 4, 2016

This adds a storage: "Qiniu".

As you kwon S3 is perfect,but unavailable in China, so we only find other solutions instead of it. Qiniu is one of most popular cloud storage services in China.

@bastengao bastengao changed the title from Storage Qiniu to Add Storage Qiniu Feb 4, 2016

@hi54yt

This comment has been minimized.

Show comment
Hide comment
@hi54yt

hi54yt commented Mar 11, 2016

+1

@bastengao

This comment has been minimized.

Show comment
Hide comment
@bastengao

bastengao Apr 19, 2016

Contributor

Hi @tombruijn, I really need this future. I'm happy to help if you have questions.

Contributor

bastengao commented Apr 19, 2016

Hi @tombruijn, I really need this future. I'm happy to help if you have questions.

@tomash

This comment has been minimized.

Show comment
Hide comment
@tomash

tomash Apr 19, 2016

Contributor

Looks decent in my opinion, but the regular ::Qiniu.establish_connection looks a bit unnerving. Might be more readable and maintainable to have this "external" class method aliased/proxied inside Storage::Qiniu class.

Contributor

tomash commented Apr 19, 2016

Looks decent in my opinion, but the regular ::Qiniu.establish_connection looks a bit unnerving. Might be more readable and maintainable to have this "external" class method aliased/proxied inside Storage::Qiniu class.

@bastengao

This comment has been minimized.

Show comment
Hide comment
@bastengao

bastengao Apr 19, 2016

Contributor

@tomash Thanks for your advice. I have already extracted #config_credentials method and pushed it.

Contributor

bastengao commented Apr 19, 2016

@tomash Thanks for your advice. I have already extracted #config_credentials method and pushed it.

@bastengao

This comment has been minimized.

Show comment
Hide comment
@bastengao

bastengao Apr 28, 2016

Contributor

@tombruijn Any advice ?

Contributor

bastengao commented Apr 28, 2016

@tombruijn Any advice ?

@tombruijn

This comment has been minimized.

Show comment
Hide comment
@tombruijn

tombruijn May 8, 2016

Member

Hi @bastengao, thanks for the PR!

The reason I've not answered this in a while is because I wasn't not entirely sure about merging it. I can't really tell what Qiniu does, as I can't read Chinese.

Not entirely sure with the additional secondary dependencies either. I know, it's a bit late to complain about that now 😓 So disregard that

I think that if rebased on the latest master branch we can merge this. So please rebase it so that I can merge it 👍

Member

tombruijn commented May 8, 2016

Hi @bastengao, thanks for the PR!

The reason I've not answered this in a while is because I wasn't not entirely sure about merging it. I can't really tell what Qiniu does, as I can't read Chinese.

Not entirely sure with the additional secondary dependencies either. I know, it's a bit late to complain about that now 😓 So disregard that

I think that if rebased on the latest master branch we can merge this. So please rebase it so that I can merge it 👍

@bastengao

This comment has been minimized.

Show comment
Hide comment
@bastengao

bastengao May 9, 2016

Contributor

@tombruijn Thanks for your response. I will do it later. 😄

Contributor

bastengao commented May 9, 2016

@tombruijn Thanks for your response. I will do it later. 😄

@bastengao

This comment has been minimized.

Show comment
Hide comment
@bastengao

bastengao May 10, 2016

Contributor

@tombruijn Rebased it.

Contributor

bastengao commented May 10, 2016

@tombruijn Rebased it.

@tombruijn tombruijn merged commit ef3492d into backup:master May 15, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tombruijn

This comment has been minimized.

Show comment
Hide comment
@tombruijn

tombruijn May 15, 2016

Member

Great! Merged it 😄

Member

tombruijn commented May 15, 2016

Great! Merged it 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment