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.

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

Aws sdk #935

Merged
merged 4 commits into from
Apr 13, 2015
Merged

Aws sdk #935

merged 4 commits into from
Apr 13, 2015

Conversation

arthurnn
Copy link
Member

  • Replace s3 gem with aws-sdk-core (amazon's gem)
  • add a restore method, so we can delete files and restore them. (I tested that in staging, and it worked)

review @dwradcliffe @sferik @qrush

@simi
Copy link
Member

simi commented Apr 12, 2015

So restore method is meant to be used manually only?

@@ -99,6 +103,8 @@ GEM
honeybadger (1.16.7)
json
i18n (0.7.0)
jmespath (1.0.2)
multi_json (~> 1.0)
Copy link
Member

Choose a reason for hiding this comment

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

👍 overall. This dependency ties us tighter to multi_json. I’m okay with it but it will make your project to completely eliminate the multi_json dependency more difficult.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right. I guess multi_json will stick around for a while still.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

thats awesome @sferik

@arthurnn
Copy link
Member Author

So restore method is meant to be used manually only?

That method , is mainly for #923 , so we can delete gems and restore them (manually) if need it.

s3.get_object(key: key, bucket: bucket);
rescue Aws::S3::Errors::NoSuchKey=> e
version_id = e.context.http_response.headers["x-amz-version-id"]
s3.delete_object(key: key, bucket: bucket, version_id: version_id)
Copy link
Member

Choose a reason for hiding this comment

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

So we're expecting the get to fail every time and raise the exception?

@simi
Copy link
Member

simi commented Apr 12, 2015

@arthurnn sure. But I was thinking about whole workflow.

Once deletion is created, those two hooks are triggered:

# app/models/deletion.rb
...
  after_create :remove_from_index #removed from index
  after_commit :remove_from_storage #removed from storage
...

But according to note from #923

Since we're using a versioned S3 bucket, we are in theory prepared for a "why_ scenario" where someone removes all of their gems that others depend on, since we can just restore them out of the bucket if something/someone went truly wrong.

it will be good to be prepared to rollback deletion also.

With restore method you'll only add it back to the storage, but not to the index. Is that alright?

So the flow will be like:

  1. open server rails console
  2. run RubygemFs.instance.restore(key)
  3. still keep it "unindexed"

@simi
Copy link
Member

simi commented Apr 12, 2015

Only restore (meant to be callable manually),store, remove, get and base_dir methods are used in #923 and master for RubygemFs.instances.

What about to add blank restore method (or raise NotImplementedError) to RubygemFs::Local and keep another methods (like bucket or s3 in RubygemFs::S3) private ?

@arthurnn
Copy link
Member Author

With restore method you'll only add it back to the storage, but not to the index. Is that alright?

Yes

So the flow will be like:

No, After this gets merged I will add a restore method to #923 Deletion model so we can restore the s3 file and the index flag on the db.

Regarding the API on RubygemFs, yes, I should make some common methods public and the others private. I will do that.

@simi
Copy link
Member

simi commented Apr 13, 2015

Thanks @arthurnn, that makes sense now.

@arthurnn
Copy link
Member Author

@dwradcliffe

So we're expecting the get to fail every time and raise the exception?

Yes, thats how the API works. it returns a 404 with a versionId, and on the sdk gem a 404 means an exception.

@dwradcliffe
Copy link
Member

@arthurnn ok cool, just making sure I understood.

@dwradcliffe
Copy link
Member

👍 🚢

arthurnn pushed a commit that referenced this pull request Apr 13, 2015
@arthurnn arthurnn merged commit 4be294e into master Apr 13, 2015
@arthurnn arthurnn deleted the aws-sdk branch April 13, 2015 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants