Skip to content
This repository was archived by the owner on Jul 13, 2023. It is now read-only.

Aws sdk v2#1730

Closed
danielwanja wants to merge 27 commits intothoughtbot:masterfrom
danielwanja:aws-sdk-v2
Closed

Aws sdk v2#1730
danielwanja wants to merge 27 commits intothoughtbot:masterfrom
danielwanja:aws-sdk-v2

Conversation

@danielwanja
Copy link
Copy Markdown

@danielwanja danielwanja commented Dec 17, 2014

  • Added s3 storage implementation using the aws-sdk v2.

houndci-bot
houndci-bot reviewed Dec 17, 2014
View reviewed changes
Comment thread lib/paperclip/storage/s3.rb Outdated
Copy link
Copy Markdown

@houndci-bot houndci-bot Dec 17, 2014

Choose a reason for hiding this comment

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

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@danielwanja
* Fixed delete image bug.
c44e97d 
* Fixed hound notification - removing unused exception variable.
houndci-bot
houndci-bot reviewed Dec 17, 2014
View reviewed changes
Comment thread lib/paperclip/storage/s3v2.rb Outdated
Copy link
Copy Markdown

@houndci-bot houndci-bot Dec 17, 2014

Choose a reason for hiding this comment

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

Line is too long. [88/80]

@danielwanja
Applied many houndci recommendations:
c774546 
* Using Ruby 1.9 hash syntax
* Using double-quoted strings
* Space after missing comma
* Wrapped lines over 80 characters, unless code was less readable
houndci-bot
houndci-bot reviewed Mar 10, 2015
View reviewed changes
Comment thread lib/paperclip/storage/s3v2.rb
Copy link
Copy Markdown

@houndci-bot houndci-bot Mar 10, 2015

Choose a reason for hiding this comment

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

Assignment Branch Condition size for flush_writes is too high. [31.89/15]

@danielwanja
Copy link
Copy Markdown
Author

danielwanja commented Mar 23, 2015

Do you guys really want me to fix all the houdci notifications. Even the line too long ones? Make code less readable in my view? What else would you like to see fixed for this to be accepted?

@booleanbetrayal
Copy link
Copy Markdown

booleanbetrayal commented Mar 24, 2015

some of those houndci warnings are super aggressive. would be nice to see this changeset get in!

@jyurek jyurek removed the unknown label Apr 24, 2015
@reshleman reshleman mentioned this pull request May 12, 2015
Merged
@maclover7
Copy link
Copy Markdown
Contributor

maclover7 commented May 13, 2015

@tute @jyurek This should probably come in 5.0, right?

@tute
Copy link
Copy Markdown
Contributor

tute commented May 13, 2015

@danielwanja thank you for your work!
@jyurek what do you think about this changeset?

@danielwanja
Copy link
Copy Markdown
Author

danielwanja commented May 13, 2015

@tute, my pleasure. Let me know what you thing of this changeset and what would be needed to have this merged.

@jyurek jyurek mentioned this pull request Jun 1, 2015
Closed
@natesire
Copy link
Copy Markdown

natesire commented Jun 1, 2015

I would like to help with this. I already mocked up a solution for the AWS S3 gem at marcel/aws-s3#108

@jyurek
Copy link
Copy Markdown

jyurek commented Jun 1, 2015

@nathantechie9 There is no change that needs to happen with the aws-s3 gem. Paperclip has been using the aws-sdk gem for some time now, which is a different gem, but aside from that, the library should not need to change to accommodate us.

@jyurek
Copy link
Copy Markdown

jyurek commented Jun 1, 2015

@danielwanja I would really appreciate it if you could look at the places Hound says are too complicated and refactor them a bit. Thanks! :)

@natesire
Copy link
Copy Markdown

natesire commented Jun 1, 2015

What's the issue with the regions? I'd like to help.

@tute
Copy link
Copy Markdown
Contributor

tute commented Jun 18, 2015

Is the functionality in this PR comparable to that of #1903?

@danielwanja
Copy link
Copy Markdown
Author

danielwanja commented Jun 18, 2015

Same idea, to use the new aws-sdk v2, different approach. PR #1903
#1903 intermingles V1 and V2
code in the same adapter using checking aws_v1? and calling the
appropriate underlying aws-sdk method. I preferred creating a new adapter
that doesn't mingle both. Being originally from Switzerland I can see
advantages in both approaches. The main goal for me was to be able to use
some of the new features the aws-sdk v2 prodives, notably using the new
kms related headers ("x-amz-server-side-encryption" => "aws:kms",
"x-amz-ssekms-key-id"
=> "alias/mycustom_encryption_key").

On Wed, Jun 17, 2015 at 10:45 PM, Tute Costa notifications@github.com
wrote:

Is the functionality in this PR comparable to that of #1903
#1903?

—
Reply to this email directly or view it on GitHub
#1730 (comment)
.

@tute tute mentioned this pull request Jun 24, 2015
Closed
@betesh betesh mentioned this pull request Jun 24, 2015
Closed
@Momus Momus mentioned this pull request Jul 27, 2015
Closed
@tute
Copy link
Copy Markdown
Contributor

tute commented Aug 24, 2015

AWS 2 support has been merged: #1903 (comment). Thanks for your work!

@tute tute closed this Aug 24, 2015
@hchood hchood mentioned this pull request Jun 2, 2016
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@danielwanja @booleanbetrayal @maclover7 @tute @natesire @jyurek @houndci-bot @sikachu