azure-sdk-for-js icon indicating copy to clipboard operation
azure-sdk-for-js copied to clipboard

Published 20 hours ago • verified publisher icon Azure

Integrate Asset-Sync Features

Open scbedd opened this issue 3 years ago • 2 comments

@harshanalluru @timovv Sorry for the delay here! I intended on having this ready for you by last Thursday but failures wait for no one :(

I have some of the node tests passing, but I'm seeing failures that make no sense to me. (all of the test-utils pass still)

It's claiming it can't find the file after restoring the recordings. I'm digging into those failures. Wondering if there is anything special about those test cases.

EDIT: I understand the failures now. Will require a proxy update which I'm executing on now. Please find gotchas on this PR Timo and Harsha, I'll get it green!

Resolves Azure/azure-sdk-tools#4256

scbedd avatar Oct 04 '22 22:10 scbedd

Thanks, @scbedd, will take a look.

HarshaNalluru avatar Oct 04 '22 22:10 HarshaNalluru

PR Resolving the test-proxy issue breaking these tests.

scbedd avatar Oct 05 '22 01:10 scbedd

API change check

API changes are not detected in this pull request.

azure-sdk avatar Nov 02 '22 00:11 azure-sdk

❤️ your commits on this Harsha. Thanks for the rewrite 👍

scbedd avatar Nov 14 '22 19:11 scbedd

Looks good! Thanks Harsha for the follow up.

I did have one broader follow-up question (but happy to merge as-is) -- now that we're enforcing that the assets.json must be at the package level, is there anything actually stopping us from calculating the assets path as ${relativeRecordingsPath()}/../assets.json? That would simplify a lot and would mean we wouldn't need to add the extra environment variable.

Will the .. remain unresolved? I'm pretty certain the test-proxy would handle it.

I don't mind any adjustments. At the end of the day @timovv if that's how you'd like it, throw a commit on here that changes the behavior. If everything checks out then 👍

scbedd avatar Nov 15 '22 17:11 scbedd