-
Notifications
You must be signed in to change notification settings - Fork 44
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
Some adjustments for Storage Driver and SourceManager to run on the cloud #519
Some adjustments for Storage Driver and SourceManager to run on the cloud #519
Conversation
🦋 Changeset detectedLatest commit: a1bdca2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 17 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
8393d36
to
4c129f2
Compare
server We want to use some methods from these 2 packages in our Latitude cloud and we have to make it work
4c129f2
to
a1bdca2
Compare
const stream = await this.materializedStorage.createWriteStream(filename) | ||
const stream = (await this.materializedStorage.createWriteStream( | ||
filename, | ||
)) as Writable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why materializedStorage
is typed a any
. We must be doing something wrong.
get isS3Driver(): boolean { | ||
return this.type === StorageType.s3 | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this for? Can't you just compare the type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can, but this is nicer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is for the server to check if the driver is S3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the an abstract parent class like this should be agnostic to its children subclasses existence.
This is not as big of a deal as if it was on the BaseConnector though, here's only two subclasses (for now). Just OCD.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep pure ocd in relation to the size of the driver code xD
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bit of OCD. Everything else is nice 👌
Describe your changes
We want to use some methods from these 2 packages in our Latitude cloud and we have to make it work.
TODO