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

S3 Improvements #738

Merged
merged 3 commits into from
Mar 1, 2016
Merged

S3 Improvements #738

merged 3 commits into from
Mar 1, 2016

Conversation

flovilmart
Copy link
Contributor

Proposed fix for #731 and #628

  • Adds generic testing for Files Adapters
  • Adds support for content type setting when uploading to S3.
  • Adds ability to enable S3 adapter tests with environment variables S3_ACCESS_KEY, S3_SECRET_KEY

@facebook-github-bot
Copy link

@flovilmart updated the pull request.

@drew-gross
Copy link
Contributor

Looks good! Can we move towards making this an external repo? Then FileAdapter implementers can run the tests without depending on Parse Server

@flovilmart
Copy link
Contributor Author

I wrote the test to make it easier to move to another repo. We should merge that, first, then move all adapters to 3rd party repos.

@gfosco had some thoughts on that too

flovilmart added a commit that referenced this pull request Mar 1, 2016
@flovilmart flovilmart merged commit 0e58c8d into master Mar 1, 2016
@flovilmart flovilmart deleted the flovilmart.s3Improvements branch March 1, 2016 20:08
var location = this.adapter.getFileLocation(config, filename);
return this.adapter.createFile(config, filename, data).then(() => {
return this.adapter.createFile(config, filename, data, contentType).then(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is sort of breaking - can we add this to the docs on FilesAdapter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeppa

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.

5 participants