[storage-file-datalake] Migrate to core-rest-pipeline by xirzec · Pull Request #24835 · Azure/azure-sdk-for-js

xirzec · 2023-02-11T00:05:08Z

Packages impacted by this PR

@azure/storage-file-datalake

Issues associated with this PR

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

azure-sdk · 2023-02-11T00:21:20Z

API change check

APIView has identified API level changes in this PR and created following API reviews.

azure-storage-file-datalake

xirzec · 2023-02-13T17:31:11Z

/azp run js - storage-file-datalake - tests

azure-pipelines · 2023-02-13T17:31:25Z

Azure Pipelines successfully started running 1 pipeline(s).

xirzec · 2023-02-14T16:36:41Z

/azp run js - storage-file-datalake - tests

azure-pipelines · 2023-02-14T16:36:55Z

Azure Pipelines successfully started running 1 pipeline(s).

EmmaZhu · 2023-02-16T07:05:47Z

@xirzec , I'm looking into this PR and may need to run some case manually. We have several cases which requires manually setting environments.

About the SAS token case failures, will you look into them? Or do you need me to look into the them?

xirzec · 2023-02-16T17:21:46Z

@EmmaZhu I have an update for the failing tests and I have been looking into an issue with the browser in live mode. I hope to have an updated version pushed soon.

I'll ping you when the live tests are passing and it's in a good state for manual testing. I appreciate you helping validate! 👍

xirzec · 2023-02-17T00:37:28Z

/azp run js - storage-file-datalake - tests

azure-pipelines · 2023-02-17T00:37:40Z

Azure Pipelines successfully started running 1 pipeline(s).

xirzec · 2023-02-17T18:48:29Z

@EmmaZhu I think it should be good for testing now! 👍

HarshaNalluru · 2023-02-21T22:11:39Z

sdk/storage/storage-blob/CHANGELOG.md

+
+- Migrated dependency on `@azure/core-http` to `@azure/core-rest-pipeline`.
+
 ## 12.13.0 (Unreleased)


remove this 12.13.0?

HarshaNalluru · 2023-02-21T22:12:31Z

sdk/storage/storage-file-datalake/CHANGELOG.md

+
+- Migrated dependency on `@azure/core-http` to `@azure/core-rest-pipeline`.
+
 ## 12.12.0 (Unreleased)


merge into 12.20.0?

I wasn't sure if this was going to ship out of the stable branch or not, definitely have to fix up the changelog before we release out of main

EmmaZhu · 2023-02-27T15:34:02Z

Hi @xirzec , I'm working on the validation.
Seems there are some required files not tracking by git, "git clean -dxf" would delete them and cause failures.

I also see some failure in one of the large file uploading case. I think I can send a PR to your branch later.

xirzec · 2023-02-27T15:45:54Z

Seems there are some required files not tracking by git, "git clean -dxf" would delete them and cause failures.

How strange. Do you know which files are having issues? I'm surprised it built in CI with untracked changes.

I also see some failure in one of the large file uploading case. I think I can send a PR to your branch later.

Sure! Also happy to take a look at a repro if you have a consistent one. 👍

EmmaZhu · 2023-02-28T07:32:19Z

sdk/storage/storage-file-datalake/review/storage-file-datalake.api.md

-        parsedHeaders: FileSystemListPathsHeaders;
-    };
-};
+export type ListPathsSegmentResponse = FileSystemListPathsHeaders & PathListModel;


Would this interface miss row response?
Would have the same question on several below response definitions like: PathCreateResponse, PathDeleteResponse, PathFlushDataResponse...

Looking into this, it seems that ListPathsSegmentResponse was not actually used anywhere in the public API surface, presumably because only FileSystemListPathsResponse is used instead. I've removed this unnecessary type.

I also fixed up the typings where _response wasn't being typed correctly for headers-only response models, namely:

PathCreateResponse
PathDeleteResponse
FileFlushResponse
FileUploadResponse
PathSetAccessControlResponse
PathSetPermissionsResponse

EmmaZhu · 2023-02-28T07:40:12Z

The failure is in test case upload with chunkSize = FILE_UPLOAD_MAX_CHUNK_SIZE should succeed but it's not caused by migration, the migration should be fine.

The 'git clean -dxf' issue might be caused by my local environment, the issue has gone on a totally new cloned environment.

I left one comment. We'd need to run some perf/resource consumption testing before releasing, but I don't think it's a blocking for this PR.

