Skip to content
This repository has been archived by the owner on Dec 7, 2022. It is now read-only.

Added azure blob block driver support #158

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

TheAzack9
Copy link

@TheAzack9 TheAzack9 commented Oct 12, 2020

Hello,

I know there already exists a pr with this change here (#124), but it is a year old and possibly a bit outdated. I have tried my best to try update it such that it supports azure blobs, but i might have missed something since i'm not too well versed in nodejs and not too familiar with the environment. :) I'm not sure if it is better that it is a separate driver package or included directly in flydrive. (I expect it could be pretty useful for others aswell)

I should've updated most of the docs and Readme's aswell.

See (#68) for additional info

@TheAzack9
Copy link
Author

Any feedback or thoughts?

@rijkvanzanten
Copy link
Contributor

@RomainLanz Any blockers on this?

@RomainLanz
Copy link
Member

Hey! 👋

Wow, this has been lost in all my notification....
Going to review it through the week!

Thanks for your contribution!

@TheAzack9
Copy link
Author

Any developments @RomainLanz ? I hate to nag, but the feature would really help ;)

@RomainLanz
Copy link
Member

Yes!

I see that there's a lot of code change that are not related to adding Azure Storage, could you please remove them?

@TheAzack9
Copy link
Author

TheAzack9 commented Nov 29, 2020

Which parts are you talking about? Most of the extra code changes are to document the Azure Storage support and adding tests.
The other change was that i had to make getStream async in order to support it with Azure Storage.

I'm just not quite sure which changes you are refering to 😅

If you just want a plain PR with only the changes necessary to add Azure Storage support, then i can fix that aswell :)

I noticed a small change that was wrong on my side, i will at least fix that part quickly. It was never my intention to remove a contributor...

@TheAzack9
Copy link
Author

@RomainLanz sorry for excessive mentions, but i need a response to continue 😅

@Xalag
Copy link

Xalag commented Jan 31, 2021

It would be good to have Azure Storage support in flydrive. So I'm also very interested in this functionality. Thanks for all the work you put in so far!

I think the change of getStream to an async function is the only one which is not related to Azure Storage support. This introduces a breaking change for the lib and should therefore not be done.
The getStream function could stay as non-async if a intermediate stream is used.
Could perhaps look like this within the AzureBlobWebServices.ts

import { PassThrough, Readable } from 'stream'

private async getStreamAsync(): Promise<Readable> {
    const stream = await this.$containerClient.getBlobClient(location).download();
    if(!stream.readableStreamBody) {
        throw handleError(new Error("Blobclient stream was not available"), location);
    }
    return stream.readableStreamBody;
}

public getStream(): NodeJS.ReadableStream {
    const intermediateStream = new PassThrough({ highWaterMark: 1 });
    try {
        getStreamAsync().then(stream => {
            stream.pipe(intermediateStream)
        }).catch(error => {
            intermediateStream.emit('error', error);
        })
    } catch (error) {
        intermediateStream.emit('error', error);
    }
    return intermediateStream
}

@RomainLanz
Copy link
Member

Hey @TheAzack9! 👋

Sorry to have took soooooo long to review this PR 😞.

Could you please write a small guide to setup an Azure account and create a container? I would need this to try your code on my machine and setup the CI after.

Thanks again!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants