[storage-blob] Storage core v2 migration - #24141
Conversation
Jeff Fisher (xirzec)
added
Client
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
EmmaZhu-MSFT (@EmmaZhu) I moved the Core changes out of this PR to simplify things a bit. Are you planning to release Storage next week? I'd like to get this change merged ASAP so we can work on the remaining Storage SDKs. |
Sarangan Rajamanickam (sarangan12)
left a comment
Sarangan Rajamanickam (sarangan12)
left a comment
There was a problem hiding this comment.
Thanks a lot for these changes Jeff!!!!
| assert.fail(); | ||
| } catch (err: any) { | ||
| assert.equal(err.name, "AbortError"); | ||
| assert.equal(err.message, "The operation was aborted.", "Unexpected error caught: " + err); |
There was a problem hiding this comment.
Why removed this line? Is the message deleted or changed?
There was a problem hiding this comment.
The message is slightly different in browsers if I recall correctly, since it gets handled by fetch and thrown from there
| if (err.name === "AbortError") { | ||
| resolve(); | ||
| } else { | ||
| reject(new Error("Expected readableStreamBody to emit AbortError")); |
There was a problem hiding this comment.
I understand it's a rare case. Here may cause an exception thrown out from the test cases, I think we should do our best to avoid this?
There was a problem hiding this comment.
I'm confused about the concern. Since this promise is awaited by the test function, if it rejects with an error Mocha should catch it and fail the test. Or am I misunderstanding your comment?
| it("Retry Policy should work when on PARSE_ERROR with unclosed root tag", async () => { | ||
| let injectCounter = 0; | ||
| const injector = new InjectorPolicyFactory(() => { | ||
| const injector = injectorPolicy(() => { |
There was a problem hiding this comment.
We may keep this case to test its compatibility.
There was a problem hiding this comment.
I'm not sure I follow, but are you suggesting we add a test case with an old-style PolicyFactory to ensure compatibility of user defined policies? That seems like a good idea since today we get some coverage from the SDKs that depend on storage-blob, but after those are migrated this will go down.
|
/azp run js - storage-blob - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
### Packages impacted by this PR `@azure/storage-file-datalake` ### Issues associated with this PR #15813 ### Describe the problem that is addressed by this PR This PR migrates storage-file-datalake to the new core pipeline in the same fashion that storage-blob was recently migrated. There are no changes to the public surface and existing recorded tests still pass. ### Provide a list of related PRs _(if any)_ #24141
### Packages impacted by this PR `@azure/storage-file-share` ### Issues associated with this PR #15813 ### Describe the problem that is addressed by this PR This PR migrates storage-file-share to the new core pipeline in the same way that storage-blob and storage-file-datalake were migrated. There are no changes to the public surface and existing recorded tests still pass. ### Provide a list of related PRs _(if any)_ #24141, #24835
### Packages impacted by this PR `@azure/storage-file-share` ### Issues associated with this PR Azure#15813 ### Describe the problem that is addressed by this PR This PR migrates storage-file-share to the new core pipeline in the same way that storage-blob and storage-file-datalake were migrated. There are no changes to the public surface and existing recorded tests still pass. ### Provide a list of related PRs _(if any)_ Azure#24141, Azure#24835
8 participants
This is a full migration of storage-blob to core-rest-pipeline. Feedback would be greatly appreciated!
This PR includes:
Packages impacted by this PR
storage-blob
Known issues/limitations