-
Notifications
You must be signed in to change notification settings - Fork 160
feat: Azure Blob Storage service #177
Conversation
|
@tbarlow12 please also add to description on what this PR is about |
PIC123
left a comment
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.
One small comment, but otherwise looks good!
| * @param blobName Blob to delete | ||
| */ | ||
| public async deleteFile(containerName: string, blobName: string): Promise<void> { | ||
| const blockBlobUrl = await this.getBlockBlobURL(containerName, blobName) |
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.
Can we make this one line like you have done with the deleteContainer method?
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.
That's a good question... Do they both get awaited?
mydiemho
left a comment
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 might be confused about how the blob sdk work, but there are undefined argument used
| const containerURL = this.getContainerURL(containerName); | ||
| do { | ||
| const listBlobsResponse = await containerURL.listBlobFlatSegment( | ||
| Aborter.none, |
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 see this in a few places here. What does it do?
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 believe marker is used for paging scenarios. It's undefined on the first iteration so the first page of data is returned.
| do { | ||
| const listContainersResponse = await this.getServiceURL().listContainersSegment( | ||
| Aborter.none, | ||
| marker, |
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.
same, marker is not defined first loop
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.
See above.
wbreza
left a comment
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.
Added a few comments - otherwise looks good.
| const containerURL = this.getContainerURL(containerName); | ||
| do { | ||
| const listBlobsResponse = await containerURL.listBlobFlatSegment( | ||
| Aborter.none, |
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 believe marker is used for paging scenarios. It's undefined on the first iteration so the first page of data is returned.
| do { | ||
| const listContainersResponse = await this.getServiceURL().listContainersSegment( | ||
| Aborter.none, | ||
| marker, |
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.
See above.
| * @param ext - Extension of files to filter on when retrieving files | ||
| * from container | ||
| */ | ||
| public async listFiles(containerName: string, ext?: string): Promise<string[]> { |
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.
Please validate input arguments using Guard to make sure they satisfy the method requirements.
| * Get ServiceURL object for Azure Blob Storage Account | ||
| */ | ||
| private getServiceURL(): ServiceURL { | ||
| const credential = this.credentials |
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.
Any reason not to use this.credentials directly vs split out into separate variable?
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.
Nope :)
Wrapper service for Azure Blob Storage functionality. To be used in update of deployment process for rollback capability. Resolves [AB#361]
Wrapper service for Azure Blob Storage functionality. To be used in update of deployment process for rollback capability.
Resolves [AB#361]