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

FilesAdapter for Azure Blob Storage #545

Closed
wants to merge 4 commits into from

Conversation

aneeshd16
Copy link
Contributor

This is a FilesAdapter for uploading, accessing and deleting files on Azure Blob Storage.
It is very similar to how the S3 adapter works.

Use the AzureBlobStorageAdapter as the filesAdapter when initializing parse-server.

options.filesAdapter = new AzureBlobStorageAdapter(
        '<connection-string>',
        '<container-name>',
        {}  
);

or with more options:

options.filesAdapter = new AzureBlobStorageAdapter(
        '<storage-account-name>',
        '<container-name>',
        {
            storageAccessKey: <storage-access-key>,
            host: <host>,
            directAccess: <true|false>
        }   
);

IMO, this addition is needed since parse-server already supports direct deployment to Azure.

This is my first PR at a major repo, I am open to feedback and suggestions! Thanks!

// If you had provided storage account name, then also provide storage access key
// Host is optional, Azure will default to the default host
// directAccess defaults to false. If set to true, the file URL will be the actual blob URL
constructor(
Copy link
Contributor

Choose a reason for hiding this comment

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

would be more consistent with other adapters to only have one parameter options

@flovilmart
Copy link
Contributor

@aneeshd16 that looks great, unit tests would be nice too, even if they fail as you don't want to provide your keys there :)

we were also talking with @drew-gross about the future of those adapters and we both are in favour to remove them from the core repository, and reference them through dependencies.

Moving to a dependency would also help for the testing as you'll be able to put encrypted keys in travis for the unit tests to run and check for failures.

I'm working on a PR (#549) that will enable a simpler and more flexible adapter architecture.

@aneeshd16
Copy link
Contributor Author

Thanks for the review! Yes, I'll add unit tests soon. Looking forward to the new design.

@facebook-github-bot
Copy link

@aneeshd16 updated the pull request.

@flovilmart
Copy link
Contributor

please rebase and add the according tests into https://github.com/ParsePlatform/parse-server/blob/master/spec/FilesController.spec.js

@aneeshd16
Copy link
Contributor Author

Okay will do!

@flovilmart
Copy link
Contributor

Thx!

@felixrieseberg
Copy link
Contributor

Oh man, we just saw the pull request - @aneeshd16, this is awesome! We at Microsoft made felixrieseberg/parse-server-azure-storage, but it'd certainly be great to have some tests (and for this to be more integrated with the Parse-Server development).

@aneeshd16
Copy link
Contributor Author

This is great! Tests are on the way. < 24hrs

@facebook-github-bot
Copy link

@aneeshd16 updated the pull request.

@codecov-io
Copy link

Current coverage is 90.09%

Merging #545 into master will decrease coverage by -1.55% as of 63e534d

@@            master    #545   diff @@
======================================
  Files           73      74     +1
  Stmts         4438    4482    +44
  Branches       889     901    +12
  Methods          0       0       
======================================
- Hit           4067    4038    -29
  Partial         10      10       
- Missed         361     434    +73

Review entire Coverage Diff as of 63e534d

Powered by Codecov. Updated on successful CI builds.

@aneeshd16
Copy link
Contributor Author

@facebook-github-bot
Copy link

@aneeshd16 updated the pull request.

# The first commit's message is:
Added FilesAdapter for Azure Blob Storage

# This is the 2nd commit message:

Updated Azure Blob Adapter to return Buffer instead of text

# This is the 3rd commit message:

Added tests for Azure Blob Storage

Added an extra "mount" parameter in FilesControllerTestFactory
@facebook-github-bot
Copy link

@aneeshd16 updated the pull request.

@facebook-github-bot
Copy link

@aneeshd16 updated the pull request.

@onosol
Copy link

onosol commented Mar 14, 2016

Is this getting merged into a new version soon? With this merged, I'm ready to go with a full Azure deployment :) Just curious what the status is.....thanks

@aneeshd16
Copy link
Contributor Author

Yep, me too. Just tell me what needs to be done so that this can be merged!

@facebook-github-bot
Copy link

@aneeshd16 updated the pull request.

@felixrieseberg
Copy link
Contributor

@onsol: In the meantime, we at Microsoft made an adapter you can use today.

var ParseServer         = require('parse-server').ParseServer;
var AzureStorageAdapter = require('parse-server-azure').FilesAdapter;

var account = 'YOUR_AZURE_STORAGE_ACCOUNT_NAME';
var container = 'YOUR_AZURE_STORAGE_CONTAINER_NAME';
var options = {
    accessKey: 'YOUR_ACCESS_KEY',
    directAccess: false // If set to true, files will be served by Azure Blob Storage directly
}

var api = new ParseServer({
  appId: process.env.APP_ID || 'myAppId',
  serverURL: process.env.SERVER_URL || 'http://localhost:1337'
  (...)
  filesAdapter: new AzureStorageAdapter(account, container, options);
});

@onosol
Copy link

onosol commented Mar 15, 2016

@felixrieseberg
Will check it out...thanks!

@gfosco
Copy link
Contributor

gfosco commented Mar 21, 2016

Like #971 I'd really like to see this become its own module on npm, instead of being included in the core server. It's easier to test and maintain that way. For example: https://www.npmjs.com/package/parse-server-sendgrid-adapter

@felixrieseberg
Copy link
Contributor

...and @aneeshd16, we at Microsoft would love contributions over at https://www.npmjs.com/package/parse-server-azure ❤️!

@flovilmart
Copy link
Contributor

Closing as we pimped the adapter tests and moved all adapters to external packaged

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.

7 participants