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

Refactored storage to factorize some code #16

Merged
merged 3 commits into from
Aug 19, 2015
Merged

Refactored storage to factorize some code #16

merged 3 commits into from
Aug 19, 2015

Conversation

Bladrak
Copy link

@Bladrak Bladrak commented Aug 12, 2015

See #11

@Bladrak Bladrak mentioned this pull request Aug 12, 2015
Closed
@masom
Copy link

masom commented Aug 12, 2015

@Bladrak Tests do cover this right?

@Bladrak
Copy link
Author

Bladrak commented Aug 13, 2015

@masom tests cover this partially; one of the storage is covered, but not the other one. However I'm not sur the current behavior is the expected one, I was hoping to discuss that but maybe this could go in another PR.

@Bladrak
Copy link
Author

Bladrak commented Aug 13, 2015

The uncovered storage is the result storage at the moment.

@masom
Copy link

masom commented Aug 13, 2015

@Bladrak Would you mind expanding on "However I'm not sure the current behavior is the expected one" ?

@Bladrak
Copy link
Author

Bladrak commented Aug 13, 2015

That's actually what's described in issue #11 ; the RESULT_STORAGE_AWS_STORAGE_ROOT_PATH is at the moment only used in the storage.s3_storage class, and not used in the result_storage.s3_storage class, which is odd.
With this PR, the variable will be used for both storages; but I have no idea whether that's what's expected, or not.

@masom
Copy link

masom commented Aug 13, 2015

Not sure on this one. @dhardy92 ?

@dhardy92
Copy link

I suppose it would be 2 settings :
RESULT_STORAGE_AWS_STORAGE_ROOT_PATH
STORAGE_AWS_STORAGE_ROOT_PATH
that could be different maybe (we could use the same bucket for both as bucket number is limited by AWS to 100 per user)

What do you think ?

For now, setting RESULT_STORAGE_AWS_STORAGE_ROOT_PATH breaks my storage.
(didn't try this patch yet)

@Bladrak
Copy link
Author

Bladrak commented Aug 13, 2015

Seems better to me @dhardy92 :)
However, maybe this should be part of another PR? As this one concerns refactoring only. Adding an option would add another purpose to this PR, which wouldn't then be as atomic.

@dhardy92
Copy link

👍 for another PR

@Bladrak Bladrak mentioned this pull request Aug 14, 2015
Merged
@masom
Copy link

masom commented Aug 14, 2015

@Bladrak good to merge?

@Bladrak
Copy link
Author

Bladrak commented Aug 14, 2015

@masom I'm currently working on PR #17 to add some tests on the other storage; maybe we should wait on this one to ensure the refactoring didn't break anything?

@masom
Copy link

masom commented Aug 14, 2015

Sure.

@Bladrak
Merge branch 'master' into refactor_storage
5c71f0f 
Conflicts:
	tc_aws/__init__.py
	tc_aws/result_storages/s3_storage.py
	tc_aws/storages/s3_storage.py
@Bladrak
Apply refactoring to recent updates
e5b4ab9
@Bladrak
Copy link
Author

Bladrak commented Aug 19, 2015

@masom should be done now :) All tests are passing

masom pushed a commit that referenced this pull request Aug 19, 2015
Merge pull request #16 from Bladrak/refactor_storage
ac759dd 
Refactored storage to factorize some code
@masom masom merged commit ac759dd into thumbor-community:master Aug 19, 2015
@masom masom removed the in progress label Aug 19, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.

None yet
3 participants
@Bladrak @masom @dhardy92