xirzec · 2023-02-28T22:52:09Z

I left one comment. We'd need to run some perf/resource consumption testing before releasing, but I don't think it's a blocking for this PR.

Yes, thank you for this! It was a good catch. @HarshaNalluru do we have existing perf tests for this package?

HarshaNalluru · 2023-02-28T23:06:33Z

@xirzec triggered the pipeline https://dev.azure.com/azure-sdk/internal/_build/results?buildId=2596390&view=results

### 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

xirzec added 10 commits February 7, 2023 22:58

regenerate client

06ec220

in progress fixing errors

948bedd

compiling

69817fb

fixup types

ee1755a

updated api.md

315fda0

fix some test issues

471d846

fix path param issue

d363038

tests passing on node

9ed9939

unblock browser tests

c8d47d9

tweak browser recordings to not have empty request body

0e08be9

xirzec added Client This issue points to a problem in the data-plane of the library. Storage Storage Service (Queues, Blobs, Files) labels Feb 11, 2023

xirzec self-assigned this Feb 11, 2023

xirzec requested review from EmmaZhu, XiaoningLiu, bterlson, jeremymeng, jiacfan, ljian3377 and vinjiang as code owners February 11, 2023 00:05

xirzec added 2 commits February 11, 2023 00:08

fix lint

f4d58c9

format

0ada497

unblock live tests on node

3ca5d6b

xirzec added 10 commits February 16, 2023 17:30

fix browser tests

fd023a9

policy-free approach to fixing pipeline issues

15eb483

fix SAS in connection string issue for browser

f465d04

revert recording changes for browser

d630990

fix recording for node

46d28e5

lint

e95c1ae

update review md

9d6b9d6

update dependency requirements

874f921

update changelog

8ab6e83

fix leaked internal class

cd3bb9a

HarshaNalluru reviewed Feb 21, 2023

View reviewed changes

EmmaZhu reviewed Feb 28, 2023

View reviewed changes

HarshaNalluru approved these changes Feb 28, 2023

View reviewed changes

PR feedback

febb1f5

EmmaZhu approved these changes Mar 1, 2023

View reviewed changes

xirzec merged commit 83d9b65 into Azure:main Mar 1, 2023

xirzec deleted the storageFileDatalake branch March 1, 2023 17:36

xirzec mentioned this pull request Jun 5, 2023

[storage-file-share] Migrate to core-rest-pipeline #26102

Merged


		- Migrated dependency on `@azure/core-http` to `@azure/core-rest-pipeline`.

		## 12.13.0 (Unreleased)


		- Migrated dependency on `@azure/core-http` to `@azure/core-rest-pipeline`.

		## 12.12.0 (Unreleased)

Conversation

xirzec commented Feb 11, 2023

Packages impacted by this PR

Issues associated with this PR

Describe the problem that is addressed by this PR

Provide a list of related PRs (if any)

Uh oh!

azure-sdk commented Feb 11, 2023

Uh oh!

xirzec commented Feb 13, 2023

Uh oh!

azure-pipelines bot commented Feb 13, 2023

Uh oh!

xirzec commented Feb 14, 2023

Uh oh!

azure-pipelines bot commented Feb 14, 2023

Uh oh!

EmmaZhu commented Feb 16, 2023

Uh oh!

xirzec commented Feb 16, 2023

Uh oh!

xirzec commented Feb 17, 2023

Uh oh!

azure-pipelines bot commented Feb 17, 2023

Uh oh!

xirzec commented Feb 17, 2023

Uh oh!

HarshaNalluru Feb 21, 2023

Choose a reason for hiding this comment

Uh oh!

HarshaNalluru Feb 21, 2023

Choose a reason for hiding this comment

Uh oh!

xirzec Feb 27, 2023

Choose a reason for hiding this comment

Uh oh!

EmmaZhu commented Feb 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xirzec commented Feb 27, 2023

Uh oh!

EmmaZhu Feb 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xirzec Feb 28, 2023

Choose a reason for hiding this comment

Uh oh!

EmmaZhu commented Feb 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xirzec commented Feb 28, 2023

Uh oh!

HarshaNalluru commented Feb 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

EmmaZhu commented Feb 27, 2023 •

edited

Loading

EmmaZhu Feb 28, 2023 •

edited

Loading

EmmaZhu commented Feb 28, 2023 •

edited

Loading